From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 BEC234696C3 for ; Thu, 16 Apr 2020 11:17:34 +0300 (MSK) Date: Thu, 16 Apr 2020 11:17:31 +0300 From: Mergen Imeev Message-ID: <20200416081731.GA16144@tarantool.org> References: <20200413041105.22576-1-roman.habibov@tarantool.org> <20200415194149.GA454@tarantool.org> <43EE02FF-1F00-4FBE-AC41-C8C58B390003@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <43EE02FF-1F00-4FBE-AC41-C8C58B390003@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 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. > > >> > >> 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() > >