From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7FA9A42EF5C for ; Tue, 30 Jun 2020 14:50:57 +0300 (MSK) Date: Tue, 30 Jun 2020 11:50:56 +0000 From: Nikita Pettik Message-ID: <20200630115056.GA31599@tarantool.org> References: <21d7145c1929bc4606c56e9a566477f248637ed1.1593096639.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <21d7145c1929bc4606c56e9a566477f248637ed1.1593096639.git.imeevma@gmail.com> Subject: Re: [Tarantool-patches] [PATCH v3 2/8] sql: change implicit cast for assignment List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org On 25 Jun 18:17, imeevma@tarantool.org wrote: > On 22.06.2020 11:48, Nikita Pettik wrote: > > On 17 Jun 15:36, imeevma@tarantool.org wrote: > >> This patch removes type changing from OP_MakeRecord. > > > > Please reflect user-visible changes in commit message. > > > Fixed. Seems to be the next patch.. > On 22.06.2020 12:32, Nikita Pettik wrote: > > On 17 Jun 15:36, imeevma@tarantool.org wrote: > >> This patch removes implicit cast from STRING to numeric > >> and vice versa of left operand of IN operator. > >> > >> Part of #4230 > >> Part of #4692 > >> --- > >> src/box/sql/expr.c | 2 +- > >> test/sql-tap/in3.test.lua | 14 +----- > >> test/sql-tap/subquery.test.lua | 69 +--------------------------- > >> test/sql-tap/tkt-80e031a00f.test.lua | 4 +- > >> test/sql/boolean.result | 12 ++--- > >> 5 files changed, 11 insertions(+), 90 deletions(-) > > On 22.06.2020 13:07, Nikita Pettik wrote: > > On 17 Jun 15:36, imeevma@tarantool.org wrote: > >> This patch replaces mem_apply_type() with mem_check_types() in > >> OP_MustBeInt, which allows to remove implicit cast in some places, > >> for example, in the IN statement. > >> > >> Part of #4230 > >> --- > >> src/box/sql/vdbe.c | 2 +- > >> test/sql-tap/e_select1.test.lua | 17 +- > >> test/sql-tap/in4.test.lua | 97 +-- > >> test/sql-tap/join.test.lua | 1 - > >> test/sql-tap/limit.test.lua | 2 +- > >> test/sql-tap/tkt-9a8b09f8e6.test.lua | 24 +- > >> test/sql-tap/tkt-fc7bd6358f.test.lua | 111 ---- > >> test/sql-tap/transitive1.test.lua | 4 +- > >> test/sql-tap/whereB.test.lua | 900 --------------------------- > >> test/sql-tap/whereC.test.lua | 8 +- > >> 10 files changed, 19 insertions(+), 1147 deletions(-) > >> delete mode 100755 test/sql-tap/tkt-fc7bd6358f.test.lua > >> delete mode 100755 test/sql-tap/whereB.test.lua > >> > > New patch: > > commit 21d7145c1929bc4606c56e9a566477f248637ed1 > Author: Mergen Imeev > Date: Wed May 27 13:49:11 2020 +0300 > > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 37283e506..f1d0345f9 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -397,6 +397,15 @@ sql_value_to_diag_str(sql_value *value); > enum mp_type > sql_value_type(sql_value *); > > +/* > + * Return the MP_type of the value of the MEM. > + * > + * @param mem MEM with the correct MEM_type. > + * @retval MP_type of the value. > + */ > +enum mp_type > +sql_value_mp_type(struct Mem *mem); It's not sql_value *, it's struct Mem. So the name of function is misleading. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 950f72ddd..009d577d2 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -417,6 +417,143 @@ sql_value_apply_type( > mem_apply_type((Mem *) pVal, type); > } > > + * > + * @param mem The MEM that contains the value to check. > + * @param type The type to check. > + * @retval 0 if the MEM_type of the value ​​and the given type are Wrong symbols. > + * compatible, -1 otherwise. > + */ > +static int > +mem_check_type(struct Mem *mem, enum field_type type) mem_test_type_compatibility mem_check_type_consistency mem_is_type_compatible I'd prefer last one and change return value to boolean. Then it turns into simple return field_mp_plain_type_is_compatible().. > + enum mp_type mp_type = sql_value_mp_type(mem); > + assert(mp_type < MP_EXT); > + if (field_mp_plain_type_is_compatible(type, mp_type, true)) > + return 0; > + return -1; > +} > + > +/** > + * Convert the numeric value contained in MEM to double. If the > + * is_precise flag is set, the conversion will succeed only if it > + * is lossless. > + * > + * @param mem The MEM that contains the numeric value. > + * @param is_precise Flag. > + * @retval 0 if the conversion was successful, -1 otherwise. > + */ > +static int > +mem_convert_to_double(struct Mem *mem, bool is_precise) > +{ > + if ((mem->flags & (MEM_Int | MEM_UInt)) == 0) > + return -1; > + if ((mem->flags & MEM_Int) != 0) { > + int64_t i = mem->u.i; > + double d = (double)i; > + if (!is_precise || i == (int64_t)d) > + mem_set_double(mem, d); > + else > + return -1; > + } else { > + uint64_t u = mem->u.u; > + double d = (double)u; > + if (!is_precise || u == (uint64_t)d) > + mem_set_double(mem, d); > + else > + return -1; > + } > + mem->field_type = FIELD_TYPE_DOUBLE; Why not incorparate field_type assingment into mem_set_double()? The same concerns other converting functions. > +/** > + * Convert the numeric value contained in MEM to another numeric > + * type. If the is_precise flag is set, the conversion will > + * succeed only if it is lossless. > + * > + * @param mem The MEM that contains the numeric value. > + * @param type The type to convert to. > + * @param is_precise Flag. > + * @retval 0 if the conversion was successful, -1 otherwise. > + */ > +static int > +mem_convert_numeric(struct Mem *mem, enum field_type type, bool is_precise) mem_convert_to_numeric ? mem_cast_to_numeric > +{ > + if (!sql_type_is_numeric(type) || > + (mem->flags & (MEM_Real | MEM_Int | MEM_UInt)) == 0) It's somehow unnatural passing to _numeric function non-numeric type to convert to. Instead let's use this function properly and replace branching with assertion. > + return -1; > + if (type == FIELD_TYPE_NUMBER) > + return 0; > + if (type == FIELD_TYPE_DOUBLE) > + return mem_convert_to_double(mem, is_precise); > + if (type == FIELD_TYPE_UNSIGNED) > + return mem_convert_to_unsigned(mem, is_precise); > + assert(type == FIELD_TYPE_INTEGER); > + return mem_convert_to_integer(mem, is_precise); > +} > + > /* > * pMem currently only holds a string type (or maybe a BLOB that we can > * interpret as a string if we want to). Compute its corresponding > @@ -2773,6 +2910,36 @@ case OP_ApplyType: { > break; > } > > +/* Opcode: CheckType P1 P2 * P4 * > + * Synopsis: type(r[P1@P2]) > + * > + * Check that types of P2 registers starting from register > + * P1 are compatible with given with given field types in P4. -> double 'with given' > + * If the MEM_type of the value and the given type are > + * incompatible, but both are numeric, this opcode attempts to Incompatible but numeric? Could you please be more specific. > + * convert the value to the type. > + */ > +case OP_ImplicitCast: { > + enum field_type *types = pOp->p4.types; > + assert(types != NULL); > + assert(types[pOp->p2] == field_type_MAX); > + pIn1 = &aMem[pOp->p1]; > + enum field_type type; > + while((type = *(types++)) != field_type_MAX) { > + assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]); > + assert(memIsValid(pIn1)); > + if (mem_check_type(pIn1, type) != 0 && > + mem_convert_numeric(pIn1, type, false) != 0) { > + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, > + sql_value_to_diag_str(pIn1), > + field_type_strs[type]); > + goto abort_due_to_error; > + } > + pIn1++; > + }