Re: [Tarantool-patches] [PATCH v3 2/8] sql: change implicit cast for assignment

[Mergen Imeev <imeevma@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 <imeevma@gmail.com>
>>>> 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
> Тесты не смотрел, предполагая что нет изменений по сравнению с
> предыдущей версией.
>
Да, тесты не изменились.