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

[Mergen Imeev <imeevma@tarantool.org>]

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.

2. Isn't boolean always less that any number?

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

>  test:finish_test()
> -- 
> 2.21.0 (Apple Git-122)
>