From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 2F88A445320 for ; Tue, 7 Jul 2020 00:27:08 +0300 (MSK) Date: Mon, 6 Jul 2020 21:27:07 +0000 From: Nikita Pettik Message-ID: <20200706212706.GB22427@tarantool.org> References: <21d7145c1929bc4606c56e9a566477f248637ed1.1593096639.git.imeevma@gmail.com> <20200630115056.GA31599@tarantool.org> <20200705142656.GA135859@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200705142656.GA135859@tarantool.org> 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: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org On 05 Jul 17:26, Mergen Imeev wrote: > Hi! Thank you for the review. My answers and new patch > below. Sorry, I didn't include diff. > > > commit 21d7145c1929bc4606c56e9a566477f248637ed1 > > > Author: Mergen Imeev > > > Date: Wed May 27 13:49:11 2020 +0300 > > > > > > + 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. > > > I think this should be done in #4906 issue since it may > lead to not always simple to understand changes. Эм, не очень понял как трактовать твой ответ. Типо тут работает, а если там засунуть, то что-то начинает падать..? 4906 немного про другое: в любом случае у нас в меме будет и field_type и mp_type. К тому же, и то, и это - рефакторинг и функциональных изменений быть не должно. > > > +/** > > > + * 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 > > > Used first option. > > > > +{ > > > + 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. > > > No, I think it is good as it is. More that that, due to > some other comments I added similar checks to all other > mem_convert_to>* functions. В остальных случаях ты туда тип вообще не передаешь. > My reason is: in all places we use this function we have > to check that field type and MEM_type are numeric, so it > make sence to move this check inside of the function. Еще раз: это полный идиотизм передавать ТИП (отличный от нумерика) к которому ты собираешься кастовать в функцию, которая по названию предполагает каст к нумерику. Считай, что я настаиваю на этом изменении. > > > + 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); > > > +} > > > + > > > /* > index 37283e506..bd0055668 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 > +mem_mp_type(struct Mem *mem); Я вытащил добавление этой функции в отдельный патч и добавил рефакторинг чтобы соеденить ее с sql_value_type(). https://github.com/tarantool/tarantool/tree/np/gh-3809-remastered Если ты ОК, я пушну вне очереди в мастер. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 950f72ddd..1386ea2c0 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > + 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; > + return 0; > +} > + > + * P1 are compatible with given field types in P4. > + * If the MEM_type of the value and the given type are > + * incompatible according to field_mp_plain_type_is_compatible(), > + * but both are numeric, this opcode attempts to 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); > + for (int i = 0; types[i] != field_type_MAX; ++i) { > + enum field_type type = types[i]; > + pIn1 = &aMem[pOp->p1 + i]; > + assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]); > + assert(memIsValid(pIn1)); > + if (mem_is_type_compatible(pIn1, type)) > + continue; > + if (mem_convert_to_numeric(pIn1, type, false) == 0) Инвертируй, пожайлуйста условие: if (mem_convert_to_numeric(pIn1, type, false) !=) { //raise an error } Это будет больще соответствовать нашему кодстайлу. > + continue; > + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, > + sql_value_to_diag_str(pIn1), field_type_strs[type]); > + goto abort_due_to_error; > + } > + break; > +} > + > /* Opcode: MakeRecord P1 P2 P3 P4 P5 > * Synopsis: r[P3]=mkrec(r[P1@P2]) > * > diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c > index 4e103a653..b106ce6bd 100644 > --- a/src/box/sql/vdbeapi.c > +++ b/src/box/sql/vdbeapi.c > @@ -225,6 +225,33 @@ sql_value_type(sql_value *pVal) > } > } > > +enum mp_type > +mem_mp_type(struct Mem *mem) Еще раз: это не апишная функция (хотя бы в терминологии скулайта), ей место скорее в vdbemem.c. См. мой рефакторинг. > +{ > --- a/test/sql-tap/autoinc.test.lua > +++ b/test/sql-tap/autoinc.test.lua Тесты не смотрел, предполагая что нет изменений по сравнению с предыдущей версией.