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

[Nikita Pettik <korablev@tarantool.org>]

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.

> 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).