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 772CB245B6 for ; Mon, 28 Jan 2019 11:39:45 -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 yfgKPS0p49HR for ; Mon, 28 Jan 2019 11:39:45 -0500 (EST) Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 6735220EF7 for ; Mon, 28 Jan 2019 11:39:44 -0500 (EST) From: "n.pettik" Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_C5C75AE0-E30C-4955-923F-FF1E6D979EFE" Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH 3/8] sql: remove numeric affinity Date: Mon, 28 Jan 2019 19:39:41 +0300 In-Reply-To: References: <58b4d75729413f02134c72886ecbc749e510a1d1.1545987214.git.korablev@tarantool.org> <28e6da44-2d1e-87b5-741d-a791750db6c4@tarantool.org> 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: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy --Apple-Mail=_C5C75AE0-E30C-4955-923F-FF1E6D979EFE Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 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. >>>=20 >>> The bug is confirmed: = https://github.com/tarantool/tarantool/issues/3907 >>>=20 >>> 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=E2=80=99t know what priority of #3907 issue (milestone is 2.1.1 = but >> we know that sometimes it may take a while). >=20 > As we agreed, it is just a Vinyl bug, and now this part of the > commit message looks strange: >=20 > " > 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. > " >=20 > 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. >=20 >>> 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=E2=80=99t say anything more now,I need to = "think about it=E2=80=9D. >=20 > 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 =3D 0; i < field_count; ++i) { >>>> struct key_part_def *part =3D &ephemer_key_parts[i]; >>>> part->fieldno =3D i; >>>> - part->type =3D FIELD_TYPE_SCALAR; >>>> part->nullable_action =3D ON_CONFLICT_ACTION_NONE; >>>> part->is_nullable =3D true; >>>> part->sort_order =3D SORT_ORDER_ASC; >>>> - if (def !=3D NULL && i < def->part_count) >>>> + if (def !=3D NULL && i < def->part_count) { >>>> + assert(def->parts[i].type < field_type_MAX); >>>> + part->type =3D def->parts[i].type !=3D = FIELD_TYPE_ANY ? >>>> + def->parts[i].type : = FIELD_TYPE_SCALAR; >>>> part->coll_id =3D def->parts[i].coll_id; >>>=20 >>> 1. How can key_part have FIELD_TYPE_ANY? We have no comparators for = ANY >>> type, it is impossible, isn't it? >> We don=E2=80=99t, and that is why I replace ANY with SCALAR. >=20 > No, you still check for "def->parts[i].type !=3D 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=E2=80=99t be fixed with ease. --Apple-Mail=_C5C75AE0-E30C-4955-923F-FF1E6D979EFE Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

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=E2=80=99t 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=E2=80=99t say anything = more now,I need to "think about it=E2=80=9D.

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 =3D 0; i < = field_count; ++i) {
  struct key_part_def *part =3D = &ephemer_key_parts[i];
  = part->fieldno =3D i;
- = part->type =3D FIELD_TYPE_SCALAR;
  = part->nullable_action =3D ON_CONFLICT_ACTION_NONE;
  part->is_nullable =3D true;
  = part->sort_order =3D SORT_ORDER_ASC;
- if (def = !=3D NULL && i < def->part_count)
+ if (def = !=3D NULL && i < def->part_count) {
+ = assert(def->parts[i].type < field_type_MAX);
+ = = = part->type =3D def->parts[i].type !=3D FIELD_TYPE_ANY ?
+ = = = =      def-&= gt;parts[i].type : FIELD_TYPE_SCALAR;
  = part->coll_id =3D 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=E2=80=99t, and = that is why I replace ANY with SCALAR.

No, you still check for = "def->parts[i].type !=3D 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=E2=80=99t be = fixed with ease.

= --Apple-Mail=_C5C75AE0-E30C-4955-923F-FF1E6D979EFE--