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

Roman Khabibov roman.habibov at tarantool.org
Fri Apr 24 16:03:23 MSK 2020


Hi! Thanks for the review.

> On Apr 21, 2020, at 02:57, Nikita Pettik <korablev at tarantool.org> wrote:
> 
> On 20 Apr 23:28, Roman Khabibov wrote:
>> 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;
> 
> Nit: please, split this into two branches. This makes code flow
> more clear. It is not so easy to parse ternary operator with
> complex condition.
@@ -3053,8 +3053,11 @@ sqlVdbeCompareMsgpack(const char **key1,
 			} else if ((pKey2->flags & MEM_Real) != 0) {
 				rc = double_compare_uint64(pKey2->u.r,
 							   mem1.u.u, -1);
+			} else if ((pKey2->flags & MEM_Null) != 0 ||
+				     (pKey2->flags & MEM_Bool) != 0) {
+				rc = 1;
 			} else {
-				rc = (pKey2->flags & MEM_Null) ? +1 : -1;
+				rc = -1;
 			}
 			break;
 		}
@@ -3071,8 +3074,11 @@ sqlVdbeCompareMsgpack(const char **key1,
 			} else if (pKey2->flags & MEM_Real) {
 				rc = double_compare_uint64(-pKey2->u.r,
 							   -mem1.u.i, 1);
+			} else if ((pKey2->flags & MEM_Null) != 0 ||
+				     (pKey2->flags & MEM_Bool) != 0) {
+				rc = 1;
 			} else {
-				rc = (pKey2->flags & MEM_Null) ? +1 : -1;
+				rc = -1;
 			}
 			break;
 		}
@@ -3096,7 +3102,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
> 
> Please, use more specific test name. Like: gh-4697-scalar-bool-sort-cmp.test.lua
> Also, now we can write tests using only SQL format. Let's strive to use
> it whenever it is possible.
> 
>> @@ -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);
> 
> Check also NULLs as values (since NULLs appear in the same code path).
diff --git a/test/sql/gh-4697-scalar-bool-sort-cmp.test.sql b/test/sql/gh-4697-scalar-bool-sort-cmp.test.sql
new file mode 100755
index 000000000..faca6819d
--- /dev/null
+++ b/test/sql/gh-4697-scalar-bool-sort-cmp.test.sql
@@ -0,0 +1,13 @@
+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);
+INSERT INTO test VALUES (3, NULL, NULL);
+
+--
+-- 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.
+--
+SELECT s2, typeof(s2) FROM test ORDER BY s2;
+SELECT s3, typeof(s3) FROM test ORDER BY s3;


commit 2dad4688dac14faded9547d4b24065b8d2fcf1a9 (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..310cec3ed 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -3053,8 +3053,11 @@ sqlVdbeCompareMsgpack(const char **key1,
 			} else if ((pKey2->flags & MEM_Real) != 0) {
 				rc = double_compare_uint64(pKey2->u.r,
 							   mem1.u.u, -1);
+			} else if ((pKey2->flags & MEM_Null) != 0 ||
+				     (pKey2->flags & MEM_Bool) != 0) {
+				rc = 1;
 			} else {
-				rc = (pKey2->flags & MEM_Null) ? +1 : -1;
+				rc = -1;
 			}
 			break;
 		}
@@ -3071,8 +3074,11 @@ sqlVdbeCompareMsgpack(const char **key1,
 			} else if (pKey2->flags & MEM_Real) {
 				rc = double_compare_uint64(-pKey2->u.r,
 							   -mem1.u.i, 1);
+			} else if ((pKey2->flags & MEM_Null) != 0 ||
+				     (pKey2->flags & MEM_Bool) != 0) {
+				rc = 1;
 			} else {
-				rc = (pKey2->flags & MEM_Null) ? +1 : -1;
+				rc = -1;
 			}
 			break;
 		}
@@ -3096,7 +3102,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/gh-4697-scalar-bool-sort-cmp.result b/test/sql/gh-4697-scalar-bool-sort-cmp.result
new file mode 100644
index 000000000..2423cd894
--- /dev/null
+++ b/test/sql/gh-4697-scalar-bool-sort-cmp.result
@@ -0,0 +1,53 @@
+-- test-run result file version 2
+CREATE TABLE test (s1 INTEGER PRIMARY KEY, s2 SCALAR UNIQUE, s3 SCALAR);
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO test VALUES (0, 1, 1);
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO test VALUES (1, 1.1, 1.1);
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO test VALUES (2, true, true);
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO test VALUES (3, NULL, NULL);
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- 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.
+--
+SELECT s2, typeof(s2) FROM test ORDER BY s2;
+ | ---
+ | - metadata:
+ |   - name: S2
+ |     type: scalar
+ |   - name: typeof(s2)
+ |     type: string
+ |   rows:
+ |   - [null, 'boolean']
+ |   - [true, 'boolean']
+ |   - [1, 'integer']
+ |   - [1.1, 'double']
+ | ...
+SELECT s3, typeof(s3) FROM test ORDER BY s3;
+ | ---
+ | - metadata:
+ |   - name: S3
+ |     type: scalar
+ |   - name: typeof(s3)
+ |     type: string
+ |   rows:
+ |   - [null, 'boolean']
+ |   - [true, 'boolean']
+ |   - [1, 'integer']
+ |   - [1.1, 'double']
+ | ...
diff --git a/test/sql/gh-4697-scalar-bool-sort-cmp.test.sql b/test/sql/gh-4697-scalar-bool-sort-cmp.test.sql
new file mode 100755
index 000000000..faca6819d
--- /dev/null
+++ b/test/sql/gh-4697-scalar-bool-sort-cmp.test.sql
@@ -0,0 +1,13 @@
+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);
+INSERT INTO test VALUES (3, NULL, NULL);
+
+--
+-- 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.
+--
+SELECT s2, typeof(s2) FROM test ORDER BY s2;
+SELECT s3, typeof(s3) FROM test ORDER BY s3;



More information about the Tarantool-patches mailing list