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

[Roman Khabibov]

Hi! Yes.

> On Apr 20, 2020, at 04:05, Nikita Pettik <korablev at tarantool.org> wrote:
> 
> On 20 Apr 03:12, Roman Khabibov wrote:
>> Hi! Kirill, two LGTMs.
> 
> Don't hurry, I'd like to look at patch as well. Could you please
> re-send patch after all fixes?
> 
>>> On Apr 18, 2020, at 12:18, Timur Safin <tsafin at tarantool.org> wrote:
>>> 
>>> If you would be asking me then LGTM now
>>> 
>>> Timur
>>> 
>>> : -----Original Message-----
>>> : From: Roman Khabibov <roman.habibov at tarantool.org>
>>> : Sent: Saturday, April 18, 2020 5:46 AM
>>> : To: Timur Safin <tsafin at tarantool.org>
>>> : Cc: Mergen Imeev <imeevma at tarantool.org>; tarantool-
>>> : patches at dev.tarantool.org
>>> : Subject: Re: [PATCH] sql: fix number and boolean sorting rules
>>> : 
>>> : 
>>> : > On Apr 17, 2020, at 14:51, Timur Safin <tsafin at tarantool.org> wrote:
>>> : >
>>> : >
>>> : >
>>> : > : -----Original Message-----
>>> : > : From: Roman Khabibov <roman.habibov at tarantool.org>
>>> : > : Sent: Friday, April 17, 2020 2:25 PM
>>> : > : To: Timur Safin <tsafin at tarantool.org>
>>> : > : Cc: Mergen Imeev <imeevma at tarantool.org>; tarantool-
>>> : > : patches at dev.tarantool.org
>>> : > : Subject: Re: [PATCH] sql: fix number and boolean sorting rules
>>> : > :
>>> : > : Hi! Thanks for the review.
>>> : > :
>>> : > : > On Apr 17, 2020, at 10:27, Timur Safin <tsafin at tarantool.org> wrote:
>>> : > : >
>>> : > : > As a random bypasser I could not resist and not add my 5 kopecks
>>> : (see
>>> : > : below).
>>> : > : >
>>> : >
>>> : >
>>> : > : >
>>> : > : Fixed.
>>> : > :
>>> : > : commit 386ba8676c0ebd381d9c03b2ebf80abd986de73b (HEAD ->
>>> : romanhabibov/gh-
>>> : > : 4697-scalar-bug, origin/romanhabibov/gh-4697-scalar-bug)
>>> : > : Author: Roman Khabibov <roman.habibov at tarantool.org>
>>> : > : Date:   Mon Apr 13 05:03:54 2020 +0300
>>> : > :
>>> : > :     sql: fix sorting rules for scalar
>>> : > :
>>> : > :     Sort values​in the correct order for scalar type when using the
>>> : > :     vdbe sorter. Boolean always precedes number if it is sorted in
>>> : > :     ascending order.
>>> : > :
>>> : > :     Closes #4697
>>> : > :
>>> : > : 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.
>>> : > : +--
>>> : >
>>> : > Not everywhere (see the same problem above ^ in the test code)
>>> : >
>>> : > Timur
>>> : >
>>> : > P.S.
>>> : > That was the purpose of 'g' modifier in my sed expression :)
>>> : > To replace all occurrences, not only the first one.
>>> : Didn’t understand that, sorry. Now it is done.
>>> : 
>>> : commit 48530ef30fbbb3c7983e75385adbc43448fb9732 (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 sorting rules for scalar
>>> : 
>>> :     Sort values ​in the correct order for scalar type when using the
>>> :     vdbe sorter. Boolean always precedes number if it is sorted in
>>> :     ascending order.
>>> : 
>>> :     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..4ac286f77
>>> : --- /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 precedes 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 48530ef30fbbb3c7983e75385adbc43448fb9732 (HEAD -> romanhabibov/gh-4697-scalar-bug, origin/romanhabibov/gh-4697-scalar-bug)
Author: Roman Khabibov <roman.habibov at tarantool.org>
Date:   Mon Apr 13 05:03:54 2020 +0300

    sql: fix sorting rules for scalar
    
    Sort values ​in the correct order for scalar type when using the
    vdbe sorter. Boolean always precedes number if it is sorted in
    ascending order.
    
    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..4ac286f77
--- /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 precedes 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()