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

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for fixes. See one comment below.

On Thu, Apr 16, 2020 at 03:34:29AM +0300, Roman Khabibov wrote:
> Hi! Thanks for the review.
> 
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4697-scalar-bug
> Issue: https://github.com/tarantool/tarantool/issues/4697
> 
> @ChangeLog
> - Fix the bug with wrong sorter order of boolean and integer
> within SCALAR.
> 
> > On Apr 15, 2020, at 22:41, Mergen Imeev <imeevma@tarantool.org> wrote:
> > 
> > Hi! Thank you for the patch! See 5 small comments below.
> > 
> > On Mon, Apr 13, 2020 at 07:11:05AM +0300, Roman Khabibov wrote:
> >> Make comparison rules in the sql sorter correct for number and
> >> boolean. Boolean should always be greater than any number.
> > 1. I think you should say that values compared here using
> > SCALAR rules. It makes no sence otherwise, since boolean
> > cannot be compared with number.
> Fixed.
> 
> > 2. Isn't boolean always less that any number?
> https://www.tarantool.io/en/doc/1.10/book/box/data_model/
> See the "Indexed field types" table.
1. Not sure about this table, I cannot see where is says that
boolean is greater than number. But, we can see the order if
we look at 'enum mp_class' in tuple_compare.cc. It says, that
nil < boolean < number < string < varbinary and so on.

Also, if we execute these commands:

box.execute('create table t(i scalar primary key);')
box.execute('insert into t values (true), (1), (1.5);')
box.execute('select * from t where i > true;')

we will receive this result:
---
- metadata:
  - name: I
    type: scalar
  rows:
  - [1]
  - [1.5]
...

It shows that any number is more that any boolean.

Actually, in you tests order in the results also show this.

> 
> >> 
> >> Closes #4697
> >> ---
> > 3. Please include links to issue and branch. Also,
> > ChangeLog is missing.
> >> src/box/sql/vdbeaux.c      |  9 ++++++---
> >> test/sql-tap/sort.test.lua | 18 +++++++++++++++++-
> >> 2 files changed, 23 insertions(+), 4 deletions(-)
> >> 
> >> 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/sort.test.lua b/test/sql-tap/sort.test.lua
> >> index 36074d6ef..15b397f1d 100755
> >> --- a/test/sql-tap/sort.test.lua
> >> +++ b/test/sql-tap/sort.test.lua
> >> @@ -1,6 +1,6 @@
> >> #!/usr/bin/env tarantool
> >> test = require("sqltester")
> >> -test:plan(60)
> >> +test:plan(61)
> >> 
> >> --!./tcltestrunner.lua
> >> -- 2001 September 15.
> >> @@ -923,4 +923,20 @@ box.internal.sql_create_function("cksum", cksum)
> >>         })
> >> 
> >> end
> >> +
> >> +-- gh-4679 Make sure that boolean > any number within scalar.
> >> +test:do_execsql_test(
> >> +    18.1,
> >> +    [[
> >> +        CREATE TABLE test (s1 INTEGER PRIMARY KEY, s2 SCALAR);
> >> +        INSERT INTO test VALUES (0, 1);
> >> +        INSERT INTO test VALUES (2, 1.0);
> >> +        INSERT INTO test VALUES (3, true);
> >> +        SELECT s2, typeof(s2) FROM test ORDER BY s2;
> >> +    ]], {
> >> +        -- <18.1>
> >> +        true,"boolean",1,"integer",1,"double"
> >> +        -- </18.1>
> >> +    })
> >> +
> > 4. I think it was decided that we should create new file
> > for each new test.
> > 
> > 5. I think, it is worth to add result of SELECT where index
> > was used. The example we can see in text of the issue.
> > Also, we should mention comewhere in the comments, that the
> > result should be the same in case index was used and in case
> > it wasn't.
> 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..159e91fca
> --- /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 > 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()
> 
> >> test:finish_test()
> >> -- 
> >> 2.21.0 (Apple Git-122)
> 
> commit 9ea60156e72dc45dbea66c9e614769b8d8677e6f (HEAD -> romanhabibov/gh-4697-scalar-bug)
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> Date:   Mon Apr 13 05:03:54 2020 +0300
> 
>     sql: fix number and boolean SCALAR sorting rules
>     
>     Make comparison SCALAR rules in the sql sorter correct for number
>     and boolean. Boolean should always be greater than any number.
>     
>     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..159e91fca
> --- /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 > 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()
> 
>