[tarantool-patches] Re: [PATCH 3/8] sql: remove numeric affinity

["n.pettik" <korablev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4477 bytes --]


> On 16/01/2019 17:26, n.pettik wrote:
>>>> Numeric affinity in SQLite means the same as real, except that it
>>>> forces integer values into floating point representation in case
>>>> it can be converted without loss (e.g. 2.0 -> 2).
>>>> Since in Tarantool core there is no difference between numeric and real
>>>> values (both are stored as values of type NUMBER), lets remove numeric
>>>> affinity and use instead real.
>>>> The only real pitfall is implicit conversion mentioned above.
>>>> What is more, vinyl engine complicates problem since it relies
>>>> on data encoding (i.e. whether it is encoded as MP_INT or MP_FLOAT).
>>>> For instance, if we encode 1.0 as MP_FLOAT during insertion, we won't
>>>> be able to use iterators from Lua, since they implicitly change type of
>>>> 1.0 and pass it to the iterator as MP_INT.  Solution to this problem is
>>>> simple: lets always attempt at encoding floats as ints if conversion
>>>> takes place without loss. This is a straightforward approach, but to
>>>> implement it we need to care about reversed (decoding) situation.
>>> 
>>> The bug is confirmed: https://github.com/tarantool/tarantool/issues/3907
>>> 
>>> I agree with Kostja - it is just a bug, that Vinyl treats differently
>>> integers and their double casts. It should not affect design decisions
>>> of this patchset.
>> Ok, we may consider part of this patch as a workaround.
>> Now affinity removal would significantly help me to fix
>> other issues connected with strict typing (e.g. implicit casts).
>> afterwards code introduced in this commit may be simplified/removed.
>> I don’t know what priority of #3907 issue (milestone is 2.1.1 but
>> we know that sometimes it may take a while).
> 
> As we agreed, it is just a Vinyl bug, and now this part of the
> commit message looks strange:
> 
> "
>    The only real pitfall is implicit conversion mentioned above.
>    What is more, vinyl engine complicates problem since it relies
>    on data encoding (i.e. whether it is encoded as MP_INT or MP_FLOAT).
>    For instance, if we encode 1.0 as MP_FLOAT during insertion, we won't
>    be able to use iterators from Lua, since they implicitly change type of
>    1.0 and pass it to the iterator as MP_INT.
> "
> 
> Here you've stated that Vinyl even for number indexes sees a difference
> between 2.0 and 2, but it is wrong (in an ideal world, but in our it is
> just a bug). It is better to write here about not a temporary bug, but
> about, for instance, the example I've showed you below.
> 
>>> But there is another reason why we can't pass *.0 as an iterator value -
>>> our fast comparators (TupleCompare, TupleCompareWithKey) are designed to
>>> work with only values of same MP_ type. They do not use slow
>>> tuple_compare_field() which is able to compare double and integer.
>> Yep, it is sad. I can’t say anything more now,I need to "think about it”.
> 
> Please, use this example or find another to support your decision
> always 'integerifying' float numbers having zero fraction.

Ok, I replaced original paragraph with yours.

>>>> diff --git a/src/box/sql.c b/src/box/sql.c
>>>> index a06c50dca..a498cd8fe 100644
>>>> --- a/src/box/sql.c
>>>> +++ b/src/box/sql.c
>>>> @@ -376,14 +376,18 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
>>>>  	for (uint32_t i = 0; i < field_count; ++i) {
>>>>  		struct key_part_def *part = &ephemer_key_parts[i];
>>>>  		part->fieldno = i;
>>>> -		part->type = FIELD_TYPE_SCALAR;
>>>>  		part->nullable_action = ON_CONFLICT_ACTION_NONE;
>>>>  		part->is_nullable = true;
>>>>  		part->sort_order = SORT_ORDER_ASC;
>>>> -		if (def != NULL && i < def->part_count)
>>>> +		if (def != NULL && i < def->part_count) {
>>>> +			assert(def->parts[i].type < field_type_MAX);
>>>> +			part->type = def->parts[i].type != FIELD_TYPE_ANY ?
>>>> +				     def->parts[i].type : FIELD_TYPE_SCALAR;
>>>>  			part->coll_id = def->parts[i].coll_id;
>>> 
>>> 1. How can key_part have FIELD_TYPE_ANY? We have no comparators for ANY
>>> type, it is impossible, isn't it?
>> We don’t, and that is why I replace ANY with SCALAR.
> 
> No, you still check for "def->parts[i].type != FIELD_TYPE_ANY", and I
> can not understand how is it possible. struct key_def can not have
> FIELD_TYPE_ANY in its parts.

Now this problem is fixed in the next patches.
In this one it can’t be fixed with ease.


[-- Attachment #2: Type: text/html, Size: 24909 bytes --]