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 BB456304BC for ; Thu, 6 Jun 2019 09:51:30 -0400 (EDT) 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 5xB_L2YC1SJq for ; Thu, 6 Jun 2019 09:51:30 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 E561C304A4 for ; Thu, 6 Jun 2019 09:51:29 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator From: "n.pettik" In-Reply-To: <91e73947-30b8-c172-f52c-be109034e0dd@tarantool.org> Date: Thu, 6 Jun 2019 16:51:26 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <789091b7acd99c908d26689f27c55f8b6dba3d16.1558700151.git.korablev@tarantool.org> <91e73947-30b8-c172-f52c-be109034e0dd@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 >> @@ -1128,8 +1129,9 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, = /* Complete information about the W >> } >> } >> /* Inequality constraint comes always at the end of = list. */ >> - if (pRangeStart !=3D NULL && >> - idx_def->key_def->parts[nEq].type =3D=3D = FIELD_TYPE_INTEGER) >> + enum field_type ineq_type =3D = idx_def->key_def->parts[nEq].type; >> + if (pRangeStart !=3D NULL && (ineq_type =3D=3D = FIELD_TYPE_INTEGER || >> + ineq_type =3D=3D = FIELD_TYPE_UNSIGNED)) >=20 > 1. Please, add a test for unsigned. I thought that it is quite obvious = - > everything needs a test, especially if the bug was found during a = review. Surely, forgot about that: diff --git a/test/sql/types.result b/test/sql/types.result index bba39e320..383f93c1a 100644 --- a/test/sql/types.result +++ b/test/sql/types.result @@ -1012,3 +1012,35 @@ box.execute("SELECT a FROM t1 WHERE a IS NULL AND = b IS NULL;") box.space.T1:drop() --- ... +format =3D {} +--- +... +format[1] =3D { name =3D 'ID', type =3D 'unsigned' } +--- +... +format[2] =3D { name =3D 'A', type =3D 'unsigned' } +--- +... +s =3D box.schema.create_space('T1', { format =3D format }) +--- +... +_ =3D s:create_index('pk') +--- +... +_ =3D s:create_index('sk', { parts =3D { 'A' } }) +--- +... +s:insert({ 1, 1 }) +--- +- [1, 1] +... +box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);") +--- +- metadata: + - name: A + type: unsigned + rows: [] +... +s:drop() +--- +... diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua index 16448436d..0218c5498 100644 --- a/test/sql/types.test.lua +++ b/test/sql/types.test.lua @@ -247,3 +247,14 @@ box.execute("SELECT a FROM t1 WHERE a =3D 1.5 AND b = IS NULL;") box.execute("SELECT a FROM t1 WHERE a IS NULL AND b IS NULL;") =20 box.space.T1:drop() + +format =3D {} +format[1] =3D { name =3D 'ID', type =3D 'unsigned' } +format[2] =3D { name =3D 'A', type =3D 'unsigned' } +s =3D box.schema.create_space('T1', { format =3D format }) +_ =3D s:create_index('pk') +_ =3D s:create_index('sk', { parts =3D { 'A' } }) +s:insert({ 1, 1 }) +box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);") + +s:drop() >>>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c >>>> index 977c0fced..e779729c7 100644 >>>> --- a/src/box/sql/wherecode.c >>>> +++ b/src/box/sql/wherecode.c >>>> @@ -1086,31 +1086,61 @@ sqlWhereCodeOneLoopStart(WhereInfo * = pWInfo, /* Complete information about the W >>>> start_constraints =3D 1; >>>> } >>>> struct index_def *idx_pk =3D space->index[0]->def; >>>> - int fieldno =3D idx_pk->key_def->parts[0].fieldno; >>>> - >>>> uint32_t pk_part_count =3D idx_pk->key_def->part_count; >>>> - if (pk_part_count =3D=3D 1 && >>>> - space->def->fields[fieldno].type =3D=3D = FIELD_TYPE_INTEGER) { >>>> - /* Right now INTEGER PRIMARY KEY is the only = option to >>>> - * get Tarantool's INTEGER column type. Need = special handling >>>> - * here: try to loosely convert FLOAT to INT. If = RHS type >>>> - * is not INT or FLOAT - skip this ites, i.e. = goto addrNxt. >>>> - */ >>>> - int limit =3D pRangeStart =3D=3D NULL ? nEq : = nEq + 1; >>>> - for (int i =3D 0; i < limit; i++) { >>>> - if (idx_def->key_def->parts[i].fieldno = =3D=3D >>>> - idx_pk->key_def->parts[0].fieldno) { >>>> - /* Here: we know for sure that = table has INTEGER >>>> - PRIMARY KEY, single column, = and Index we're >>>> - trying to use for scan = contains this column. */ >>>> - if (i < nEq) >>>> - sqlVdbeAddOp2(v, = OP_MustBeInt, regBase + i, addrNxt); >>>> - else >>>> - force_integer_reg =3D = regBase + i; >>>> - break; >>>> - } >>>> + /* >>>> + * Tarantool's iterator over integer fields doesn't >>>> + * tolerate floating point values. Hence, if term >>>> + * is equality comparison and value of operand is >>>> + * not integer, we can skip it since it always >>>> + * results in false: INT a =3D=3D 0.5 -> false; >>>> + * It is done using OP_MustBeInt facilities. >>>> + * In case term is greater comparison (a > ?), we >>>> + * should notify OP_SeekGT to process truncation of >>>> + * floating point value: a > 0.5 -> a >=3D 1; >>>> + * It is done by setting P5 flag for OP_Seek*. >>>> + * It is worth mentioning that we do not need >>>> + * this step when it comes for less comparison: >>>> + * key is NULL in this case: values are ordered >>>> + * as NULL, ... NULL, min_value, ..., so to >>>> + * fetch min value we pass NULL to GT iterator. >>>=20 >>> 4. I did not get the 'less comparator =3D=3D compare with NULL' >>> idea. Firstly you describe how to compare 'a > ?', this is >>> fully understandable. Then you, as I see, go to the 'a < ?' >>> case, but somewhy say, that it is the same as 'NULL < ?'. >>> Do not understand. >>=20 >> I say that in this case key which is passed to iterator is NULL >> and type of iterator is GE. >=20 > 2. Why? I still do not understand. What if I wrote 'a < 100'? you > do not replace 100 with NULL. But the comment says, that for any > 'less' operator a key is NULL: "when it comes for less comparison > key is NULL=E2=80=9D. Consider example: box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);") box.execute("INSERT INTO t1 VALUES (1, 1), (2, 2), (3, -1), (4, NULL), = (5, NULL);=E2=80=9D) box.execute("SELECT a FROM t1 WHERE a < 100;=E2=80=9D) Bytecode for this query is: 102> 0 Init 0 1 0 00 Start at 1 102> 1 IteratorOpen 1 1 0 space 00 index id =3D = 1, space ptr =3D space or reg[0]; unique_unnamed_T1_2 102> 2 Explain 0 0 0 SEARCH TABLE T1 USING COVERING = INDEX unique_unnamed_T1_2 (A 3 Null 0 1 0 00 r[1]=3DNULL 102> 4 SeekGT 1 10 1 1 00 key=3Dr[1] 102> 5 Integer 100 1 0 00 r[1]=3D100 102> 6 IdxGE 1 10 1 1 00 key=3Dr[1] 102> 7 Column 1 1 2 00 r[2]=3DT1.A 102> 8 ResultRow 2 1 0 00 output=3Dr[2] 102> 9 Next 1 6 0 00=20 102> 10 Halt 0 0 0 00=20 As you can see, SeekGT accepts NULL as a key for iterator. Then we iterate until search condition is true. So, basically 100 is replaced with NULL and not used as a key of iterator. I wanted to underline this fact in my comment. >> Consequently, pRangeStart is NULL. >> I=E2=80=99ve slightly updated this part of comment: >>=20 >> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c >> index 3025e5e6b..01ee9ce90 100644 >> --- a/src/box/sql/wherecode.c >> +++ b/src/box/sql/wherecode.c >> @@ -1089,10 +1089,10 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, = /* Complete information about the W >> * floating point value: a > 0.5 -> a >=3D 1; >> * It is done by setting P5 flag for OP_Seek*. >> * It is worth mentioning that we do not need >> - * this step when it comes for less comparison: >> - * key is NULL in this case: values are ordered >> - * as NULL, ... NULL, min_value, ..., so to >> - * fetch min value we pass NULL to GT iterator. >> + * this step when it comes for less (<) comparison >> + * of nullable field. Key is NULL in this case: >> + * values are ordered as NULL, ... NULL, min_value, >> + * so to fetch min value we pass NULL to GT iterator. >>=20 >>=20 >> @@ -1112,24 +1112,33 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, = /* Complete information about the W >> /* >> * OP_MustBeInt consider NULLs as >> * non-integer values, so firstly >> * check whether value is NULL or not. >> */ >> - seek_addr =3D sqlVdbeAddOp1(v, = OP_IsNull, >> + seek_addrs[i] =3D sqlVdbeAddOp1(v, = OP_IsNull, >> regBase); >=20 > 3. Now the indentation is broken. diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c index fde2efee0..4bd14bd5f 100644 --- a/src/box/sql/wherecode.c +++ b/src/box/sql/wherecode.c @@ -1130,7 +1130,7 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, = /* Complete information about the W * check whether value is NULL or not. */ seek_addrs[i] =3D sqlVdbeAddOp1(v, = OP_IsNull, - regBase); + regBase); sqlVdbeAddOp2(v, OP_MustBeInt, regBase + = i, addrNxt); start_types[i] =3D FIELD_TYPE_SCALAR;