From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 24CEA4696C3 for ; Wed, 15 Apr 2020 22:41:52 +0300 (MSK) Date: Wed, 15 Apr 2020 22:41:49 +0300 From: Mergen Imeev Message-ID: <20200415194149.GA454@tarantool.org> References: <20200413041105.22576-1-roman.habibov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200413041105.22576-1-roman.habibov@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 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. 2. Isn't boolean always less that any number? > > 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. > test:finish_test() > -- > 2.21.0 (Apple Git-122) >