[Tarantool-patches] [PATCH] sql: fix number and boolean sorting rules
Roman Khabibov
roman.habibov at tarantool.org
Thu Apr 16 03:34:29 MSK 2020
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 at 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.
>>
>> 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 at 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()
More information about the Tarantool-patches
mailing list