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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! See a comment below.

On 28/01/2019 19:39, n.pettik wrote:
> 
>> 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.
> 

What a problem can not be fixed? I did this:

diff --git a/src/box/sql.c b/src/box/sql.c
index 530ec2384..f53600837 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -387,8 +387,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
  		part->sort_order = SORT_ORDER_ASC;
  		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->type = def->parts[i].type;
  			part->coll_id = def->parts[i].coll_id;
  		} else {
  			part->coll_id = COLL_NONE;

And no test failed. This is because, as I said, key_part.type
is never ever can be ANY. I was not talking above about ANY
removal from all places, but from this particular one. And here
we can be sure, that key_part.type != FIELD_TYPE_ANY always.

Please, apply this.