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 7B2B525829 for ; Tue, 22 Jan 2019 10:41:48 -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 ib-md2RpVItb for ; Tue, 22 Jan 2019 10:41:48 -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 375E225380 for ; Tue, 22 Jan 2019 10:41:48 -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: Date: Tue, 22 Jan 2019 18:41:34 +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! Thanks for the fixes! 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. > > The only workaround I can come up right now with is to add to > every OP_Found/OP_Seek etc opcode OP_PromoteType. > It would attempt at fetching space from cursor and gently > applying types from format to record to be passed to iterator. > But this doesn’t seem to be acceptable solution for many reasons. I think, the implementation in this patch is ok. It is not necessary to create new opcodes, nor think up another solution. > > Otherwise, I have no idea how to determine required conversions > *.0 -> * without execution context. It is impossible of course. You can not learn what a value you will fetch from cursors before VDBE execution. > >> Moreover, I think, we should forbid implicit *.0 -> * This is why we >> designed strict typing, isn't it? > > It is too strict rule, ANSI allows this kind of conversion. > The only restriction is that there shouldn’t be precision loss > during conversion. It has been already discussed with > Konstantin and Peter Gulutzan, I inline part of that thread > (Topic “[dev] Re: Casts") Understood, ok. Thanks for the investigation. >>> 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. > >> >>> - else >>> + } else { >>> part->coll_id = COLL_NONE; >>> + part->type = FIELD_TYPE_SCALAR; >>> + } >>> } >>> struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts, >>> field_count);