From: Roman Khabibov <roman.habibov@tarantool.org>
To: Nikita Pettik <korablev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] sql: fix number and boolean sorting rules
Date: Fri, 24 Apr 2020 16:03:23 +0300 [thread overview]
Message-ID: <E86508CC-86A2-4BFD-823F-052EBA94AAE0@tarantool.org> (raw)
In-Reply-To: <20200420235706.GA28948@tarantool.org>
Hi! Thanks for the review.
> On Apr 21, 2020, at 02:57, Nikita Pettik <korablev@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@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;
next prev parent reply other threads:[~2020-04-24 13:03 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-13 4:11 Roman Khabibov
2020-04-15 19:41 ` Mergen Imeev
2020-04-16 0:34 ` Roman Khabibov
2020-04-16 8:17 ` Mergen Imeev
2020-04-17 0:48 ` Roman Khabibov
2020-04-17 6:35 ` Mergen Imeev
2020-04-17 7:27 ` Timur Safin
2020-04-17 11:25 ` Roman Khabibov
2020-04-17 11:51 ` Timur Safin
2020-04-18 2:46 ` Roman Khabibov
2020-04-18 9:18 ` Timur Safin
2020-04-20 0:12 ` Roman Khabibov
2020-04-20 1:05 ` Nikita Pettik
2020-04-20 20:28 ` Roman Khabibov
2020-04-20 23:57 ` Nikita Pettik
2020-04-24 13:03 ` Roman Khabibov [this message]
2020-04-27 1:34 ` Nikita Pettik
2020-04-27 15:31 ` Nikita Pettik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E86508CC-86A2-4BFD-823F-052EBA94AAE0@tarantool.org \
--to=roman.habibov@tarantool.org \
--cc=korablev@tarantool.org \
--cc=tarantool-patches@dev.tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH] sql: fix number and boolean sorting rules' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox