From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 6147F445320 for ; Tue, 7 Jul 2020 12:29:42 +0300 (MSK) References: <21d7145c1929bc4606c56e9a566477f248637ed1.1593096639.git.imeevma@gmail.com> <20200630115056.GA31599@tarantool.org> <20200705142656.GA135859@tarantool.org> <20200706212706.GB22427@tarantool.org> From: Mergen Imeev Message-ID: <5bbf04dc-addd-f188-f371-fba854f0a468@tarantool.org> Date: Tue, 7 Jul 2020 12:29:40 +0300 MIME-Version: 1.0 In-Reply-To: <20200706212706.GB22427@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org On 07.07.2020 00:27, Nikita Pettik wrote: > 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. > К тому же, и то, и это - рефакторинг и функциональных изменений быть > не должно. Не уверен, что это чистый рефакторинг, т.к. точно помны возникала проблема с тем, что MEM_type мог быть не совместимым с field_type т.к. field_type не изменился при переписывании мема. Стоит сделать это изменение для mem_set_* отдельным тикетом? > >>>> +/** >>>> + * 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 > Тесты не смотрел, предполагая что нет изменений по сравнению с > предыдущей версией. > Да, тесты не изменились.