From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id DD3D04696C3 for ; Fri, 24 Apr 2020 16:03:25 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3594.4.19\)) From: Roman Khabibov In-Reply-To: <20200420235706.GA28948@tarantool.org> Date: Fri, 24 Apr 2020 16:03:23 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <6CC4ABB2-D49D-4721-B913-B999D70844FB@tarantool.org> <20200417063512.GA10928@tarantool.org> <1c1301d61489$a1af1790$e50d46b0$@tarantool.org> <8A7E5353-3471-4B54-91F7-F35AB4A4B039@tarantool.org> <1e3201d614ae$8cc16d40$a64447c0$@tarantool.org> <2D88A96E-5B9A-418C-8DD1-BB9BFD1EDD57@tarantool.org> <203801d61562$5ce44390$16accab0$@tarantool.org> <610CF1D6-A724-4BE2-A717-C3F44A733D7A@tarantool.org> <20200420010518.GA25021@tarantool.org> <1207B8C8-659E-44EB-80F1-9ABBF6B65FA1@tarantool.org> <20200420235706.GA28948@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] sql: fix number and boolean sorting rules List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. > On Apr 21, 2020, at 02:57, Nikita Pettik = wrote: >=20 > 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 =3D double_compare_uint64(pKey2->u.r, >> mem1.u.u, = -1); >> } else { >> - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; >> + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || >> + (pKey2->flags & MEM_Bool) !=3D 0 ? = 1 : -1; >=20 > 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) !=3D 0) { rc =3D double_compare_uint64(pKey2->u.r, mem1.u.u, = -1); + } else if ((pKey2->flags & MEM_Null) !=3D 0 || + (pKey2->flags & MEM_Bool) !=3D 0) { + rc =3D 1; } else { - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; + rc =3D -1; } break; } @@ -3071,8 +3074,11 @@ sqlVdbeCompareMsgpack(const char **key1, } else if (pKey2->flags & MEM_Real) { rc =3D = double_compare_uint64(-pKey2->u.r, -mem1.u.i, = 1); + } else if ((pKey2->flags & MEM_Null) !=3D 0 || + (pKey2->flags & MEM_Bool) !=3D 0) { + rc =3D 1; } else { - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; + rc =3D -1; } break; } @@ -3096,7 +3102,8 @@ sqlVdbeCompareMsgpack(const char **key1, rc =3D +1; } } else { - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || + (pKey2->flags & MEM_Bool) !=3D 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 >=20 > 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. >=20 >> @@ -0,0 +1,35 @@ >> +#!/usr/bin/env tarantool >> +test =3D 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); >=20 > 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 Date: Mon Apr 13 05:03:54 2020 +0300 sql: fix sorting rules for scalar =20 Sort values =E2=80=8Bin the correct order for scalar type when using = the vdbe sorter. Boolean always precedes number if it is sorted in ascending order. =20 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) !=3D 0) { rc =3D double_compare_uint64(pKey2->u.r, mem1.u.u, = -1); + } else if ((pKey2->flags & MEM_Null) !=3D 0 || + (pKey2->flags & MEM_Bool) !=3D 0) { + rc =3D 1; } else { - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; + rc =3D -1; } break; } @@ -3071,8 +3074,11 @@ sqlVdbeCompareMsgpack(const char **key1, } else if (pKey2->flags & MEM_Real) { rc =3D = double_compare_uint64(-pKey2->u.r, -mem1.u.i, = 1); + } else if ((pKey2->flags & MEM_Null) !=3D 0 || + (pKey2->flags & MEM_Bool) !=3D 0) { + rc =3D 1; } else { - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; + rc =3D -1; } break; } @@ -3096,7 +3102,8 @@ sqlVdbeCompareMsgpack(const char **key1, rc =3D +1; } } else { - rc =3D (pKey2->flags & MEM_Null) ? +1 : = -1; + rc =3D (pKey2->flags & MEM_Null) !=3D 0 = || + (pKey2->flags & MEM_Bool) !=3D 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;