From: Mergen Imeev <imeevma@tarantool.org>
To: Roman Khabibov <roman.habibov@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] sql: fix number and boolean sorting rules
Date: Fri, 17 Apr 2020 09:35:13 +0300 [thread overview]
Message-ID: <20200417063512.GA10928@tarantool.org> (raw)
In-Reply-To: <6CC4ABB2-D49D-4721-B913-B999D70844FB@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 <imeevma@tarantool.org> 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 <imeevma@tarantool.org> 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"
> >>>> + -- </18.1>
> >>>> + })
> >>>> +
> >>> 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;
> >> + ]], {
> >> + -- <scalar-bug-1.0>
> >> + true,"boolean",1,"integer",1.1,"double"
> >> + })
> >> +
> >> +test:do_execsql_test(
> >> + "scalar-bug-2.0",
> >> + [[
> >> + SELECT s3, typeof(s3) FROM test ORDER BY s3;
> >> + ]], {
> >> + -- <scalar-bug-2.0>
> >> + 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 <roman.habibov@tarantool.org>
> >> 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;
> >> + ]], {
> >> + -- <scalar-bug-1.0>
> >> + true,"boolean",1,"integer",1.1,"double"
> >> + })
> >> +
> >> +test:do_execsql_test(
> >> + "scalar-bug-2.0",
> >> + [[
> >> + SELECT s3, typeof(s3) FROM test ORDER BY s3;
> >> + ]], {
> >> + -- <scalar-bug-2.0>
> >> + 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 <roman.habibov@tarantool.org>
> 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;
> + ]], {
> + -- <scalar-bug-1.0>
> + true,"boolean",1,"integer",1.1,"double"
> + })
> +
> +test:do_execsql_test(
> + "scalar-bug-2.0",
> + [[
> + SELECT s3, typeof(s3) FROM test ORDER BY s3;
> + ]], {
> + -- <scalar-bug-2.0>
> + true,"boolean",1,"integer",1.1,"double"
> + })
> +
> +test:finish_test()
>
next prev parent reply other threads:[~2020-04-17 6:35 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-13 4:11 Roman Khabibov
2020-04-15 19:41 ` Mergen Imeev
2020-04-16 0:34 ` Roman Khabibov
2020-04-16 8:17 ` Mergen Imeev
2020-04-17 0:48 ` Roman Khabibov
2020-04-17 6:35 ` Mergen Imeev [this message]
2020-04-17 7:27 ` Timur Safin
2020-04-17 11:25 ` Roman Khabibov
2020-04-17 11:51 ` Timur Safin
2020-04-18 2:46 ` Roman Khabibov
2020-04-18 9:18 ` Timur Safin
2020-04-20 0:12 ` Roman Khabibov
2020-04-20 1:05 ` Nikita Pettik
2020-04-20 20:28 ` Roman Khabibov
2020-04-20 23:57 ` Nikita Pettik
2020-04-24 13:03 ` Roman Khabibov
2020-04-27 1:34 ` Nikita Pettik
2020-04-27 15:31 ` Nikita Pettik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200417063512.GA10928@tarantool.org \
--to=imeevma@tarantool.org \
--cc=roman.habibov@tarantool.org \
--cc=tarantool-patches@dev.tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH] sql: fix number and boolean sorting rules' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox