From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B27AF4696C3 for ; Fri, 17 Apr 2020 03:48:59 +0300 (MSK) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3594.4.19\)) From: Roman Khabibov In-Reply-To: <20200416081731.GA16144@tarantool.org> Date: Fri, 17 Apr 2020 03:48:57 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <6CC4ABB2-D49D-4721-B913-B999D70844FB@tarantool.org> References: <20200413041105.22576-1-roman.habibov@tarantool.org> <20200415194149.GA454@tarantool.org> <43EE02FF-1F00-4FBE-AC41-C8C58B390003@tarantool.org> <20200416081731.GA16144@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] sql: fix number and boolean sorting rules List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. > On Apr 16, 2020, at 11:17, Mergen Imeev wrote: >=20 > Hi! Thank you for fixes. See one comment below. >=20 > On Thu, Apr 16, 2020 at 03:34:29AM +0300, Roman Khabibov wrote: >> Hi! Thanks for the review. >>=20 >> Branch: = https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4697-scalar-bu= g >> Issue: https://github.com/tarantool/tarantool/issues/4697 >>=20 >> @ChangeLog >> - Fix the bug with wrong sorter order of boolean and integer >> within SCALAR. >>=20 >>> On Apr 15, 2020, at 22:41, Mergen Imeev = wrote: >>>=20 >>> Hi! Thank you for the patch! See 5 small comments below. >>>=20 >>> On Mon, Apr 13, 2020 at 07:11:05AM +0300, Roman Khabibov wrote: >>>> Make comparison rules in the sql sorter correct for number and >>>> boolean. Boolean should always be greater than any number. >>> 1. I think you should say that values compared here using >>> SCALAR rules. It makes no sence otherwise, since boolean >>> cannot be compared with number. >> Fixed. >>=20 >>> 2. Isn't boolean always less that any number? >> https://www.tarantool.io/en/doc/1.10/book/box/data_model/ >> See the "Indexed field types" table. > 1. Not sure about this table, I cannot see where is says that > boolean is greater than number. But, we can see the order if > we look at 'enum mp_class' in tuple_compare.cc. It says, that > nil < boolean < number < string < varbinary and so on. >=20 > Also, if we execute these commands: >=20 > box.execute('create table t(i scalar primary key);') > box.execute('insert into t values (true), (1), (1.5);') > box.execute('select * from t where i > true;') >=20 > we will receive this result: > --- > - metadata: > - name: I > type: scalar > rows: > - [1] > - [1.5] > ... >=20 > It shows that any number is more that any boolean. >=20 > Actually, in you tests order in the results also show this. Sorry, I mixed up the sorting order. Fixed. >>=20 >>>>=20 >>>> Closes #4697 >>>> --- >>> 3. Please include links to issue and branch. Also, >>> ChangeLog is missing. >>>> src/box/sql/vdbeaux.c | 9 ++++++--- >>>> test/sql-tap/sort.test.lua | 18 +++++++++++++++++- >>>> 2 files changed, 23 insertions(+), 4 deletions(-) >>>>=20 >>>> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c >>>> index b88726ea1..985c6ef54 100644 >>>> --- a/src/box/sql/vdbeaux.c >>>> +++ b/src/box/sql/vdbeaux.c >>>> @@ -3054,7 +3054,8 @@ sqlVdbeCompareMsgpack(const char **key1, >>>> rc =3D double_compare_uint64(pKey2->u.r, >>>> mem1.u.u, = -1); >>>> } else { >>>> - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; >>>> + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || >>>> + (pKey2->flags & MEM_Bool) !=3D 0 ? = 1 : -1; >>>> } >>>> break; >>>> } >>>> @@ -3072,7 +3073,8 @@ sqlVdbeCompareMsgpack(const char **key1, >>>> rc =3D = double_compare_uint64(-pKey2->u.r, >>>> -mem1.u.i, = 1); >>>> } else { >>>> - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; >>>> + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || >>>> + (pKey2->flags & MEM_Bool) !=3D 0 ? = 1 : -1; >>>> } >>>> break; >>>> } >>>> @@ -3096,7 +3098,8 @@ sqlVdbeCompareMsgpack(const char **key1, >>>> rc =3D +1; >>>> } >>>> } else { >>>> - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; >>>> + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || >>>> + (pKey2->flags & MEM_Bool) !=3D 0 ? = 1 : -1; >>>> } >>>> break; >>>> } >>>> diff --git a/test/sql-tap/sort.test.lua = b/test/sql-tap/sort.test.lua >>>> index 36074d6ef..15b397f1d 100755 >>>> --- a/test/sql-tap/sort.test.lua >>>> +++ b/test/sql-tap/sort.test.lua >>>> @@ -1,6 +1,6 @@ >>>> #!/usr/bin/env tarantool >>>> test =3D require("sqltester") >>>> -test:plan(60) >>>> +test:plan(61) >>>>=20 >>>> --!./tcltestrunner.lua >>>> -- 2001 September 15. >>>> @@ -923,4 +923,20 @@ box.internal.sql_create_function("cksum", = cksum) >>>> }) >>>>=20 >>>> end >>>> + >>>> +-- gh-4679 Make sure that boolean > any number within scalar. >>>> +test:do_execsql_test( >>>> + 18.1, >>>> + [[ >>>> + CREATE TABLE test (s1 INTEGER PRIMARY KEY, s2 SCALAR); >>>> + INSERT INTO test VALUES (0, 1); >>>> + INSERT INTO test VALUES (2, 1.0); >>>> + INSERT INTO test VALUES (3, true); >>>> + SELECT s2, typeof(s2) FROM test ORDER BY s2; >>>> + ]], { >>>> + -- <18.1> >>>> + true,"boolean",1,"integer",1,"double" >>>> + -- >>>> + }) >>>> + >>> 4. I think it was decided that we should create new file >>> for each new test. >>>=20 >>> 5. I think, it is worth to add result of SELECT where index >>> was used. The example we can see in text of the issue. >>> Also, we should mention comewhere in the comments, that the >>> result should be the same in case index was used and in case >>> it wasn't. >> diff --git a/test/sql-tap/gh-4697-scalar-bug.test.lua = b/test/sql-tap/gh-4697-scalar-bug.test.lua >> new file mode 100755 >> index 000000000..159e91fca >> --- /dev/null >> +++ b/test/sql-tap/gh-4697-scalar-bug.test.lua >> @@ -0,0 +1,35 @@ >> +#!/usr/bin/env tarantool >> +test =3D require("sqltester") >> +test:plan(2) >> + >> +-- >> +-- gh-4679: Make sure that boolean > any number within scalar. >> +-- Result with order by indexed (using index) and non-indexed >> +-- (using no index) must be the same. >> +-- >> +test:execsql [[ >> + CREATE TABLE test (s1 INTEGER PRIMARY KEY, s2 SCALAR UNIQUE, s3 = SCALAR); >> + INSERT INTO test VALUES (0, 1, 1); >> + INSERT INTO test VALUES (1, 1.1, 1.1); >> + INSERT INTO test VALUES (2, true, true); >> +]] >> + >> +test:do_execsql_test( >> + "scalar-bug-1.0", >> + [[ >> + SELECT s2, typeof(s2) FROM test ORDER BY s2; >> + ]], { >> + -- >> + true,"boolean",1,"integer",1.1,"double" >> + }) >> + >> +test:do_execsql_test( >> + "scalar-bug-2.0", >> + [[ >> + SELECT s3, typeof(s3) FROM test ORDER BY s3; >> + ]], { >> + -- >> + true,"boolean",1,"integer",1.1,"double" >> + }) >> + >> +test:finish_test() >>=20 >>>> test:finish_test() >>>> --=20 >>>> 2.21.0 (Apple Git-122) >>=20 >> commit 9ea60156e72dc45dbea66c9e614769b8d8677e6f (HEAD -> = romanhabibov/gh-4697-scalar-bug) >> Author: Roman Khabibov >> Date: Mon Apr 13 05:03:54 2020 +0300 >>=20 >> sql: fix number and boolean SCALAR sorting rules >>=20 >> Make comparison SCALAR rules in the sql sorter correct for number >> and boolean. Boolean should always be greater than any number. >>=20 >> Closes #4697 >>=20 >> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c >> index b88726ea1..985c6ef54 100644 >> --- a/src/box/sql/vdbeaux.c >> +++ b/src/box/sql/vdbeaux.c >> @@ -3054,7 +3054,8 @@ sqlVdbeCompareMsgpack(const char **key1, >> rc =3D double_compare_uint64(pKey2->u.r, >> mem1.u.u, = -1); >> } else { >> - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; >> + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || >> + (pKey2->flags & MEM_Bool) !=3D 0 ? = 1 : -1; >> } >> break; >> } >> @@ -3072,7 +3073,8 @@ sqlVdbeCompareMsgpack(const char **key1, >> rc =3D = double_compare_uint64(-pKey2->u.r, >> -mem1.u.i, = 1); >> } else { >> - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; >> + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || >> + (pKey2->flags & MEM_Bool) !=3D 0 ? = 1 : -1; >> } >> break; >> } >> @@ -3096,7 +3098,8 @@ sqlVdbeCompareMsgpack(const char **key1, >> rc =3D +1; >> } >> } else { >> - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; >> + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || >> + (pKey2->flags & MEM_Bool) !=3D 0 ? = 1 : -1; >> } >> break; >> } >> diff --git a/test/sql-tap/gh-4697-scalar-bug.test.lua = b/test/sql-tap/gh-4697-scalar-bug.test.lua >> new file mode 100755 >> index 000000000..159e91fca >> --- /dev/null >> +++ b/test/sql-tap/gh-4697-scalar-bug.test.lua >> @@ -0,0 +1,35 @@ >> +#!/usr/bin/env tarantool >> +test =3D require("sqltester") >> +test:plan(2) >> + >> +-- >> +-- gh-4679: Make sure that boolean > any number within scalar. >> +-- Result with order by indexed (using index) and non-indexed >> +-- (using no index) must be the same. >> +-- >> +test:execsql [[ >> + CREATE TABLE test (s1 INTEGER PRIMARY KEY, s2 SCALAR UNIQUE, s3 = SCALAR); >> + INSERT INTO test VALUES (0, 1, 1); >> + INSERT INTO test VALUES (1, 1.1, 1.1); >> + INSERT INTO test VALUES (2, true, true); >> +]] >> + >> +test:do_execsql_test( >> + "scalar-bug-1.0", >> + [[ >> + SELECT s2, typeof(s2) FROM test ORDER BY s2; >> + ]], { >> + -- >> + true,"boolean",1,"integer",1.1,"double" >> + }) >> + >> +test:do_execsql_test( >> + "scalar-bug-2.0", >> + [[ >> + SELECT s3, typeof(s3) FROM test ORDER BY s3; >> + ]], { >> + -- >> + true,"boolean",1,"integer",1.1,"double" >> + }) >> + >> +test:finish_test( commit 8a2133d84b88a050181d858d6109abf3635a121b (HEAD -> = romanhabibov/gh-4697-scalar-bug, origin/romanhabibov/gh-4697-scalar-bug) Author: Roman Khabibov Date: Mon Apr 13 05:03:54 2020 +0300 sql: fix number and boolean SCALAR sorting rules =20 Make comparison SCALAR rules in the sql sorter correct for number and boolean. Boolean should always follow before any number. =20 Closes #4697 diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index b88726ea1..985c6ef54 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -3054,7 +3054,8 @@ sqlVdbeCompareMsgpack(const char **key1, rc =3D double_compare_uint64(pKey2->u.r, mem1.u.u, = -1); } else { - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || + (pKey2->flags & MEM_Bool) !=3D 0 ? = 1 : -1; } break; } @@ -3072,7 +3073,8 @@ sqlVdbeCompareMsgpack(const char **key1, rc =3D = double_compare_uint64(-pKey2->u.r, -mem1.u.i, = 1); } else { - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || + (pKey2->flags & MEM_Bool) !=3D 0 ? = 1 : -1; } break; } @@ -3096,7 +3098,8 @@ sqlVdbeCompareMsgpack(const char **key1, rc =3D +1; } } else { - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || + (pKey2->flags & MEM_Bool) !=3D 0 ? = 1 : -1; } break; } diff --git a/test/sql-tap/gh-4697-scalar-bug.test.lua = b/test/sql-tap/gh-4697-scalar-bug.test.lua new file mode 100755 index 000000000..6f600f27b --- /dev/null +++ b/test/sql-tap/gh-4697-scalar-bug.test.lua @@ -0,0 +1,35 @@ +#!/usr/bin/env tarantool +test =3D require("sqltester") +test:plan(2) + +-- +-- gh-4679: Make sure that boolean follow before any number within +-- scalar. Result with order by indexed (using index) and +-- non-indexed (using no index) must be the same. +-- +test:execsql [[ + CREATE TABLE test (s1 INTEGER PRIMARY KEY, s2 SCALAR UNIQUE, s3 = SCALAR); + INSERT INTO test VALUES (0, 1, 1); + INSERT INTO test VALUES (1, 1.1, 1.1); + INSERT INTO test VALUES (2, true, true); +]] + +test:do_execsql_test( + "scalar-bug-1.0", + [[ + SELECT s2, typeof(s2) FROM test ORDER BY s2; + ]], { + -- + true,"boolean",1,"integer",1.1,"double" + }) + +test:do_execsql_test( + "scalar-bug-2.0", + [[ + SELECT s3, typeof(s3) FROM test ORDER BY s3; + ]], { + -- + true,"boolean",1,"integer",1.1,"double" + }) + +test:finish_test()