From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 91BF64696C3 for ; Fri, 17 Apr 2020 09:35:15 +0300 (MSK) Date: Fri, 17 Apr 2020 09:35:13 +0300 From: Mergen Imeev Message-ID: <20200417063512.GA10928@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> <6CC4ABB2-D49D-4721-B913-B999D70844FB@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <6CC4ABB2-D49D-4721-B913-B999D70844FB@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: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for the fixes. I think that the your commit-message and comments sounds quite strange, but I am not sure that I am qualified enough to say that for sure. Please, send this patch to NIkita. Other than that, LGTM. On Fri, Apr 17, 2020 at 03:48:57AM +0300, Roman Khabibov wrote: > Hi! Thanks for the review. > > > On Apr 16, 2020, at 11:17, Mergen Imeev wrote: > > > > Hi! Thank you for fixes. See one comment below. > > > > On Thu, Apr 16, 2020 at 03:34:29AM +0300, Roman Khabibov wrote: > >> Hi! Thanks for the review. > >> > >> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4697-scalar-bug > >> Issue: https://github.com/tarantool/tarantool/issues/4697 > >> > >> @ChangeLog > >> - Fix the bug with wrong sorter order of boolean and integer > >> within SCALAR. > >> > >>> On Apr 15, 2020, at 22:41, Mergen Imeev wrote: > >>> > >>> Hi! Thank you for the patch! See 5 small comments below. > >>> > >>> 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. > >> > >>> 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. > > > > Also, if we execute these commands: > > > > 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;') > > > > we will receive this result: > > --- > > - metadata: > > - name: I > > type: scalar > > rows: > > - [1] > > - [1.5] > > ... > > > > It shows that any number is more that any boolean. > > > > Actually, in you tests order in the results also show this. > Sorry, I mixed up the sorting order. Fixed. > > >> > >>>> > >>>> 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(-) > >>>> > >>>> 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 = double_compare_uint64(pKey2->u.r, > >>>> mem1.u.u, -1); > >>>> } else { > >>>> - rc = (pKey2->flags & MEM_Null) ? +1 : -1; > >>>> + rc = (pKey2->flags & MEM_Null) != 0 || > >>>> + (pKey2->flags & MEM_Bool) != 0 ? 1 : -1; > >>>> } > >>>> break; > >>>> } > >>>> @@ -3072,7 +3073,8 @@ sqlVdbeCompareMsgpack(const char **key1, > >>>> rc = double_compare_uint64(-pKey2->u.r, > >>>> -mem1.u.i, 1); > >>>> } else { > >>>> - rc = (pKey2->flags & MEM_Null) ? +1 : -1; > >>>> + rc = (pKey2->flags & MEM_Null) != 0 || > >>>> + (pKey2->flags & MEM_Bool) != 0 ? 1 : -1; > >>>> } > >>>> break; > >>>> } > >>>> @@ -3096,7 +3098,8 @@ sqlVdbeCompareMsgpack(const char **key1, > >>>> rc = +1; > >>>> } > >>>> } else { > >>>> - rc = (pKey2->flags & MEM_Null) ? +1 : -1; > >>>> + rc = (pKey2->flags & MEM_Null) != 0 || > >>>> + (pKey2->flags & MEM_Bool) != 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 = require("sqltester") > >>>> -test:plan(60) > >>>> +test:plan(61) > >>>> > >>>> --!./tcltestrunner.lua > >>>> -- 2001 September 15. > >>>> @@ -923,4 +923,20 @@ box.internal.sql_create_function("cksum", cksum) > >>>> }) > >>>> > >>>> 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. > >>> > >>> 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 = 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() > >> > >>>> test:finish_test() > >>>> -- > >>>> 2.21.0 (Apple Git-122) > >> > >> commit 9ea60156e72dc45dbea66c9e614769b8d8677e6f (HEAD -> 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 > >> > >> Make comparison SCALAR rules in the sql sorter correct for number > >> and boolean. Boolean should always be greater than any number. > >> > >> 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 = double_compare_uint64(pKey2->u.r, > >> mem1.u.u, -1); > >> } else { > >> - rc = (pKey2->flags & MEM_Null) ? +1 : -1; > >> + rc = (pKey2->flags & MEM_Null) != 0 || > >> + (pKey2->flags & MEM_Bool) != 0 ? 1 : -1; > >> } > >> break; > >> } > >> @@ -3072,7 +3073,8 @@ sqlVdbeCompareMsgpack(const char **key1, > >> rc = double_compare_uint64(-pKey2->u.r, > >> -mem1.u.i, 1); > >> } else { > >> - rc = (pKey2->flags & MEM_Null) ? +1 : -1; > >> + rc = (pKey2->flags & MEM_Null) != 0 || > >> + (pKey2->flags & MEM_Bool) != 0 ? 1 : -1; > >> } > >> break; > >> } > >> @@ -3096,7 +3098,8 @@ sqlVdbeCompareMsgpack(const char **key1, > >> rc = +1; > >> } > >> } else { > >> - rc = (pKey2->flags & MEM_Null) ? +1 : -1; > >> + rc = (pKey2->flags & MEM_Null) != 0 || > >> + (pKey2->flags & MEM_Bool) != 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 = 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 > > Make comparison SCALAR rules in the sql sorter correct for number > and boolean. Boolean should always follow before any number. > > 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 = double_compare_uint64(pKey2->u.r, > mem1.u.u, -1); > } else { > - rc = (pKey2->flags & MEM_Null) ? +1 : -1; > + rc = (pKey2->flags & MEM_Null) != 0 || > + (pKey2->flags & MEM_Bool) != 0 ? 1 : -1; > } > break; > } > @@ -3072,7 +3073,8 @@ sqlVdbeCompareMsgpack(const char **key1, > rc = double_compare_uint64(-pKey2->u.r, > -mem1.u.i, 1); > } else { > - rc = (pKey2->flags & MEM_Null) ? +1 : -1; > + rc = (pKey2->flags & MEM_Null) != 0 || > + (pKey2->flags & MEM_Bool) != 0 ? 1 : -1; > } > break; > } > @@ -3096,7 +3098,8 @@ sqlVdbeCompareMsgpack(const char **key1, > rc = +1; > } > } else { > - rc = (pKey2->flags & MEM_Null) ? +1 : -1; > + rc = (pKey2->flags & MEM_Null) != 0 || > + (pKey2->flags & MEM_Bool) != 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 = 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() >