[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