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

["Timur Safin" <tsafin@tarantool.org>]

If you would be asking me then LGTM now

Timur

: -----Original Message-----
: From: Roman Khabibov <roman.habibov@tarantool.org>
: Sent: Saturday, April 18, 2020 5:46 AM
: To: Timur Safin <tsafin@tarantool.org>
: Cc: Mergen Imeev <imeevma@tarantool.org>; tarantool-
: patches@dev.tarantool.org
: Subject: Re: [PATCH] sql: fix number and boolean sorting rules
: 
: 
: > On Apr 17, 2020, at 14:51, Timur Safin <tsafin@tarantool.org> wrote:
: >
: >
: >
: > : -----Original Message-----
: > : From: Roman Khabibov <roman.habibov@tarantool.org>
: > : Sent: Friday, April 17, 2020 2:25 PM
: > : To: Timur Safin <tsafin@tarantool.org>
: > : Cc: Mergen Imeev <imeevma@tarantool.org>; tarantool-
: > : patches@dev.tarantool.org
: > : Subject: Re: [PATCH] sql: fix number and boolean sorting rules
: > :
: > : Hi! Thanks for the review.
: > :
: > : > On Apr 17, 2020, at 10:27, Timur Safin <tsafin@tarantool.org> wrote:
: > : >
: > : > As a random bypasser I could not resist and not add my 5 kopecks
: (see
: > : below).
: > : >
: >
: >
: > : >
: > : Fixed.
: > :
: > : commit 386ba8676c0ebd381d9c03b2ebf80abd986de73b (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/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..6f600f27b
: > : --- /dev/null
: > : +++ b/test/sql-tap/gh-4697-scalar-bug.test.lua
: > : @@ -0,0 +1,35 @@
: > : +#!/usr/bin/env tarantool
: > : +test = require("sqltester")
: > : +test:plan(2)
: > : +
: > : +--
: > : +-- gh-4679: Make sure that boolean follow before any number within
: > : +-- scalar. Result with order by indexed (using index) and
: > : +-- non-indexed (using no index) must be the same.
: > : +--
: >
: > Not everywhere (see the same problem above ^ in the test code)
: >
: > Timur
: >
: > P.S.
: > That was the purpose of 'g' modifier in my sed expression :)
: > To replace all occurrences, not only the first one.
: Didn’t understand that, sorry. Now it is done.
: 
: commit 48530ef30fbbb3c7983e75385adbc43448fb9732 (HEAD -> 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..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;
:  			}
:  			break;
:  		}
: @@ -3072,7 +3073,8 @@ sqlVdbeCompareMsgpack(const char **key1,
:  				rc = double_compare_uint64(-pKey2->u.r,
:  							   -mem1.u.i, 1);
:  			} else {
: -				rc = (pKey2->flags & MEM_Null) ? +1 : -1;
: +				rc = (pKey2->flags & MEM_Null) != 0 ||
: +				     (pKey2->flags & MEM_Bool) != 0 ? 1 : -1;
:  			}
:  			break;
:  		}
: @@ -3096,7 +3098,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
: @@ -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);
: +]]
: +
: +test:do_execsql_test(
: +    "scalar-bug-1.0",
: +    [[
: +        SELECT s2, typeof(s2) FROM test ORDER BY s2;
: +    ]], {
: +        -- <scalar-bug-1.0>
: +        true,"boolean",1,"integer",1.1,"double"
: +    })
: +
: +test:do_execsql_test(
: +    "scalar-bug-2.0",
: +    [[
: +        SELECT s3, typeof(s3) FROM test ORDER BY s3;
: +    ]], {
: +        -- <scalar-bug-2.0>
: +        true,"boolean",1,"integer",1.1,"double"
: +    })
: +
: +test:finish_test()