Re: [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thank you for the patch. See 18 comments below.

1. Title of commit-message is too long. Please try to fit it to 50 symbols.

On Tue, May 25, 2021 at 12:01:00PM +0300, Timur Safin wrote:
> We need to modify explicit casts table according to the RFC developed
> previously in /doc/rfc/5910-consistent-sql-lua-types.md. This patch
> introduces changes where BOOLEAN type involved, thus, for simplicity
> sake, we mark unchanged cells in the table below as '.'
> 
2. I believe is it better to use RFC name instead of this relative link.

3. Do you really need whole table? I believe it is overkill to have it here.
You already mentioned RFC and it is simpler to just have description of 
explicit
cast related to BOOLEAN.

> Since now on BOOLEAN will be only compatible with itself and STRINGs
> (and recursively with SCALAR, which includes both those types). We
> remove all other possible combinations which are defined now, these
> cells marked with '-'.
> 
>               | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
>               +---+---+---+---+---+---+---+---+---+---+---+---+---+
>  0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
>  1.  unsigned |   | . | . | . | . | - |   | . |   |   |   |   | . |
>  2.    string |   | . | . | . | . | S | . | . |   |   |   |   | . |
>  4.    double |   | . | . | . | . | - |   | . |   |   |   |   | . |
>  5.   integer |   | . | . | . | . | - |   | . |   |   |   |   | . |
>  6.   boolean |   | - | Y | - | - | Y |   |   |   |   |   |   | . |
>  7. varbinary |   |   | . |   |   | - | . |   |   |   |   |   | . |
>  3.    number |   | . | . | . | . | - |   | . |   |   |   |   | . |
>  9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
>  8.    scalar |   | . | . | . | . | S | . | . |   |   |   |   | . |
>               +---+---+---+---+---+---+---+---+---+---+---+---+---+
> 
4. Again this strange order in table.

> Current patch fixes BOOLEAN to other types entries, simultaneously
> updating corresponding tests.
> 
> Full check for current conversion table will be committed later as
> separate comit.
> 
5. Do you need to mention this is commit-message for BOOLEAN?

> Relates to #5910, #6009
> Part of #4407
6. Wrong issue number. Also, not sure that it is right to include
"Relates to". You already have link to issue and RFC.


> ---
>  src/box/sql/mem.c               | 37 --------------------------------
>  test/sql-tap/cse.test.lua       | 12 +++++------
>  test/sql-tap/e_select1.test.lua |  2 +-
>  test/sql-tap/in1.test.lua       | 16 +++++++-------
>  test/sql-tap/misc3.test.lua     |  2 +-
>  test/sql/boolean.result         | 38 +++++++--------------------------
>  test/sql/types.result           | 14 ++++--------
>  7 files changed, 28 insertions(+), 93 deletions(-)
> 
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index b6ff6397f..d40366754 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -526,15 +526,6 @@ int_to_str0(struct Mem *mem)
>  	return mem_copy_str0(mem, str);
>  }
>  
> -static inline int
> -int_to_bool(struct Mem *mem)
> -{
> -	mem->u.b = mem->u.i != 0;
> -	mem->flags = MEM_Bool;
> -	mem->field_type = FIELD_TYPE_BOOLEAN;
> -	return 0;
> -}
> -
>  static inline int
>  str_to_str0(struct Mem *mem)
>  {
> @@ -717,24 +708,6 @@ double_to_str0(struct Mem *mem)
>  	return 0;
>  }
>  
> -static inline int
> -double_to_bool(struct Mem *mem)
> -{
> -	mem->u.b = mem->u.r != 0.;
> -	mem->flags = MEM_Bool;
> -	mem->field_type = FIELD_TYPE_BOOLEAN;
> -	return 0;
> -}
> -
> -static inline int
> -bool_to_int(struct Mem *mem)
> -{
> -	mem->u.u = (uint64_t)mem->u.b;
> -	mem->flags = MEM_UInt;
> -	mem->field_type = FIELD_TYPE_UNSIGNED;
> -	return 0;
> -}
> -
>  static inline int
>  bool_to_str0(struct Mem *mem)
>  {
> @@ -766,8 +739,6 @@ mem_to_int(struct Mem *mem)
>  		return bytes_to_int(mem);
>  	if ((mem->flags & MEM_Real) != 0)
>  		return double_to_int(mem);
> -	if ((mem->flags & MEM_Bool) != 0)
> -		return bool_to_int(mem);
>  	return -1;
>  }
>  
> @@ -803,8 +774,6 @@ mem_to_number(struct Mem *mem)
>  	assert((mem->flags & MEM_PURE_TYPE_MASK) != 0);
>  	if ((mem->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0)
>  		return 0;
> -	if ((mem->flags & MEM_Bool) != 0)
> -		return bool_to_int(mem);
>  	if ((mem->flags & (MEM_Str | MEM_Blob)) != 0) {
>  		if (bytes_to_int(mem) == 0)
>  			return 0;
> @@ -879,8 +848,6 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
>  			return bytes_to_uint(mem);
>  		if ((mem->flags & MEM_Real) != 0)
>  			return double_to_int(mem);
> -		if ((mem->flags & MEM_Bool) != 0)
> -			return bool_to_int(mem);
>  		return -1;
>  	case FIELD_TYPE_STRING:
>  		return mem_to_str(mem);
> @@ -891,12 +858,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
>  	case FIELD_TYPE_BOOLEAN:
>  		if ((mem->flags & MEM_Bool) != 0)
>  			return 0;
> -		if ((mem->flags & (MEM_UInt | MEM_Int)) != 0)
> -			return int_to_bool(mem);
>  		if ((mem->flags & MEM_Str) != 0)
>  			return str_to_bool(mem);
> -		if ((mem->flags & MEM_Real) != 0)
> -			return double_to_bool(mem);
>  		return -1;
>  	case FIELD_TYPE_VARBINARY:
>  		if ((mem->flags & MEM_Blob) != 0)
> diff --git a/test/sql-tap/cse.test.lua b/test/sql-tap/cse.test.lua
> index e09f955a0..84ff936e6 100755
> --- a/test/sql-tap/cse.test.lua
> +++ b/test/sql-tap/cse.test.lua
> @@ -31,7 +31,7 @@ test:do_test(
>              INSERT INTO t1 VALUES(2,21,22,23,24,25);
>          ]]
>          return test:execsql [[
> -            SELECT b, -b, ~b, NOT CAST(b AS BOOLEAN), NOT NOT CAST(b AS BOOLEAN), b-b, b+b, b*b, b/b, b FROM t1
> +            SELECT b, -b, ~b, NOT (b <> 0), NOT NOT (b <> 0), b-b, b+b, b*b, b/b, b FROM t1
7. Since you already change this line, add spaces before and after all
operations with two operands.

>          ]]
>      end, {
>          -- <cse-1.1>
> @@ -102,7 +102,7 @@ test:do_execsql_test(
>  test:do_execsql_test(
>      "cse-1.6.3",
>      [[
> -        SELECT CASE WHEN CAST(b AS BOOLEAN) THEN d WHEN CAST(e AS BOOLEAN)  THEN f ELSE 999 END, b, c, d FROM t1
> +        SELECT CASE WHEN b<>0 THEN d WHEN e<>0 THEN f ELSE 999 END, b, c, d FROM t1
8. Please add spaces before and after '<>'.

>      ]], {
>          -- <cse-1.6.3>
>          13, 11, 12, 13, 23, 21, 22, 23
> @@ -112,7 +112,7 @@ test:do_execsql_test(
>  test:do_execsql_test(
>      "cse-1.6.4",
>      [[
> -        SELECT b, c, d, CASE WHEN CAST(b AS BOOLEAN) THEN d WHEN CAST(e AS BOOLEAN) THEN f ELSE 999 END FROM t1
> +        SELECT b, c, d, CASE WHEN b<>0 THEN d WHEN e<>0 THEN f ELSE 999 END FROM t1
9. Same.

>      ]], {
>          -- <cse-1.6.4>
>          11, 12, 13, 13, 21, 22, 23, 23
> @@ -122,7 +122,7 @@ test:do_execsql_test(
>  test:do_execsql_test(
>      "cse-1.6.5",
>      [[
> -        SELECT b, c, d, CASE WHEN false THEN d WHEN CAST(e AS BOOLEAN) THEN f ELSE 999 END FROM t1
> +        SELECT b, c, d, CASE WHEN false THEN d WHEN e<>0 THEN f ELSE 999 END FROM t1
10. Same.

>      ]], {
>          -- <cse-1.6.5>
>          11, 12, 13, 15, 21, 22, 23, 25
> @@ -132,7 +132,7 @@ test:do_execsql_test(
>  test:do_execsql_test(
>      "cse-1.7",
>      [[
> -        SELECT a, -a, ~a, NOT CAST(a AS BOOLEAN), NOT NOT CAST(a AS BOOLEAN), a-a, a+a, a*a, a/a, a FROM t1
> +        SELECT a, -a, ~a, NOT (a <> 0), NOT NOT (a <> 0), a-a, a+a, a*a, a/a, a FROM t1
11. Same as in 7.

>      ]], {
>          -- <cse-1.7>
>          1, -1 ,-2, false, true, 0, 2, 1, 1, 1, 2, -2, -3, false, true, 0, 4, 4, 1, 2
> @@ -152,7 +152,7 @@ test:do_execsql_test(
>  test:do_execsql_test(
>      "cse-1.9",
>      [[
> -        SELECT NOT CAST(b AS BOOLEAN), ~b, NOT NOT CAST(b AS BOOLEAN), b FROM t1
> +        SELECT NOT (b <> 0), ~b, NOT NOT (b <>0), b FROM t1
12. Please add space after '<>'.

>      ]], {
>          -- <cse-1.9>
>          false, -12, true, 11, false, -22, true, 21
> diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
> index ab0faa376..28ea1d82f 100755
> --- a/test/sql-tap/e_select1.test.lua
> +++ b/test/sql-tap/e_select1.test.lua
> @@ -910,7 +910,7 @@ test:do_select_tests(
>  
>          {"3", "SELECT sum(b+1) FROM z1 NATURAL LEFT JOIN z3", {-43.06}},
>          {"4", "SELECT sum(b+2) FROM z1 NATURAL LEFT JOIN z3", {-38.06}},
> -        {"5", "SELECT sum(CAST(b IS NOT NULL AS INTEGER)) FROM z1 NATURAL LEFT JOIN z3", {5}},
> +        {"5", "SELECT sum(CASE WHEN b IS NOT NULL THEN 1 ELSE 0 END) FROM z1 NATURAL LEFT JOIN z3", {5}},
>      })
>  
>  -- EVIDENCE-OF: R-26684-40576 Each non-aggregate expression in the
> diff --git a/test/sql-tap/in1.test.lua b/test/sql-tap/in1.test.lua
> index 4b51da6e8..0fb059760 100755
> --- a/test/sql-tap/in1.test.lua
> +++ b/test/sql-tap/in1.test.lua
> @@ -97,13 +97,13 @@ test:do_execsql_test(
>          -- </in-1.6>
>      })
>  
> -test:do_execsql_test(
> +test:do_catchsql_test(
13. Do you really need to change type of the test? I believe you can do the
same you did in the previous test.

>      "in-1.7",
>      [[
>          SELECT a+ 100*CAST((a BETWEEN 1 and 3) AS INTEGER) FROM t1 ORDER BY b
>      ]], {
>          -- <in-1.7>
> -        101, 102, 103, 4, 5, 6, 7, 8, 9, 10
> +        1, "Type mismatch: can not convert TRUE to integer"
>          -- </in-1.7>
>      })
>  
> @@ -154,13 +154,13 @@ test:do_execsql_test(
>          -- </in-2.4>
>      })
>  
> -test:do_execsql_test(
> +test:do_catchsql_test(
14. Same.

>      "in-2.5",
>      [[
>          SELECT a+100*(CAST(b IN (8,16,24) AS INTEGER)) FROM t1 ORDER BY b
>      ]], {
>          -- <in-2.5>
> -        1, 2, 103, 104, 5, 6, 7, 8, 9, 10
> +        1, "Type mismatch: can not convert FALSE to integer"
>          -- </in-2.5>
>      })
>  
> @@ -204,13 +204,13 @@ test:do_execsql_test(
>          -- </in-2.9>
>      })
>  
> -test:do_execsql_test(
> +test:do_catchsql_test(
15. Same.

>      "in-2.10",
>      [[
>          SELECT a FROM t1 WHERE LEAST(0, CAST(b IN (a,30) AS INT)) <> 0
>      ]], {
>          -- <in-2.10>
> -
> +        1, "Type mismatch: can not convert FALSE to integer"
>          -- </in-2.10>
>      })
>  
> @@ -250,13 +250,13 @@ test:do_execsql_test(
>          -- </in-3.2>
>      })
>  
> -test:do_execsql_test(
> +test:do_catchsql_test(
16. Same.

>      "in-3.3",
>      [[
>          SELECT a + 100*(CAST(b IN (SELECT b FROM t1 WHERE a<5) AS INTEGER)) FROM t1 ORDER BY b
>      ]], {
>          -- <in-3.3>
> -        101, 102, 103, 104, 5, 6, 7, 8, 9, 10
> +        1, "Type mismatch: can not convert TRUE to integer"
>          -- </in-3.3>
>      })
>  
> diff --git a/test/sql-tap/misc3.test.lua b/test/sql-tap/misc3.test.lua
> index 313484b5d..c2dc67355 100755
> --- a/test/sql-tap/misc3.test.lua
> +++ b/test/sql-tap/misc3.test.lua
> @@ -510,7 +510,7 @@ test:do_execsql_test(
>  test:do_execsql_test(
>      "misc-8.2",
>      [[
> -        SELECT count(*) FROM t3 WHERE 1+CAST((b IN ('abc','xyz')) AS INTEGER)==2
> +        SELECT count(*) FROM t3 WHERE b IN ('abc','xyz')
17. Somehow this change looks a bit too much. What checks this test?

>      ]], {
>          -- <misc-8.2>
>          2
> diff --git a/test/sql/boolean.result b/test/sql/boolean.result
> index 177a39fb9..b268eb2fe 100644
> --- a/test/sql/boolean.result
> +++ b/test/sql/boolean.result
> @@ -502,23 +502,13 @@ INSERT INTO t3 VALUES (4, false)
>  -- Check CAST from BOOLEAN to the other types.
>  SELECT cast(true AS INTEGER), cast(false AS INTEGER);
>   | ---
> - | - metadata:
> - |   - name: COLUMN_1
> - |     type: integer
> - |   - name: COLUMN_2
> - |     type: integer
> - |   rows:
> - |   - [1, 0]
> + | - null
> + | - 'Type mismatch: can not convert TRUE to integer'
>   | ...
>  SELECT cast(true AS NUMBER), cast(false AS NUMBER);
>   | ---
> - | - metadata:
> - |   - name: COLUMN_1
> - |     type: number
> - |   - name: COLUMN_2
> - |     type: number
> - |   rows:
> - |   - [1, 0]
> + | - null
> + | - 'Type mismatch: can not convert TRUE to number'
>   | ...
>  -- gh-4462: ensure that text representation is uppercase.
>  SELECT cast(true AS TEXT), cast(false AS TEXT);
> @@ -545,25 +535,13 @@ SELECT cast(true AS BOOLEAN), cast(false AS BOOLEAN);
>  -- Check CAST to BOOLEAN from the other types.
>  SELECT cast(100 AS BOOLEAN), cast(1 AS BOOLEAN), cast(0 AS BOOLEAN);
>   | ---
> - | - metadata:
> - |   - name: COLUMN_1
> - |     type: boolean
> - |   - name: COLUMN_2
> - |     type: boolean
> - |   - name: COLUMN_3
> - |     type: boolean
> - |   rows:
> - |   - [true, true, false]
> + | - null
> + | - 'Type mismatch: can not convert 100 to boolean'
>   | ...
>  SELECT cast(0.123 AS BOOLEAN), cast(0.0 AS BOOLEAN);
>   | ---
> - | - metadata:
> - |   - name: COLUMN_1
> - |     type: boolean
> - |   - name: COLUMN_2
> - |     type: boolean
> - |   rows:
> - |   - [true, false]
> + | - null
> + | - 'Type mismatch: can not convert 0.123 to boolean'
>   | ...
>  SELECT cast('true' AS BOOLEAN), cast('false' AS BOOLEAN);
>   | ---
> diff --git a/test/sql/types.result b/test/sql/types.result
> index 687ca3b15..d59cbef7d 100644
> --- a/test/sql/types.result
> +++ b/test/sql/types.result
> @@ -1035,11 +1035,8 @@ box.execute("SELECT CAST(18446744073709551615 AS SCALAR);")
>  ...
>  box.execute("SELECT CAST(18446744073709551615 AS BOOLEAN);")
>  ---
> -- metadata:
> -  - name: COLUMN_1
> -    type: boolean
> -  rows:
> -  - [true]
> +- null
> +- 'Type mismatch: can not convert 18446744073709551615 to boolean'
>  ...
>  box.execute("SELECT CAST('18446744073709551615' AS INTEGER);")
>  ---
> @@ -1105,11 +1102,8 @@ box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
>  ...
>  box.execute("SELECT CAST(true AS UNSIGNED);")
>  ---
> -- metadata:
> -  - name: COLUMN_1
> -    type: unsigned
> -  rows:
> -  - [1]
> +- null
> +- 'Type mismatch: can not convert TRUE to unsigned'
>  ...
>  box.execute("SELECT CAST('123' AS UNSIGNED);")
>  ---
> -- 
> 2.29.2
> 
18. Please add tests for this patch.