[Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables
Mergen Imeev
imeevma at tarantool.org
Tue Jun 1 17:02:38 MSK 2021
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.
More information about the Tarantool-patches
mailing list