Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mergen Imeev <imeevma@tarantool.org>
To: Nikita Pettik <korablev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v3 2/8] sql: change implicit cast for assignment
Date: Tue, 7 Jul 2020 12:29:40 +0300	[thread overview]
Message-ID: <5bbf04dc-addd-f188-f371-fba854f0a468@tarantool.org> (raw)
In-Reply-To: <20200706212706.GB22427@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
> Тесты не смотрел, предполагая что нет изменений по сравнению с
> предыдущей версией.
>
Да, тесты не изменились.

  reply	other threads:[~2020-07-07  9:29 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-25 15:17 [Tarantool-patches] [PATCH v3 0/8] Remove implicit cast imeevma
2020-06-25 15:17 ` [Tarantool-patches] [PATCH v3 1/8] sql: introduce mem_set_double() imeevma
2020-06-28 13:31   ` Nikita Pettik
2020-07-06 14:02     ` Nikita Pettik
2020-06-25 15:17 ` [Tarantool-patches] [PATCH v3 2/8] sql: change implicit cast for assignment imeevma
2020-06-30 11:50   ` Nikita Pettik
2020-07-05 14:26     ` Mergen Imeev
2020-07-06 21:27       ` Nikita Pettik
2020-07-07  9:29         ` Mergen Imeev [this message]
2020-07-07 15:35           ` Nikita Pettik
2020-07-10 10:49           ` Nikita Pettik
2020-06-25 15:17 ` [Tarantool-patches] [PATCH v3 3/8] sql: remove mem_apply_type() from OP_MakeRecord imeevma
2020-06-25 15:17 ` [Tarantool-patches] [PATCH v3 4/8] sql: replace ApplyType by CheckType for IN operator imeevma
2020-06-29 12:56   ` Nikita Pettik
2020-07-05 14:28     ` Mergen Imeev
2020-07-06 22:06       ` Nikita Pettik
2020-07-07 11:26         ` Mergen Imeev
2020-07-07 16:29           ` Nikita Pettik
2020-06-25 15:17 ` [Tarantool-patches] [PATCH v3 5/8] sql: remove mem_apply_type() from OP_MustBeInt imeevma
2020-06-29 13:29   ` Nikita Pettik
2020-07-05 14:29     ` Mergen Imeev
2020-06-25 15:17 ` [Tarantool-patches] [PATCH v3 6/8] sql: remove implicit cast for comparison imeevma
2020-06-29 23:51   ` Nikita Pettik
2020-07-05 14:47     ` Mergen Imeev
2020-07-06 23:11       ` Nikita Pettik
2020-06-25 15:17 ` [Tarantool-patches] [PATCH v3 7/8] sql: remove unused functions imeevma
2020-06-29 23:52   ` Nikita Pettik
2020-07-05 14:50     ` Mergen Imeev
2020-06-25 15:17 ` [Tarantool-patches] [PATCH v3 8/8] sql: show value and its type in type mismatch error imeevma
2020-06-30  0:22   ` Nikita Pettik
2020-07-05 15:03     ` Mergen Imeev
2020-07-06 21:44       ` Nikita Pettik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5bbf04dc-addd-f188-f371-fba854f0a468@tarantool.org \
    --to=imeevma@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v3 2/8] sql: change implicit cast for assignment' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox