From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 6B0CD25E85 for ; Wed, 30 Jan 2019 08:04:30 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id sRb-HAJlfNUa for ; Wed, 30 Jan 2019 08:04:30 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B999A25DF3 for ; Wed, 30 Jan 2019 08:04:29 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 3/8] sql: remove numeric affinity References: <58b4d75729413f02134c72886ecbc749e510a1d1.1545987214.git.korablev@tarantool.org> <28e6da44-2d1e-87b5-741d-a791750db6c4@tarantool.org> From: Vladislav Shpilevoy Message-ID: <3f72dcc1-0a98-908e-ae1f-f603758d4129@tarantool.org> Date: Wed, 30 Jan 2019 16:04:26 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.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.