Re: [Tarantool-patches] [PATCH] sql: fix number and boolean sorting rules

[Roman Khabibov <roman.habibov@tarantool.org>]

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()