[tarantool-patches] Re: [PATCH 3/8] sql: remove numeric affinity
n.pettik
korablev at tarantool.org
Mon Jan 28 19:39:41 MSK 2019
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20190128/046209ed/attachment.html>
More information about the Tarantool-patches
mailing list