Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables
@ 2021-05-25  9:00 Timur Safin via Tarantool-patches
  2021-05-25  9:01 ` [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables Timur Safin via Tarantool-patches
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-05-25  9:00 UTC (permalink / raw)
  To: tarantool-patches, imeevma


Recent RFC "Consistent Lua/SQL types" (#6009) defined ideal explicit
and implicit conversion tables we would like to have for all current 
and future Taranool SQL types. 

This patchset modifies explict conversion tables, and implicit 
conversion table to come soon. The ideal picture would be as below:

              | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
              +---+---+---+---+---+---+---+---+---+---+---+---+---+
 0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
 1.  unsigned |   | Y | Y | Y | Y |   |   | Y |   |   |   |   | Y |
 2.    string |   | S | Y | S | S | S | Y | S |   |   |   |   | Y |
 4.    double |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
 5.   integer |   | S | Y | Y | Y |   |   | Y |   |   |   |   | Y |
 6.   boolean |   |   | Y |   |   | Y |   |   |   |   |   |   | Y |
 7. varbinary |   |   | Y |   |   |   | Y |   |   |   |   |   | Y |
 3.    number |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
 9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
 8.    scalar |   | S | Y | S | S | S | S | S |   |   |   |   | Y |
              +---+---+---+---+---+---+---+---+---+---+---+---+---+

Please pay attention that we omit DECIMAL, UUID, SCALAR and MAP rows
and columns, as they not yet fully supported by Tarantool SQL. Once 
their support will be landed we will modify conversion table and 
tests (which we also introducing with current patchset).


Timur Safin (3):
  sql: fixes for boolean expressions in explicit converstion tables
  sql: enabled ANY as target for explicit conversions
  sql: introduced explicit casts test e_casts.test.lua

 extra/mkkeywordhash.c           |   3 +-
 src/box/sql/mem.c               |  39 +---
 src/box/sql/parse.y             |   3 +-
 test/sql-tap/cse.test.lua       |  12 +-
 test/sql-tap/e_casts.test.lua   | 355 ++++++++++++++++++++++++++++++++
 test/sql-tap/e_select1.test.lua |   2 +-
 test/sql-tap/in1.test.lua       |  16 +-
 test/sql-tap/keyword1.test.lua  |   2 +-
 test/sql-tap/misc3.test.lua     |   2 +-
 test/sql/boolean.result         |  38 +---
 test/sql/types.result           |  14 +-
 11 files changed, 390 insertions(+), 96 deletions(-)
 create mode 100755 test/sql-tap/e_casts.test.lua

-- 
2.29.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables
  2021-05-25  9:00 [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Timur Safin via Tarantool-patches
@ 2021-05-25  9:01 ` Timur Safin via Tarantool-patches
  2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
  2021-05-25  9:01 ` [Tarantool-patches] [PATCH 2/3] sql: enabled ANY as target for explicit conversions Timur Safin via Tarantool-patches
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-05-25  9:01 UTC (permalink / raw)
  To: tarantool-patches, imeevma

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

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

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.

Relates to #5910, #6009
Part of #4407
---
 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
         ]]
     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
     ]], {
         -- <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
     ]], {
         -- <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
     ]], {
         -- <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
     ]], {
         -- <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
     ]], {
         -- <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(
     "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(
     "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(
     "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(
     "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')
     ]], {
         -- <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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Tarantool-patches] [PATCH 2/3] sql: enabled ANY as target for explicit conversions
  2021-05-25  9:00 [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Timur Safin via Tarantool-patches
  2021-05-25  9:01 ` [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables Timur Safin via Tarantool-patches
@ 2021-05-25  9:01 ` Timur Safin via Tarantool-patches
  2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
  2021-05-25  9:01 ` [Tarantool-patches] [PATCH 3/3] sql: introduced explicit casts test e_casts.test.lua Timur Safin via Tarantool-patches
  2021-06-01 14:02 ` [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Mergen Imeev via Tarantool-patches
  3 siblings, 1 reply; 13+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-05-25  9:01 UTC (permalink / raw)
  To: tarantool-patches, imeevma

For consistency sake it was decided to provide
`CAST(anything TO ANY)` as allowed, but still noop.

Fuller explicit conversions table is described in the
/doc/rfc/5910-consistent-sql-lua-types.md and here we
represent only relevant to ANY.

Furthermore, there is no direct way in SQL to create literal and
expression of ANY type, thus there is no defined CAST from ANY
to anything else.

              | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
              +---+---+---+---+---+---+---+---+---+---+---+---+---+
 0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
 1.  unsigned |   | . | . | . | . |   |   | . |   |   |   |   | Y |
 2.    string |   | . | . | . | . | . | . | . |   |   |   |   | Y |
 4.    double |   | . | . | . | . |   |   | . |   |   |   |   | Y |
 5.   integer |   | . | . | . | . |   |   | . |   |   |   |   | Y |
 6.   boolean |   |   | . |   |   | . |   |   |   |   |   |   | Y |
 7. varbinary |   |   | . |   |   |   | . |   |   |   |   |   | Y |
 3.    number |   | . | . | . | . |   |   | . |   |   |   |   | Y |
 9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
 8.    scalar |   | . | . | . | . | . | . | . |   |   |   |   | Y |
              +---+---+---+---+---+---+---+---+---+---+---+---+---+

* We introduced new token ANY as known lexem to the Lemon parser.
  While renamed prior %wildcard to ANYTHING;
* Modified explicit conversion rules to allow ANY as _target_ in
  conversion pair.

Relates to #5910, #6009
Part of #4407
---
 extra/mkkeywordhash.c          | 3 ++-
 src/box/sql/mem.c              | 2 ++
 src/box/sql/parse.y            | 3 ++-
 test/sql-tap/keyword1.test.lua | 2 +-
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index 7480c0211..d343cf706 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -60,6 +60,7 @@ static Keyword aKeywordTable[] = {
   { "ALL",                    "TK_ALL",         true  },
   { "ALTER",                  "TK_ALTER",       true  },
   { "ANALYZE",                "TK_STANDARD",    true  },
+  { "ANY",                    "TK_ID",          true  },
   { "AND",                    "TK_AND",         true  },
   { "AS",                     "TK_AS",          true  },
   { "ASC",                    "TK_ASC",         true  },
@@ -178,7 +179,7 @@ static Keyword aKeywordTable[] = {
   { "WITH",                   "TK_WITH",        true  },
   { "WHEN",                   "TK_WHEN",        true  },
   { "WHERE",                  "TK_WHERE",       true  },
-  { "ANY",                    "TK_STANDARD",    true  },
+  { "ANYTHING",               "TK_STANDARD",    true  },
   { "ASENSITIVE",             "TK_STANDARD",    true  },
   { "BLOB",                   "TK_STANDARD",    true  },
   { "BINARY",                 "TK_ID",          true  },
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index d40366754..b4f1078ae 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -874,6 +874,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
 		    (mem->flags & MEM_Subtype) != 0)
 			return -1;
 		return 0;
+	case FIELD_TYPE_ANY:
+		return 0;
 	default:
 		break;
 	}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index abc363951..e586d0181 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -270,7 +270,7 @@ columnlist ::= tcons.
   QUERY KEY OFFSET RAISE RELEASE REPLACE RESTRICT
   RENAME CTIME_KW IF ENABLE DISABLE
   .
-%wildcard ANY.
+%wildcard ANYTHING.
 
 
 // And "ids" is an identifer-or-string.
@@ -1834,6 +1834,7 @@ typedef(A) ::= SCALAR . { A.type = FIELD_TYPE_SCALAR; }
 typedef(A) ::= BOOL . { A.type = FIELD_TYPE_BOOLEAN; }
 typedef(A) ::= BOOLEAN . { A.type = FIELD_TYPE_BOOLEAN; }
 typedef(A) ::= VARBINARY . { A.type = FIELD_TYPE_VARBINARY; }
+typedef(A) ::= ANY . { A.type = FIELD_TYPE_ANY; }
 
 /**
  * Time-like types are temporary disabled, until they are
diff --git a/test/sql-tap/keyword1.test.lua b/test/sql-tap/keyword1.test.lua
index f9a9c6865..4855f3ebc 100755
--- a/test/sql-tap/keyword1.test.lua
+++ b/test/sql-tap/keyword1.test.lua
@@ -242,7 +242,7 @@ for _, kw in ipairs(bannedkws) do
     local query = 'CREATE TABLE '..kw..'(a INT PRIMARY KEY);'
     if kw == 'end' or kw == 'match' or kw == 'release' or kw == 'rename' or
        kw == 'replace' or kw == 'binary' or kw == 'character' or
-       kw == 'smallint' then
+       kw == 'smallint' or kw == 'any' then
         test:do_catchsql_test(
         "bannedkw1-"..kw..".1",
         query, {
-- 
2.29.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Tarantool-patches] [PATCH 3/3] sql: introduced explicit casts test e_casts.test.lua
  2021-05-25  9:00 [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Timur Safin via Tarantool-patches
  2021-05-25  9:01 ` [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables Timur Safin via Tarantool-patches
  2021-05-25  9:01 ` [Tarantool-patches] [PATCH 2/3] sql: enabled ANY as target for explicit conversions Timur Safin via Tarantool-patches
@ 2021-05-25  9:01 ` Timur Safin via Tarantool-patches
  2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
  2021-06-01 14:02 ` [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Mergen Imeev via Tarantool-patches
  3 siblings, 1 reply; 13+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-05-25  9:01 UTC (permalink / raw)
  To: tarantool-patches, imeevma

* e_casts.test.lua is generating expressions of `cast(input as output)`
  form. And checks whether we expectedly succeed or fail, given the
  previously agreed excplicit conversion table from
  /doc/rfc/5910-consistent-sql-lua-types.md;

* At the moment we skip DECIMALs, UUIDs, ARRAYs and MAPs as not yet
  fully supported. Need to be reenabled later;

* there is `-verbose` mode one may activate to enable more detailed
  reporting.

Relates to #5910, #6009
Part of #4407
---
 test/sql-tap/e_casts.test.lua | 355 ++++++++++++++++++++++++++++++++++
 1 file changed, 355 insertions(+)
 create mode 100755 test/sql-tap/e_casts.test.lua

diff --git a/test/sql-tap/e_casts.test.lua b/test/sql-tap/e_casts.test.lua
new file mode 100755
index 000000000..eafebd5bc
--- /dev/null
+++ b/test/sql-tap/e_casts.test.lua
@@ -0,0 +1,355 @@
+#!/usr/bin/env tarantool
+local test = require("sqltester")
+test:plan(222)
+
+local yaml = require("yaml")
+local ffi = require("ffi")
+
+local verbose = 0
+
+if arg[1] == '-v' or arg[1] == '--verbose' then
+    verbose = 1
+end
+
+ffi.cdef [[
+    enum field_type {
+        FIELD_TYPE_ANY = 0,
+        FIELD_TYPE_UNSIGNED,
+        FIELD_TYPE_STRING,
+        FIELD_TYPE_NUMBER,
+        FIELD_TYPE_DOUBLE,
+        FIELD_TYPE_INTEGER,
+        FIELD_TYPE_BOOLEAN,
+        FIELD_TYPE_VARBINARY,
+        FIELD_TYPE_SCALAR,
+        FIELD_TYPE_DECIMAL,
+        FIELD_TYPE_UUID,
+        FIELD_TYPE_ARRAY,
+        FIELD_TYPE_MAP,
+        field_type_MAX
+    };
+]]
+
+-- Date/time/interval types to be uncommented and used
+-- once corresponding box implementation completed
+local t_any = ffi.C.FIELD_TYPE_ANY
+local t_unsigned = ffi.C.FIELD_TYPE_UNSIGNED
+local t_string = ffi.C.FIELD_TYPE_STRING
+local t_number = ffi.C.FIELD_TYPE_NUMBER
+local t_double = ffi.C.FIELD_TYPE_DOUBLE
+local t_integer = ffi.C.FIELD_TYPE_INTEGER
+local t_boolean = ffi.C.FIELD_TYPE_BOOLEAN
+local t_varbinary = ffi.C.FIELD_TYPE_VARBINARY
+local t_scalar = ffi.C.FIELD_TYPE_SCALAR
+local t_decimal = ffi.C.FIELD_TYPE_DECIMAL
+-- local t_date = -1
+-- local t_time = -2
+-- local t_timestamp = -3
+-- local t_interval = -4
+local t_uuid = ffi.C.FIELD_TYPE_UUID
+local t_array = ffi.C.FIELD_TYPE_ARRAY
+local t_map = ffi.C.FIELD_TYPE_MAP
+
+local proper_order = {
+    t_any,
+    t_unsigned,
+    t_string,
+    t_double,
+    t_integer,
+    t_boolean,
+    t_varbinary,
+    t_number,
+    t_decimal,
+    t_uuid,
+    -- t_date,
+    -- t_time,
+    -- t_timestamp,
+    -- t_interval,
+    t_array,
+    t_map,
+    t_scalar,
+}
+
+local type_names = {
+    [t_any]       = 'any',
+    [t_unsigned]  = 'unsigned',
+    [t_string]    = 'string',
+    [t_double]    = 'double',
+    [t_integer]   = 'integer',
+    [t_boolean]   = 'boolean',
+    [t_varbinary] = 'varbinary',
+    [t_number]    = 'number',
+    [t_decimal]   = 'decimal',
+    [t_uuid]      = 'uuid',
+    -- [t_date]      = 'date',
+    -- [t_time]      = 'time',
+    -- [t_timestamp] = 'timestamp',
+    -- [t_interval]  = 'interval',
+    [t_array]     = 'array',
+    [t_map]       = 'map',
+    [t_scalar]    = 'scalar',
+}
+
+-- not all types implemented/enabled at the moment
+-- but we do keep their projected status in the
+-- spec table
+local enabled_type = {
+    [t_any]       = false, -- there is no way in SQL to instantiate ANY type expression
+    [t_unsigned]  = true,
+    [t_string]    = true,
+    [t_double]    = true,
+    [t_integer]   = true,
+    [t_boolean]   = true,
+    [t_varbinary] = true,
+    [t_number]    = true,
+    [t_decimal]   = false,
+    [t_uuid]      = false,
+    -- [t_date]     = false,
+    -- [t_time]     = false,
+    -- [t_timestamp]= false,
+    -- [t_interval] = False,
+    [t_array]     = false,
+    [t_map]       = false,
+    [t_scalar]    = true,
+}
+
+-- table of _TSV_ (tab separated values)
+-- copied from sql-lua-tables-v5.xls // TNT implicit today
+local explicit_casts_table_spec = {
+    [t_any] =     {"Y", "S", "Y", "S", "S", "S", "S", "S", "S", "S", "S", "S", "S"},
+    [t_unsigned]= {"Y", "Y", "Y", "Y", "Y", "" , "" , "Y", "Y", "" , "" , "" , "Y"},
+    [t_string] =  {"Y", "S", "Y", "S", "S", "S", "Y", "S", "S", "S", "S", "S", "Y"},
+    [t_double] =  {"Y", "S", "Y", "Y", "S", "" , "" , "Y", "Y", "" , "" , "" , "Y"},
+    [t_integer] = {"Y", "S", "Y", "Y", "Y", "" , "" , "Y", "Y", "" , "" , "" , "Y"},
+    [t_boolean] = {"Y", "" , "Y", "" , "" , "Y", "" , "" , "" , "" , "" , "" , "Y"},
+    [t_varbinary]={"Y", "" , "Y", "N", "" , "" , "Y", "" , "" , "S", "" , "" , "Y"},
+    [t_number] =  {"Y", "S", "Y", "Y", "S", "" , "" , "Y", "Y", "" , "" , "" , "Y"},
+    [t_decimal] = {"Y", "S", "Y", "S", "S", "" , "" , "Y", "Y", "" , "" , "" , "Y"},
+    [t_uuid] =    {"Y", "" , "Y", "" , "" , "" , "Y", "" , "" , "Y", "" , "" , "Y"},
+    [t_array] =   {"Y", "N", "Y", "N", "N", "N", "" , "" , "" , "" , "Y", "" , "N"},
+    [t_map] =     {"Y", "N", "Y", "N", "N", "N", "" , "" , "" , "" , "" , "Y", "N"},
+    [t_scalar] =  {"Y", "S", "Y", "S", "S", "S", "S", "S", "S", "S", "" , "" , "Y"},
+}
+
+-- local extra_checks = false
+local explicit_casts = {}
+-- local implicit_casts = {}
+
+-- implicit conversion table is considered consistent if
+-- it's sort of symmetric against diagonal
+-- (not necessary that always/sometimes are matching
+-- but at least something should be presented)
+
+--[[ local function check_table_consistency(table)
+    for _, i in ipairs(proper_order) do
+        local string = ''
+        for _, j in ipairs(proper_order) do
+            print(i, j)
+            -- local item = implicit_casts[i][j]
+            -- string = string .. (xlat[item] or ' ')
+        end
+        print(string)
+    end
+end
+]]
+
+    -- if there is enabled extra checks then check ocnsistency of input tables
+    -- just to make sure their sanity
+--[[     if extra_checks then
+        check_table_consistency()
+    end
+ ]]
+
+local c_no = 0
+local c_maybe = 1
+local c_yes = 2
+
+local function normalize_cast(v)
+    local xlat =  {
+        ['Y'] = c_yes,
+        ['S'] = c_maybe,
+        ['N'] = c_no,
+    }
+    return xlat[v ~= nil and v or 'N']
+end
+
+local function human_cast(v)
+    local xlat = {
+        [c_yes] = 'Y',
+        [c_maybe] = 'S',
+        [c_no] = ' '
+    }
+    return xlat[v ~= nil and v or c_no]
+end
+
+local function load_casts_spec(spec_table)
+    local casts = {}
+    for i, t_from  in ipairs(proper_order) do
+        local row = spec_table[t_from]
+        casts[t_from] = {}
+        for j, t_to  in ipairs(proper_order) do
+            if enabled_type[t_from] and enabled_type[t_to] then
+                casts[t_from][t_to] = normalize_cast(spec_table[t_from][j])
+            end
+        end
+    end
+    -- if there is enabled extra checks then check ocnsistency of input tables
+    -- just to make sure their sanity
+--[[     if extra_checks then
+        check_table_consistency()
+    end ]]
+
+    return casts
+end
+
+explicit_casts = load_casts_spec(explicit_casts_table_spec)
+
+if verbose > 0 then
+    local function show_casts_table(table)
+        local max_len = #"12. varbinary" + 1
+
+        -- show banner
+        local col_names = ''
+        for i, t_val in ipairs(proper_order) do
+            col_names = col_names .. string.format("%2d |", t_val)
+        end
+        col_names = string.sub(col_names, 1, #col_names-1)
+        print(string.format("%"..max_len.."s|%s|", "", col_names))
+        -- show splitter
+        local banner = '+---+---+---+---+---+---+---+---+---+---+---+---+---+'
+        print(string.format("%"..max_len.."s%s", "", banner))
+
+        for i, from in ipairs(proper_order) do
+            local line = ''
+            for j, to in ipairs(proper_order) do
+                line = line .. string.format("%2s |", human_cast(table[from][to]))
+            end
+            line = string.sub(line, 1, #line-1)
+            local s = string.format("%2d.%10s |%s|", from, type_names[from], line)
+            print(s)
+        end
+        print(string.format("%"..max_len.."s%s", "", banner))
+    end
+
+    show_casts_table(explicit_casts)
+end
+
+local function merge_tables(...)
+    local n = select('#', ...)
+    local tables = {...}
+    local result = {}
+
+    for i=1,n do
+        local t = tables[i]
+        --print(yaml.encode(t))
+        assert(type(tables[i]) == 'table')
+        for j,v in pairs(t) do
+            table.insert(result, v)
+        end
+    end
+    return result
+end
+
+local gen_type_samples = {
+        [t_unsigned]  = {"0", "1", "2"},
+        [t_integer]   = {"-678", "-1", "0", "1", "2", "3", "678"},
+        [t_double]    = {"0.0", "123.4", "-567.8"},
+        [t_string]    = {"''", "'1'", "'abc'", "'def'", "'TRUE'", "'FALSE'"},
+        [t_boolean]   = {"false", "true", "null"},
+        [t_varbinary] = {"X'312E3233'", "X'2D392E3837'", "X'302E30303031'"},
+}
+
+local function gen_type_exprs(type)
+    if type == t_number then
+        return merge_tables(gen_type_samples[t_unsigned],
+                            gen_type_samples[t_integer],
+                            gen_type_samples[t_double])
+    end
+    if type == t_scalar then
+        return merge_tables(gen_type_samples[t_unsigned],
+                            gen_type_samples[t_integer],
+                            gen_type_samples[t_double],
+                            gen_type_samples[t_string],
+                            gen_type_samples[t_boolean],
+                            gen_type_samples[t_varbinary])
+    end
+    return gen_type_samples[type] or {}
+end
+
+local function gen_sql_cast_from_to(t_from, t_to)
+    local queries = {}
+    local from_exprs = gen_type_exprs(t_from)
+    local to_typename = type_names[t_to]
+    for _, expr in pairs(from_exprs) do
+        local query = string.format([[ select cast(%s as %s); ]], expr, to_typename)
+        table.insert(queries, query)
+    end
+    return queries
+end
+
+local function catch_query(query)
+    local result = {pcall(box.execute, query)}
+
+    if not result[1] or result[3] ~= nil then
+        return false, result[3]
+    end
+    return true, result[2]
+end
+
+local function label_for(from, to, query)
+    local parent_frame = debug.getinfo(2, "nSl")
+    local filename = parent_frame.source:sub(1,1) == "@" and parent_frame.source:sub(2)
+    local line = parent_frame.currentline
+    return string.format("%s+%d:[%s,%s] %s", filename, line,
+                         type_names[from], type_names[to], query)
+end
+
+for i, from in ipairs(proper_order) do
+    for j, to in ipairs(proper_order) do
+        -- skip ANY, DECIMAL, UUID, etc.
+        if enabled_type[from] and enabled_type[to] then
+            local cell = explicit_casts[from][to]
+            local gen = gen_sql_cast_from_to(from, to)
+            local failures = {}
+            local successes = {}
+            local castable = false
+            local expected = explicit_casts[from][to]
+            if verbose > 0 then
+                print(expected, yaml.encode(gen))
+            end
+            for i, v in pairs(gen) do
+                local ok, result
+                ok, result = catch_query(v)
+                if verbose > 0 then
+                    print(string.format("ok = %s, result = %s, query = %s",
+                         ok, result, v))
+
+                end
+                -- print(v, 'ok'..yaml.encode(ok), 'result'..yaml.encode(result))
+                if expected == c_yes then
+                    test:ok(true == ok, label_for(from, to, v))
+                elseif expected == c_no then
+                    test:ok(false == ok, label_for(from, to, v))
+                else
+                -- we can't report immediately for c_maybe because some 
+                -- cases allowed to fail, so postpone decision
+                    if ok then
+                        castable = true
+                        table.insert(successes, {result, v})
+                    else
+                        table.insert(failures, {result, v})
+                    end
+                end
+            end
+
+            -- ok, we aggregated stats for c_maybe mode - check it now
+            if expected == c_maybe then
+                    test:ok(castable, label_for(from, to, #gen and gen[1] or ''),
+                            failures)
+            end
+        end
+    end
+end
+
+
+test:finish_test()
-- 
2.29.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables
  2021-05-25  9:00 [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Timur Safin via Tarantool-patches
                   ` (2 preceding siblings ...)
  2021-05-25  9:01 ` [Tarantool-patches] [PATCH 3/3] sql: introduced explicit casts test e_casts.test.lua Timur Safin via Tarantool-patches
@ 2021-06-01 14:02 ` Mergen Imeev via Tarantool-patches
  2021-06-02 21:04   ` Timur Safin via Tarantool-patches
  3 siblings, 1 reply; 13+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-06-01 14:02 UTC (permalink / raw)
  To: Timur Safin; +Cc: tarantool-patches

Hi! Thank you for the patch-set. See 6 comments below.

0. In general, I want to say that I do not like your style of 
commit-messages.
I think there are too many unnecessary words and constructions in it. But
that's just my opinion. I have not seen any rules prohibiting this.

1. Why "tables" in name of the patch? I see only one table.

On Tue, May 25, 2021 at 12:00:59PM +0300, Timur Safin wrote:
> 
> Recent RFC "Consistent Lua/SQL types" (#6009) defined ideal explicit
> and implicit conversion tables we would like to have for all current 
> and future Taranool SQL types. 
> 
2. Why do you need number of pull-request here? I mean, you already have 
a name
of RFC.

> This patchset modifies explict conversion tables, and implicit 
3. Here should be "explicit" I believe.

> conversion table to come soon. The ideal picture would be as below:
> 

>               | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
>               +---+---+---+---+---+---+---+---+---+---+---+---+---+
>  0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
>  1.  unsigned |   | Y | Y | Y | Y |   |   | Y |   |   |   |   | Y |
>  2.    string |   | S | Y | S | S | S | Y | S |   |   |   |   | Y |
>  4.    double |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
>  5.   integer |   | S | Y | Y | Y |   |   | Y |   |   |   |   | Y |
>  6.   boolean |   |   | Y |   |   | Y |   |   |   |   |   |   | Y |
>  7. varbinary |   |   | Y |   |   |   | Y |   |   |   |   |   | Y |
>  3.    number |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
>  9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
>  8.    scalar |   | S | Y | S | S | S | S | S |   |   |   |   | Y |
>               +---+---+---+---+---+---+---+---+---+---+---+---+---+
> 
4. Why there is such strange order in table? Also, I believe it makes 
sense to
describe what "S", "Y" means and why some cells are empty.

> Please pay attention that we omit DECIMAL, UUID, SCALAR and MAP rows
> and columns, as they not yet fully supported by Tarantool SQL. Once 
> their support will be landed we will modify conversion table and 
> tests (which we also introducing with current patchset).
> 
5. Why not drop them if they are omitted? I mean, "modify" in name of 
the letter
means that you change existing rules.

6. Please include links to issue and branch in cover-letter.

> 
> Timur Safin (3):
>   sql: fixes for boolean expressions in explicit converstion tables
>   sql: enabled ANY as target for explicit conversions
>   sql: introduced explicit casts test e_casts.test.lua
> 
>  extra/mkkeywordhash.c           |   3 +-
>  src/box/sql/mem.c               |  39 +---
>  src/box/sql/parse.y             |   3 +-
>  test/sql-tap/cse.test.lua       |  12 +-
>  test/sql-tap/e_casts.test.lua   | 355 ++++++++++++++++++++++++++++++++
>  test/sql-tap/e_select1.test.lua |   2 +-
>  test/sql-tap/in1.test.lua       |  16 +-
>  test/sql-tap/keyword1.test.lua  |   2 +-
>  test/sql-tap/misc3.test.lua     |   2 +-
>  test/sql/boolean.result         |  38 +---
>  test/sql/types.result           |  14 +-
>  11 files changed, 390 insertions(+), 96 deletions(-)
>  create mode 100755 test/sql-tap/e_casts.test.lua
> 
> -- 
> 2.29.2
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables
  2021-05-25  9:01 ` [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables Timur Safin via Tarantool-patches
@ 2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
  2021-06-02 21:03     ` Timur Safin via Tarantool-patches
  2021-06-02 21:10     ` Timur Safin via Tarantool-patches
  0 siblings, 2 replies; 13+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-06-01 14:02 UTC (permalink / raw)
  To: Timur Safin; +Cc: tarantool-patches

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/3] sql: enabled ANY as target for explicit conversions
  2021-05-25  9:01 ` [Tarantool-patches] [PATCH 2/3] sql: enabled ANY as target for explicit conversions Timur Safin via Tarantool-patches
@ 2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
  2021-06-02 21:04     ` Timur Safin via Tarantool-patches
  0 siblings, 1 reply; 13+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-06-01 14:02 UTC (permalink / raw)
  To: Timur Safin; +Cc: tarantool-patches

Thank you for the patch. See 10 comments below.

1. I believe we do not have field type ANY in SQL. Also, I believe that 
to add
it to explicit cast we have to add it to SQL.

2. What this patch actually does? I mean, after this patch I still not able
to cast values to ANY:

tarantool> box.execute('SELECT CAST(1 AS ANY);')
---
- null
- 'At line 1 at or near position 18: keyword ''ANY'' is reserved. Please 
use double
   quotes if ''ANY'' is an identifier.'
...

On Tue, May 25, 2021 at 12:01:01PM +0300, Timur Safin wrote:
> For consistency sake it was decided to provide
> `CAST(anything TO ANY)` as allowed, but still noop.
> 
> Fuller explicit conversions table is described in the
> /doc/rfc/5910-consistent-sql-lua-types.md and here we
> represent only relevant to ANY.
> 
3. I still think that it is better to put RFC name instead of  this relative
link.

> Furthermore, there is no direct way in SQL to create literal and
> expression of ANY type, thus there is no defined CAST from ANY
> to anything else.
> 
4. There is no way to create literal, however you can use space with 
field type
ANY, right?

>               | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
>               +---+---+---+---+---+---+---+---+---+---+---+---+---+
>  0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
>  1.  unsigned |   | . | . | . | . |   |   | . |   |   |   |   | Y |
>  2.    string |   | . | . | . | . | . | . | . |   |   |   |   | Y |
>  4.    double |   | . | . | . | . |   |   | . |   |   |   |   | Y |
>  5.   integer |   | . | . | . | . |   |   | . |   |   |   |   | Y |
>  6.   boolean |   |   | . |   |   | . |   |   |   |   |   |   | Y |
>  7. varbinary |   |   | . |   |   |   | . |   |   |   |   |   | Y |
>  3.    number |   | . | . | . | . |   |   | . |   |   |   |   | Y |
>  9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
> 12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
>  8.    scalar |   | . | . | . | . | . | . | . |   |   |   |   | Y |
>               +---+---+---+---+---+---+---+---+---+---+---+---+---+
> 
5. This strange order again.

6. Again, do you really need this table in commit-message? This time it 
is also
a lot simpler to just describe this explicit conversion rules using words.

> * We introduced new token ANY as known lexem to the Lemon parser.
>   While renamed prior %wildcard to ANYTHING;
> * Modified explicit conversion rules to allow ANY as _target_ in
>   conversion pair.
> 
> Relates to #5910, #6009
> Part of #4407
7. Wrong issue number. Also, I am again not sure that you should include
"Relates to" here.

> ---
>  extra/mkkeywordhash.c          | 3 ++-
>  src/box/sql/mem.c              | 2 ++
>  src/box/sql/parse.y            | 3 ++-
>  test/sql-tap/keyword1.test.lua | 2 +-
>  4 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
> index 7480c0211..d343cf706 100644
> --- a/extra/mkkeywordhash.c
> +++ b/extra/mkkeywordhash.c
> @@ -60,6 +60,7 @@ static Keyword aKeywordTable[] = {
>    { "ALL",                    "TK_ALL",         true  },
>    { "ALTER",                  "TK_ALTER",       true  },
>    { "ANALYZE",                "TK_STANDARD",    true  },
> +  { "ANY",                    "TK_ID",          true  },
8. Why some types describes as TK_ID, and some as TK_<type name>?

>    { "AND",                    "TK_AND",         true  },
>    { "AS",                     "TK_AS",          true  },
>    { "ASC",                    "TK_ASC",         true  },
> @@ -178,7 +179,7 @@ static Keyword aKeywordTable[] = {
>    { "WITH",                   "TK_WITH",        true  },
>    { "WHEN",                   "TK_WHEN",        true  },
>    { "WHERE",                  "TK_WHERE",       true  },
> -  { "ANY",                    "TK_STANDARD",    true  },
> +  { "ANYTHING",               "TK_STANDARD",    true  },
9. Why not name it as TK_WILDCARD to avoid further misunderstanding?

>    { "ASENSITIVE",             "TK_STANDARD",    true  },
>    { "BLOB",                   "TK_STANDARD",    true  },
>    { "BINARY",                 "TK_ID",          true  },
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index d40366754..b4f1078ae 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -874,6 +874,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
>  		    (mem->flags & MEM_Subtype) != 0)
>  			return -1;
>  		return 0;
> +	case FIELD_TYPE_ANY:
> +		return 0;
>  	default:
>  		break;
>  	}
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index abc363951..e586d0181 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -270,7 +270,7 @@ columnlist ::= tcons.
>    QUERY KEY OFFSET RAISE RELEASE REPLACE RESTRICT
>    RENAME CTIME_KW IF ENABLE DISABLE
>    .
> -%wildcard ANY.
> +%wildcard ANYTHING.
>  
>  
>  // And "ids" is an identifer-or-string.
> @@ -1834,6 +1834,7 @@ typedef(A) ::= SCALAR . { A.type = FIELD_TYPE_SCALAR; }
>  typedef(A) ::= BOOL . { A.type = FIELD_TYPE_BOOLEAN; }
>  typedef(A) ::= BOOLEAN . { A.type = FIELD_TYPE_BOOLEAN; }
>  typedef(A) ::= VARBINARY . { A.type = FIELD_TYPE_VARBINARY; }
> +typedef(A) ::= ANY . { A.type = FIELD_TYPE_ANY; }
>  
>  /**
>   * Time-like types are temporary disabled, until they are
> diff --git a/test/sql-tap/keyword1.test.lua b/test/sql-tap/keyword1.test.lua
> index f9a9c6865..4855f3ebc 100755
> --- a/test/sql-tap/keyword1.test.lua
> +++ b/test/sql-tap/keyword1.test.lua
> @@ -242,7 +242,7 @@ for _, kw in ipairs(bannedkws) do
>      local query = 'CREATE TABLE '..kw..'(a INT PRIMARY KEY);'
>      if kw == 'end' or kw == 'match' or kw == 'release' or kw == 'rename' or
>         kw == 'replace' or kw == 'binary' or kw == 'character' or
> -       kw == 'smallint' then
> +       kw == 'smallint' or kw == 'any' then
>          test:do_catchsql_test(
>          "bannedkw1-"..kw..".1",
>          query, {
> -- 
> 2.29.2
> 
10. Why there is no tests?


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 3/3] sql: introduced explicit casts test e_casts.test.lua
  2021-05-25  9:01 ` [Tarantool-patches] [PATCH 3/3] sql: introduced explicit casts test e_casts.test.lua Timur Safin via Tarantool-patches
@ 2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
  2021-06-02 21:04     ` Timur Safin via Tarantool-patches
  0 siblings, 1 reply; 13+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-06-01 14:02 UTC (permalink / raw)
  To: Timur Safin; +Cc: tarantool-patches

Thank you for the patch. See 13 comments below.

On Tue, May 25, 2021 at 12:01:02PM +0300, Timur Safin wrote:
> * e_casts.test.lua is generating expressions of `cast(input as output)`
>   form. And checks whether we expectedly succeed or fail, given the
>   previously agreed excplicit conversion table from
>   /doc/rfc/5910-consistent-sql-lua-types.md;
> 
> * At the moment we skip DECIMALs, UUIDs, ARRAYs and MAPs as not yet
>   fully supported. Need to be reenabled later;
> 
> * there is `-verbose` mode one may activate to enable more detailed
>   reporting.
1. How to use this mode? I mean,
./test-run.py sql-tap/e_casts.test.lua --verbose
gives a lot simpler result that I expect from this much of code.

2. What means e_casts? If it is for explicit casts, why not name it 
something
like "explicit_cast.test.lua".

3. As far as I remember, "sometimes" doesn't mean that CAST(1.0 as UNSIGNED)
can return 1 or can throw an error. It means that there are rules that 
define
when values of one type can be converted to values of another type. So,
instead of checking failures for "maybe" we can check that "from" value
should fail or not according to these rules and use proper test.

Also, using this approach we can try to use randomized "from" values. Just
thought.

4. Why you test only literals? Nothing indicates that values from spaces
shouldn't be tested here.

> 
> Relates to #5910, #6009
> Part of #4407
5. Wrong issue number. Also, I still think that "relates to" is not 
needed here.

> ---
>  test/sql-tap/e_casts.test.lua | 355 ++++++++++++++++++++++++++++++++++
>  1 file changed, 355 insertions(+)
>  create mode 100755 test/sql-tap/e_casts.test.lua
> 
> diff --git a/test/sql-tap/e_casts.test.lua b/test/sql-tap/e_casts.test.lua
> new file mode 100755
> index 000000000..eafebd5bc
> --- /dev/null
> +++ b/test/sql-tap/e_casts.test.lua
> @@ -0,0 +1,355 @@
> +#!/usr/bin/env tarantool
> +local test = require("sqltester")
6. What point to use sqltester here is you do not use its functions? You 
can use
'tap' instead.

> +test:plan(222)
> +
> +local yaml = require("yaml")
> +local ffi = require("ffi")
> +
> +local verbose = 0
> +
> +if arg[1] == '-v' or arg[1] == '--verbose' then
7. How to use '-v' option?

> +    verbose = 1
> +end
> +
> +ffi.cdef [[
> +    enum field_type {
> +        FIELD_TYPE_ANY = 0,
> +        FIELD_TYPE_UNSIGNED,
> +        FIELD_TYPE_STRING,
> +        FIELD_TYPE_NUMBER,
> +        FIELD_TYPE_DOUBLE,
> +        FIELD_TYPE_INTEGER,
> +        FIELD_TYPE_BOOLEAN,
> +        FIELD_TYPE_VARBINARY,
> +        FIELD_TYPE_SCALAR,
> +        FIELD_TYPE_DECIMAL,
> +        FIELD_TYPE_UUID,
> +        FIELD_TYPE_ARRAY,
> +        FIELD_TYPE_MAP,
> +        field_type_MAX
> +    };
> +]]
8. Is there any point to use ffi here? Can't you use something simpler?
For example:
local types = {
	any		= 0,
	unsigned	= 1,
...

Than instead of t_any you can use types.any and so on.

If you start from 1 instead of 0 you can also use type_names instead of
proper_order.

> +
> +-- Date/time/interval types to be uncommented and used
> +-- once corresponding box implementation completed
> +local t_any = ffi.C.FIELD_TYPE_ANY
> +local t_unsigned = ffi.C.FIELD_TYPE_UNSIGNED
> +local t_string = ffi.C.FIELD_TYPE_STRING
> +local t_number = ffi.C.FIELD_TYPE_NUMBER
> +local t_double = ffi.C.FIELD_TYPE_DOUBLE
> +local t_integer = ffi.C.FIELD_TYPE_INTEGER
> +local t_boolean = ffi.C.FIELD_TYPE_BOOLEAN
> +local t_varbinary = ffi.C.FIELD_TYPE_VARBINARY
> +local t_scalar = ffi.C.FIELD_TYPE_SCALAR
> +local t_decimal = ffi.C.FIELD_TYPE_DECIMAL
> +-- local t_date = -1
> +-- local t_time = -2
> +-- local t_timestamp = -3
> +-- local t_interval = -4
> +local t_uuid = ffi.C.FIELD_TYPE_UUID
> +local t_array = ffi.C.FIELD_TYPE_ARRAY
> +local t_map = ffi.C.FIELD_TYPE_MAP
> +
> +local proper_order = {
> +    t_any,
> +    t_unsigned,
> +    t_string,
> +    t_double,
> +    t_integer,
> +    t_boolean,
> +    t_varbinary,
> +    t_number,
> +    t_decimal,
> +    t_uuid,
> +    -- t_date,
> +    -- t_time,
> +    -- t_timestamp,
> +    -- t_interval,
9. I think it is better to check them for "wrong type name" or remove these
types.

> +    t_array,
> +    t_map,
> +    t_scalar,
> +}
> +
> +local type_names = {
> +    [t_any]       = 'any',
> +    [t_unsigned]  = 'unsigned',
> +    [t_string]    = 'string',
> +    [t_double]    = 'double',
> +    [t_integer]   = 'integer',
> +    [t_boolean]   = 'boolean',
> +    [t_varbinary] = 'varbinary',
> +    [t_number]    = 'number',
> +    [t_decimal]   = 'decimal',
> +    [t_uuid]      = 'uuid',
> +    -- [t_date]      = 'date',
> +    -- [t_time]      = 'time',
> +    -- [t_timestamp] = 'timestamp',
> +    -- [t_interval]  = 'interval',
> +    [t_array]     = 'array',
> +    [t_map]       = 'map',
> +    [t_scalar]    = 'scalar',
> +}
> +
> +-- not all types implemented/enabled at the moment
> +-- but we do keep their projected status in the
> +-- spec table
> +local enabled_type = {
> +    [t_any]       = false, -- there is no way in SQL to instantiate ANY type expression
> +    [t_unsigned]  = true,
> +    [t_string]    = true,
> +    [t_double]    = true,
> +    [t_integer]   = true,
> +    [t_boolean]   = true,
> +    [t_varbinary] = true,
> +    [t_number]    = true,
> +    [t_decimal]   = false,
> +    [t_uuid]      = false,
> +    -- [t_date]     = false,
> +    -- [t_time]     = false,
> +    -- [t_timestamp]= false,
> +    -- [t_interval] = False,
> +    [t_array]     = false,
> +    [t_map]       = false,
> +    [t_scalar]    = true,
> +}
> +
> +-- table of _TSV_ (tab separated values)
> +-- copied from sql-lua-tables-v5.xls // TNT implicit today
> +local explicit_casts_table_spec = {
> +    [t_any] =     {"Y", "S", "Y", "S", "S", "S", "S", "S", "S", "S", "S", "S", "S"},
> +    [t_unsigned]= {"Y", "Y", "Y", "Y", "Y", "" , "" , "Y", "Y", "" , "" , "" , "Y"},
> +    [t_string] =  {"Y", "S", "Y", "S", "S", "S", "Y", "S", "S", "S", "S", "S", "Y"},
> +    [t_double] =  {"Y", "S", "Y", "Y", "S", "" , "" , "Y", "Y", "" , "" , "" , "Y"},
> +    [t_integer] = {"Y", "S", "Y", "Y", "Y", "" , "" , "Y", "Y", "" , "" , "" , "Y"},
> +    [t_boolean] = {"Y", "" , "Y", "" , "" , "Y", "" , "" , "" , "" , "" , "" , "Y"},
> +    [t_varbinary]={"Y", "" , "Y", "N", "" , "" , "Y", "" , "" , "S", "" , "" , "Y"},
> +    [t_number] =  {"Y", "S", "Y", "Y", "S", "" , "" , "Y", "Y", "" , "" , "" , "Y"},
> +    [t_decimal] = {"Y", "S", "Y", "S", "S", "" , "" , "Y", "Y", "" , "" , "" , "Y"},
> +    [t_uuid] =    {"Y", "" , "Y", "" , "" , "" , "Y", "" , "" , "Y", "" , "" , "Y"},
> +    [t_array] =   {"Y", "N", "Y", "N", "N", "N", "" , "" , "" , "" , "Y", "" , "N"},
> +    [t_map] =     {"Y", "N", "Y", "N", "N", "N", "" , "" , "" , "" , "" , "Y", "N"},
> +    [t_scalar] =  {"Y", "S", "Y", "S", "S", "S", "S", "S", "S", "S", "" , "" , "Y"},
> +}
10. Why there is 'N' and '' both in table? Can't be make it use only one 
of these?

> +
> +-- local extra_checks = false
> +local explicit_casts = {}
> +-- local implicit_casts = {}
> +
> +-- implicit conversion table is considered consistent if
> +-- it's sort of symmetric against diagonal
> +-- (not necessary that always/sometimes are matching
> +-- but at least something should be presented)
> +
> +--[[ local function check_table_consistency(table)
> +    for _, i in ipairs(proper_order) do
> +        local string = ''
> +        for _, j in ipairs(proper_order) do
> +            print(i, j)
> +            -- local item = implicit_casts[i][j]
> +            -- string = string .. (xlat[item] or ' ')
> +        end
> +        print(string)
> +    end
> +end
> +]]
> +
> +    -- if there is enabled extra checks then check ocnsistency of input tables
> +    -- just to make sure their sanity
> +--[[     if extra_checks then
> +        check_table_consistency()
> +    end
> + ]]
> +
11. Please drop commented part of code.

> +local c_no = 0
> +local c_maybe = 1
> +local c_yes = 2
> +
> +local function normalize_cast(v)
> +    local xlat =  {
> +        ['Y'] = c_yes,
> +        ['S'] = c_maybe,
> +        ['N'] = c_no,
> +    }
> +    return xlat[v ~= nil and v or 'N']
> +end
> +
> +local function human_cast(v)
> +    local xlat = {
> +        [c_yes] = 'Y',
> +        [c_maybe] = 'S',
> +        [c_no] = ' '
> +    }
> +    return xlat[v ~= nil and v or c_no]
> +end
> +
> +local function load_casts_spec(spec_table)
> +    local casts = {}
> +    for i, t_from  in ipairs(proper_order) do
> +        local row = spec_table[t_from]
> +        casts[t_from] = {}
> +        for j, t_to  in ipairs(proper_order) do
> +            if enabled_type[t_from] and enabled_type[t_to] then
> +                casts[t_from][t_to] = normalize_cast(spec_table[t_from][j])
> +            end
> +        end
> +    end
> +    -- if there is enabled extra checks then check ocnsistency of input tables
> +    -- just to make sure their sanity
> +--[[     if extra_checks then
> +        check_table_consistency()
> +    end ]]
> +
> +    return casts
> +end
> +
> +explicit_casts = load_casts_spec(explicit_casts_table_spec)
> +
> +if verbose > 0 then
> +    local function show_casts_table(table)
12. I believe word 'table' is reserved for in Lua. Not sure that it is good
idea to overwrite this reserved word.

> +        local max_len = #"12. varbinary" + 1
> +
> +        -- show banner
> +        local col_names = ''
> +        for i, t_val in ipairs(proper_order) do
> +            col_names = col_names .. string.format("%2d |", t_val)
> +        end
> +        col_names = string.sub(col_names, 1, #col_names-1)
> +        print(string.format("%"..max_len.."s|%s|", "", col_names))
> +        -- show splitter
> +        local banner = '+---+---+---+---+---+---+---+---+---+---+---+---+---+'
> +        print(string.format("%"..max_len.."s%s", "", banner))
> +
> +        for i, from in ipairs(proper_order) do
> +            local line = ''
> +            for j, to in ipairs(proper_order) do
> +                line = line .. string.format("%2s |", human_cast(table[from][to]))
> +            end
> +            line = string.sub(line, 1, #line-1)
> +            local s = string.format("%2d.%10s |%s|", from, type_names[from], line)
> +            print(s)
> +        end
> +        print(string.format("%"..max_len.."s%s", "", banner))
> +    end
> +
> +    show_casts_table(explicit_casts)
> +end
> +
> +local function merge_tables(...)
> +    local n = select('#', ...)
> +    local tables = {...}
> +    local result = {}
> +
> +    for i=1,n do
> +        local t = tables[i]
> +        --print(yaml.encode(t))
> +        assert(type(tables[i]) == 'table')
> +        for j,v in pairs(t) do
> +            table.insert(result, v)
> +        end
> +    end
> +    return result
> +end
> +
> +local gen_type_samples = {
> +        [t_unsigned]  = {"0", "1", "2"},
> +        [t_integer]   = {"-678", "-1", "0", "1", "2", "3", "678"},
> +        [t_double]    = {"0.0", "123.4", "-567.8"},
> +        [t_string]    = {"''", "'1'", "'abc'", "'def'", "'TRUE'", "'FALSE'"},
> +        [t_boolean]   = {"false", "true", "null"},
> +        [t_varbinary] = {"X'312E3233'", "X'2D392E3837'", "X'302E30303031'"},
> +}
> +
> +local function gen_type_exprs(type)
> +    if type == t_number then
> +        return merge_tables(gen_type_samples[t_unsigned],
> +                            gen_type_samples[t_integer],
> +                            gen_type_samples[t_double])
> +    end
> +    if type == t_scalar then
> +        return merge_tables(gen_type_samples[t_unsigned],
> +                            gen_type_samples[t_integer],
> +                            gen_type_samples[t_double],
> +                            gen_type_samples[t_string],
> +                            gen_type_samples[t_boolean],
> +                            gen_type_samples[t_varbinary])
> +    end
> +    return gen_type_samples[type] or {}
> +end
13. Do you need this function? You can just expand gen_type_samples 
using its
code.

This comment can be implemented for some other functions, for example 
the next
one. I think it will make code simpler to understand.

> +
> +local function gen_sql_cast_from_to(t_from, t_to)
> +    local queries = {}
> +    local from_exprs = gen_type_exprs(t_from)
> +    local to_typename = type_names[t_to]
> +    for _, expr in pairs(from_exprs) do
> +        local query = string.format([[ select cast(%s as %s); ]], expr, to_typename)
> +        table.insert(queries, query)
> +    end
> +    return queries
> +end
> +
> +local function catch_query(query)
> +    local result = {pcall(box.execute, query)}
> +
> +    if not result[1] or result[3] ~= nil then
> +        return false, result[3]
> +    end
> +    return true, result[2]
> +end
> +
> +local function label_for(from, to, query)
> +    local parent_frame = debug.getinfo(2, "nSl")
> +    local filename = parent_frame.source:sub(1,1) == "@" and parent_frame.source:sub(2)
> +    local line = parent_frame.currentline
> +    return string.format("%s+%d:[%s,%s] %s", filename, line,
> +                         type_names[from], type_names[to], query)
> +end
> +
> +for i, from in ipairs(proper_order) do
> +    for j, to in ipairs(proper_order) do
> +        -- skip ANY, DECIMAL, UUID, etc.
> +        if enabled_type[from] and enabled_type[to] then
> +            local cell = explicit_casts[from][to]
> +            local gen = gen_sql_cast_from_to(from, to)
> +            local failures = {}
> +            local successes = {}
> +            local castable = false
> +            local expected = explicit_casts[from][to]
> +            if verbose > 0 then
> +                print(expected, yaml.encode(gen))
> +            end
> +            for i, v in pairs(gen) do
> +                local ok, result
> +                ok, result = catch_query(v)
> +                if verbose > 0 then
> +                    print(string.format("ok = %s, result = %s, query = %s",
> +                         ok, result, v))
> +
> +                end
> +                -- print(v, 'ok'..yaml.encode(ok), 'result'..yaml.encode(result))
> +                if expected == c_yes then
> +                    test:ok(true == ok, label_for(from, to, v))
> +                elseif expected == c_no then
> +                    test:ok(false == ok, label_for(from, to, v))
> +                else
> +                -- we can't report immediately for c_maybe because some 
> +                -- cases allowed to fail, so postpone decision
> +                    if ok then
> +                        castable = true
> +                        table.insert(successes, {result, v})
> +                    else
> +                        table.insert(failures, {result, v})
> +                    end
> +                end
> +            end
> +
> +            -- ok, we aggregated stats for c_maybe mode - check it now
> +            if expected == c_maybe then
> +                    test:ok(castable, label_for(from, to, #gen and gen[1] or ''),
> +                            failures)
> +            end
> +        end
> +    end
> +end
> +
> +
> +test:finish_test()
> -- 
> 2.29.2
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables
  2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
@ 2021-06-02 21:03     ` Timur Safin via Tarantool-patches
  2021-06-02 21:10     ` Timur Safin via Tarantool-patches
  1 sibling, 0 replies; 13+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-02 21:03 UTC (permalink / raw)
  To: 'Mergen Imeev'; +Cc: tarantool-patches

: From: Mergen Imeev <imeevma@tarantool.org>
: Sent: Tuesday, June 1, 2021 5:03 PM
: To: Timur Safin <tsafin@tarantool.org>
: Cc: tarantool-patches@dev.tarantool.org
: Subject: Re: [PATCH 1/3] sql: fixes for boolean expressions in explicit
: converstion tables
: 
: 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.

Yup, will try

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

They are equivalent

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

This is local modification of a table, with all changes highlighted. 
As I said it elsewhere - picture worth thousands words. Verbal description
tends to be more verbose, and hard to skim. This simple table is very easy
to recognize the changed and unmodified.

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

We will live with this order forever, due to RFC pictures. 

: 
: > 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?

Yup, I needed it at this version. Because I've updated all touched
tests in this commit, but larger test, with generated expressions
and fully covered matrix of all combination assumed to be committed
separately.

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

Relates to creates links in GitHub for easier navigation between
related. And original discussion _is_ related to this whole process.

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

Good point. Makes sense.

: 
: >          ]]
: >      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 '<>'.

Yup.

: 
: >      ]], {
: >          -- <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.

Yup.

: 
: >      ]], {
: >          -- <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.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables
  2021-06-01 14:02 ` [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Mergen Imeev via Tarantool-patches
@ 2021-06-02 21:04   ` Timur Safin via Tarantool-patches
  0 siblings, 0 replies; 13+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-02 21:04 UTC (permalink / raw)
  To: 'Mergen Imeev'; +Cc: tarantool-patches

: From: Mergen Imeev <imeevma@tarantool.org>
: Sent: Tuesday, June 1, 2021 5:03 PM
: To: Timur Safin <tsafin@tarantool.org>
: Cc: tarantool-patches@dev.tarantool.org
: Subject: Re: [PATCH 0/3] sql: modify explicit conversion tables
: 
: Hi! Thank you for the patch-set. See 6 comments below.
: 
: 0. In general, I want to say that I do not like your style of
: commit-messages.
: I think there are too many unnecessary words and constructions in it. But
: that's just my opinion. I have not seen any rules prohibiting this.

¯\_(ツ)_/¯

Good you realize the possibility of various styles of commit messages
from different people. And that some people do have several decades prior
experience in American companies with some established English writing skills. 

: 
: 1. Why "tables" in name of the patch? I see only one table.

There will be another table - implicit casts.

: 
: On Tue, May 25, 2021 at 12:00:59PM +0300, Timur Safin wrote:
: >
: > Recent RFC "Consistent Lua/SQL types" (#6009) defined ideal explicit
: > and implicit conversion tables we would like to have for all current
: > and future Taranool SQL types.
: >
: 2. Why do you need number of pull-request here? I mean, you already have
: a name
: of RFC.

They are mostly equivalent, yes. 

: 
: > This patchset modifies explict conversion tables, and implicit
: 3. Here should be "explicit" I believe.

Will include implicit soon.

: 
: > conversion table to come soon. The ideal picture would be as below:
: >
: 
: >               | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
: >               +---+---+---+---+---+---+---+---+---+---+---+---+---+
: >  0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
: >  1.  unsigned |   | Y | Y | Y | Y |   |   | Y |   |   |   |   | Y |
: >  2.    string |   | S | Y | S | S | S | Y | S |   |   |   |   | Y |
: >  4.    double |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
: >  5.   integer |   | S | Y | Y | Y |   |   | Y |   |   |   |   | Y |
: >  6.   boolean |   |   | Y |   |   | Y |   |   |   |   |   |   | Y |
: >  7. varbinary |   |   | Y |   |   |   | Y |   |   |   |   |   | Y |
: >  3.    number |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
: >  9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
: > 10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
: > 11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
: > 12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
: >  8.    scalar |   | S | Y | S | S | S | S | S |   |   |   |   | Y |
: >               +---+---+---+---+---+---+---+---+---+---+---+---+---+
: >
: 4. Why there is such strange order in table? 

This order is from illustrations of the spec. It's too late to redraw
all pictures there, but reordering here is easy. So we need to live 
with this order forever :(

: Also, I believe it makes
: sense to
: describe what "S", "Y" means and why some cells are empty.

Yes, good point. 

: 
: > Please pay attention that we omit DECIMAL, UUID, SCALAR and MAP rows
: > and columns, as they not yet fully supported by Tarantool SQL. Once
: > their support will be landed we will modify conversion table and
: > tests (which we also introducing with current patchset).
: >
: 5. Why not drop them if they are omitted? I mean, "modify" in name of
: the letter
: means that you change existing rules.

Because they already visualized in the RFC, and table will significantly
differ to version from spec, if we will remove all not yet implemented
columns and rows (and if we would reorder them in correct 
`enum field_type` order)

If we would keep this table more or less in the same form for all of
future updates in future commits for newer types, then this might
produce consistent picture along the way.

: 
: 6. Please include links to issue and branch in cover-letter.

Yup

: 
: >
: > Timur Safin (3):
: >   sql: fixes for boolean expressions in explicit converstion tables
: >   sql: enabled ANY as target for explicit conversions
: >   sql: introduced explicit casts test e_casts.test.lua
: >
: >  extra/mkkeywordhash.c           |   3 +-
: >  src/box/sql/mem.c               |  39 +---
: >  src/box/sql/parse.y             |   3 +-
: >  test/sql-tap/cse.test.lua       |  12 +-
: >  test/sql-tap/e_casts.test.lua   | 355 ++++++++++++++++++++++++++++++++
: >  test/sql-tap/e_select1.test.lua |   2 +-
: >  test/sql-tap/in1.test.lua       |  16 +-
: >  test/sql-tap/keyword1.test.lua  |   2 +-
: >  test/sql-tap/misc3.test.lua     |   2 +-
: >  test/sql/boolean.result         |  38 +---
: >  test/sql/types.result           |  14 +-
: >  11 files changed, 390 insertions(+), 96 deletions(-)
: >  create mode 100755 test/sql-tap/e_casts.test.lua
: >
: > --
: > 2.29.2
: >


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/3] sql: enabled ANY as target for explicit conversions
  2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
@ 2021-06-02 21:04     ` Timur Safin via Tarantool-patches
  0 siblings, 0 replies; 13+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-02 21:04 UTC (permalink / raw)
  To: 'Mergen Imeev'; +Cc: tarantool-patches

: From: Mergen Imeev <imeevma@tarantool.org>
: Sent: Tuesday, June 1, 2021 5:03 PM
: To: Timur Safin <tsafin@tarantool.org>
: Cc: tarantool-patches@dev.tarantool.org
: Subject: Re: [PATCH 2/3] sql: enabled ANY as target for explicit conversions
: 
: Thank you for the patch. See 10 comments below.
: 
: 1. I believe we do not have field type ANY in SQL. Also, I believe that
: to add
: it to explicit cast we have to add it to SQL.

Yes, there is no way to declare field of ANY type, or create such literal. But
As result of discussion, for consistency sake (with SCALAR) it was decided to still allow 
explicit cast to ANY, just as plain noop.

: 
: 2. What this patch actually does? I mean, after this patch I still not able
: to cast values to ANY:
: 
: tarantool> box.execute('SELECT CAST(1 AS ANY);')
: ---
: - null
: - 'At line 1 at or near position 18: keyword ''ANY'' is reserved. Please
: use double
:    quotes if ''ANY'' is an identifier.'
: ...

That's odd - I've seen such behavior when there was no renamed %wildcard ANY and 
Old generated parser code. It was working for me with that old patch, but will check.


: 
: On Tue, May 25, 2021 at 12:01:01PM +0300, Timur Safin wrote:
: > For consistency sake it was decided to provide
: > `CAST(anything TO ANY)` as allowed, but still noop.
: >
: > Fuller explicit conversions table is described in the
: > /doc/rfc/5910-consistent-sql-lua-types.md and here we
: > represent only relevant to ANY.
: >

: 3. I still think that it is better to put RFC name instead of  this relative
: link.

Whatever.

: 
: > Furthermore, there is no direct way in SQL to create literal and
: > expression of ANY type, thus there is no defined CAST from ANY
: > to anything else.
: >

: 4. There is no way to create literal, however you can use space with
: field type
: ANY, right?

Nope, SQL is strict-typed language at runtime. ANY is NoSQL type, and
there should be no way to declare untyped field. But if, by any chance,
we have  received data of `any` type from Lua side, then we should have
some means to at least cast to it expression.

: 
: >               | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
: >               +---+---+---+---+---+---+---+---+---+---+---+---+---+
: >  0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
: >  1.  unsigned |   | . | . | . | . |   |   | . |   |   |   |   | Y |
: >  2.    string |   | . | . | . | . | . | . | . |   |   |   |   | Y |
: >  4.    double |   | . | . | . | . |   |   | . |   |   |   |   | Y |
: >  5.   integer |   | . | . | . | . |   |   | . |   |   |   |   | Y |
: >  6.   boolean |   |   | . |   |   | . |   |   |   |   |   |   | Y |
: >  7. varbinary |   |   | . |   |   |   | . |   |   |   |   |   | Y |
: >  3.    number |   | . | . | . | . |   |   | . |   |   |   |   | Y |
: >  9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
: > 10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
: > 11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
: > 12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
: >  8.    scalar |   | . | . | . | . | . | . | . |   |   |   |   | Y |
: >               +---+---+---+---+---+---+---+---+---+---+---+---+---+
: >
: 5. This strange order again.

This order is from illustrations of the spec. It's too late to redraw
all pictures there, but reordering here is easy. So we need to live 
with this order forever :(

: 
: 6. Again, do you really need this table in commit-message? This time it
: is also
: a lot simpler to just describe this explicit conversion rules using words.

Picture worth thousand words. You tend to prefer wording, many prefer
pictures.

: 
: > * We introduced new token ANY as known lexem to the Lemon parser.
: >   While renamed prior %wildcard to ANYTHING;
: > * Modified explicit conversion rules to allow ANY as _target_ in
: >   conversion pair.
: >
: > Relates to #5910, #6009
: > Part of #4407
: 7. Wrong issue number. Also, I am again not sure that you should include
: "Relates to" here.

Relates will allow us to keep them linked with parent specification.
Even if there will be multiple steps and commits.

: 
: > ---
: >  extra/mkkeywordhash.c          | 3 ++-
: >  src/box/sql/mem.c              | 2 ++
: >  src/box/sql/parse.y            | 3 ++-
: >  test/sql-tap/keyword1.test.lua | 2 +-
: >  4 files changed, 7 insertions(+), 3 deletions(-)
: >
: > diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
: > index 7480c0211..d343cf706 100644
: > --- a/extra/mkkeywordhash.c
: > +++ b/extra/mkkeywordhash.c
: > @@ -60,6 +60,7 @@ static Keyword aKeywordTable[] = {
: >    { "ALL",                    "TK_ALL",         true  },
: >    { "ALTER",                  "TK_ALTER",       true  },
: >    { "ANALYZE",                "TK_STANDARD",    true  },
: > +  { "ANY",                    "TK_ID",          true  },
: 8. Why some types describes as TK_ID, and some as TK_<type name>?
: 
: >    { "AND",                    "TK_AND",         true  },
: >    { "AS",                     "TK_AS",          true  },
: >    { "ASC",                    "TK_ASC",         true  },
: > @@ -178,7 +179,7 @@ static Keyword aKeywordTable[] = {
: >    { "WITH",                   "TK_WITH",        true  },
: >    { "WHEN",                   "TK_WHEN",        true  },
: >    { "WHERE",                  "TK_WHERE",       true  },
: > -  { "ANY",                    "TK_STANDARD",    true  },
: > +  { "ANYTHING",               "TK_STANDARD",    true  },
: 9. Why not name it as TK_WILDCARD to avoid further misunderstanding?
: 
: >    { "ASENSITIVE",             "TK_STANDARD",    true  },
: >    { "BLOB",                   "TK_STANDARD",    true  },
: >    { "BINARY",                 "TK_ID",          true  },
: > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
: > index d40366754..b4f1078ae 100644
: > --- a/src/box/sql/mem.c
: > +++ b/src/box/sql/mem.c
: > @@ -874,6 +874,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type
: type)
: >  		    (mem->flags & MEM_Subtype) != 0)
: >  			return -1;
: >  		return 0;
: > +	case FIELD_TYPE_ANY:
: > +		return 0;
: >  	default:
: >  		break;
: >  	}
: > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
: > index abc363951..e586d0181 100644
: > --- a/src/box/sql/parse.y
: > +++ b/src/box/sql/parse.y
: > @@ -270,7 +270,7 @@ columnlist ::= tcons.
: >    QUERY KEY OFFSET RAISE RELEASE REPLACE RESTRICT
: >    RENAME CTIME_KW IF ENABLE DISABLE
: >    .
: > -%wildcard ANY.
: > +%wildcard ANYTHING.
: >
: >
: >  // And "ids" is an identifer-or-string.
: > @@ -1834,6 +1834,7 @@ typedef(A) ::= SCALAR . { A.type =
: FIELD_TYPE_SCALAR; }
: >  typedef(A) ::= BOOL . { A.type = FIELD_TYPE_BOOLEAN; }
: >  typedef(A) ::= BOOLEAN . { A.type = FIELD_TYPE_BOOLEAN; }
: >  typedef(A) ::= VARBINARY . { A.type = FIELD_TYPE_VARBINARY; }
: > +typedef(A) ::= ANY . { A.type = FIELD_TYPE_ANY; }
: >
: >  /**
: >   * Time-like types are temporary disabled, until they are
: > diff --git a/test/sql-tap/keyword1.test.lua b/test/sql-
: tap/keyword1.test.lua
: > index f9a9c6865..4855f3ebc 100755
: > --- a/test/sql-tap/keyword1.test.lua
: > +++ b/test/sql-tap/keyword1.test.lua
: > @@ -242,7 +242,7 @@ for _, kw in ipairs(bannedkws) do
: >      local query = 'CREATE TABLE '..kw..'(a INT PRIMARY KEY);'
: >      if kw == 'end' or kw == 'match' or kw == 'release' or kw == 'rename'
: or
: >         kw == 'replace' or kw == 'binary' or kw == 'character' or
: > -       kw == 'smallint' then
: > +       kw == 'smallint' or kw == 'any' then
: >          test:do_catchsql_test(
: >          "bannedkw1-"..kw..".1",
: >          query, {
: > --
: > 2.29.2
: >


: 10. Why there is no tests?

All newly introduced modifications extensively covered in e_casts.test.lua
test you see the other patch. [And I really mean "extensively", because test matrix
there covers hundreds and hundreds of test cases, which is impossible to cover 
with manual result-based test. Regardless how hard you try and how careful you are]

I'll merge all explicit conversion code changes with corresponding part of e_casts.test.lua
if we had to have test in the same commit.

Timur


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 3/3] sql: introduced explicit casts test e_casts.test.lua
  2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
@ 2021-06-02 21:04     ` Timur Safin via Tarantool-patches
  0 siblings, 0 replies; 13+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-02 21:04 UTC (permalink / raw)
  To: 'Mergen Imeev'; +Cc: tarantool-patches

: From: Mergen Imeev <imeevma@tarantool.org>
: Sent: Tuesday, June 1, 2021 5:03 PM
: To: Timur Safin <tsafin@tarantool.org>
: Cc: tarantool-patches@dev.tarantool.org
: Subject: Re: [PATCH 3/3] sql: introduced explicit casts test
: e_casts.test.lua
: 
: Thank you for the patch. See 13 comments below.
: 
: On Tue, May 25, 2021 at 12:01:02PM +0300, Timur Safin wrote:
: > * e_casts.test.lua is generating expressions of `cast(input as output)`
: >   form. And checks whether we expectedly succeed or fail, given the
: >   previously agreed excplicit conversion table from
: >   /doc/rfc/5910-consistent-sql-lua-types.md;
: >
: > * At the moment we skip DECIMALs, UUIDs, ARRAYs and MAPs as not yet
: >   fully supported. Need to be reenabled later;
: >
: > * there is `-verbose` mode one may activate to enable more detailed
: >   reporting.
: 1. How to use this mode? I mean,
: ./test-run.py sql-tap/e_casts.test.lua --verbose
: gives a lot simpler result that I expect from this much of code.

For debugging of various failures I run it directly via script as simple as
that:

20:05 $ ~/bin/sql-tap-run.sh sql-tap/e_casts.test.lua -v | head -40
TAP version 13
1..3
              | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
              +---+---+---+---+---+---+---+---+---+---+---+---+---+
 0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
 1.  unsigned |   | Y | Y | Y | Y |   |   | Y |   |   |   |   | Y |
 2.    string |   | S | Y | S | S | S | Y | S |   |   |   |   | Y |
 4.    double |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
 5.   integer |   | S | Y | Y | Y |   |   | Y |   |   |   |   | Y |
 6.   boolean |   |   | Y |   |   | Y |   |   |   |   |   |   | Y |
 7. varbinary |   |   | Y |   |   |   | Y |   |   |   |   |   | Y |
 3.    number |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
 9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
 8.    scalar |   | S | Y | S | S | S | S | S |   |   |   |   | Y |
              +---+---+---+---+---+---+---+---+---+---+---+---+---+
              | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
              +---+---+---+---+---+---+---+---+---+---+---+---+---+
 0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
 1.  unsigned |   | Y | Y | Y | Y |   |   | Y |   |   |   |   | Y |
 2.    string |   | S | Y | S | S |   | Y | S |   |   |   |   | Y |
 4.    double |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
 5.   integer |   | S | Y | Y | Y |   |   | Y |   |   |   |   | Y |
 6.   boolean |   |   |   |   |   | Y |   |   |   |   |   |   | Y |
 7. varbinary |   |   | Y |   |   |   | Y |   |   |   |   |   | Y |
 3.    number |   | S | Y | Y | S |   |   | Y |   |   |   |   | Y |
 9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
 8.    scalar |   | S | S | S | S | S | S | S |   |   |   |   | S |
              +---+---+---+---+---+---+---+---+---+---+---+---+---+
    # e_casts - check consistency of implicit conversion table
    1..169
    ok - sql-tap/e_casts.test.lua:244 [any,any] nil ~= nil
    ok - sql-tap/e_casts.test.lua:244 [any,unsigned] nil ~= nil
    ok - sql-tap/e_casts.test.lua:244 [any,string] nil ~= nil
    ok - sql-tap/e_casts.test.lua:244 [any,double] nil ~= nil
...

I need to see those tables (explicit and implicit) visualized to make 
sure I have them correctly entered and be compatible with written to RFC.
This output is incompatible (and unnecessary) for TAP mode, that's why it's 
not activated by default.


: 
: 2. What means e_casts? If it is for explicit casts, why not name it
: something
: like "explicit_cast.test.lua".

Because it's not planned to be only about explicit casts :)
[You'll see it's development in the next version of patchset]

And this strange e_* prefix is inherited from the times when I wanted
to resurrect e_expr.test.lua Now, having asked about it, I do realize
it makes no much sense and probably should be removed.

Will rename to sql-tap/casts.test.lua

: 
: 3. As far as I remember, "sometimes" doesn't mean that CAST(1.0 as UNSIGNED)
: can return 1 or can throw an error. It means that there are rules that
: define
: when values of one type can be converted to values of another type. So,
: instead of checking failures for "maybe" we can check that "from" value
: should fail or not according to these rules and use proper test.

Yes, sometimes meant that some part of data might be safely converted, but
others will not. And here we should prepare proper set where some values
are valid for conversion and others are not. (See my answer below)

: 
: Also, using this approach we can try to use randomized "from" values. Just
: thought.

I thought about it some day, but no, randomized data will introduce unnecessary
volatility to testing and will not add any extra value to this particular case.
A well prepared data-set provides here much better results, when we
actually check only compile time reactions, and not run-time.

Though, eventually this approach might be modified for fuzzing of this part
of code, and there randomization will be necessary, but not today.

: 
: 4. Why you test only literals? Nothing indicates that values from spaces
: shouldn't be tested here.

At the moment of writing this patchset I've not yet figured out how to
conveniently deal with generated fields of expected types. Now, for implicit
conversion type I do have easy approach, and will add this case for run-time
explicit conversion of different type expressions.

: 
: >
: > Relates to #5910, #6009
: > Part of #4407
: 5. Wrong issue number. Also, I still think that "relates to" is not
: needed here.

Yup thanks for the catch about 4470! [But I'd still keep relation to
consistent-type spec - that's part of larger work]

: 
: > ---
: >  test/sql-tap/e_casts.test.lua | 355 ++++++++++++++++++++++++++++++++++
: >  1 file changed, 355 insertions(+)
: >  create mode 100755 test/sql-tap/e_casts.test.lua
: >
: > diff --git a/test/sql-tap/e_casts.test.lua b/test/sql-tap/e_casts.test.lua
: > new file mode 100755
: > index 000000000..eafebd5bc
: > --- /dev/null
: > +++ b/test/sql-tap/e_casts.test.lua
: > @@ -0,0 +1,355 @@
: > +#!/usr/bin/env tarantool
: > +local test = require("sqltester")
: 6. What point to use sqltester here is you do not use its functions? You
: can use
: 'tap' instead.

Good point - sqltester is not used here.

: 
: > +test:plan(222)
: > +
: > +local yaml = require("yaml")
: > +local ffi = require("ffi")
: > +
: > +local verbose = 0
: > +
: > +if arg[1] == '-v' or arg[1] == '--verbose' then
: 7. How to use '-v' option?
: 
: > +    verbose = 1
: > +end
: > +
: > +ffi.cdef [[
: > +    enum field_type {
: > +        FIELD_TYPE_ANY = 0,
: > +        FIELD_TYPE_UNSIGNED,
: > +        FIELD_TYPE_STRING,
: > +        FIELD_TYPE_NUMBER,
: > +        FIELD_TYPE_DOUBLE,
: > +        FIELD_TYPE_INTEGER,
: > +        FIELD_TYPE_BOOLEAN,
: > +        FIELD_TYPE_VARBINARY,
: > +        FIELD_TYPE_SCALAR,
: > +        FIELD_TYPE_DECIMAL,
: > +        FIELD_TYPE_UUID,
: > +        FIELD_TYPE_ARRAY,
: > +        FIELD_TYPE_MAP,
: > +        field_type_MAX
: > +    };
: > +]]
: 8. Is there any point to use ffi here? Can't you use something simpler?
: For example:
: local types = {
: 	any		= 0,
: 	unsigned	= 1,
: ...
: 
: Than instead of t_any you can use types.any and so on.

Originally the point was to just copy-paste the definition from Tarantool
code as-is, without any massaging or modifications.

[Ideally it should be extracted from the actual code src/box/field_def.h code. 
And I'll probably do so, eventually, because field_def.h is prepared for such
extraction, and this place is marked with /** \cond public */ 
/** \endcond public */ boundaries, so enum could be easily extracted]

: 
: If you start from 1 instead of 0 you can also use type_names instead of
: proper_order.

At the moment I don't consider that getting rid of ffi.cdef will simplify
anything or provide any extra value. 

: 
: > +
: > +-- Date/time/interval types to be uncommented and used
: > +-- once corresponding box implementation completed
: > +local t_any = ffi.C.FIELD_TYPE_ANY
: > +local t_unsigned = ffi.C.FIELD_TYPE_UNSIGNED
: > +local t_string = ffi.C.FIELD_TYPE_STRING
: > +local t_number = ffi.C.FIELD_TYPE_NUMBER
: > +local t_double = ffi.C.FIELD_TYPE_DOUBLE
: > +local t_integer = ffi.C.FIELD_TYPE_INTEGER
: > +local t_boolean = ffi.C.FIELD_TYPE_BOOLEAN
: > +local t_varbinary = ffi.C.FIELD_TYPE_VARBINARY
: > +local t_scalar = ffi.C.FIELD_TYPE_SCALAR
: > +local t_decimal = ffi.C.FIELD_TYPE_DECIMAL
: > +-- local t_date = -1
: > +-- local t_time = -2
: > +-- local t_timestamp = -3
: > +-- local t_interval = -4
: > +local t_uuid = ffi.C.FIELD_TYPE_UUID
: > +local t_array = ffi.C.FIELD_TYPE_ARRAY
: > +local t_map = ffi.C.FIELD_TYPE_MAP
: > +
: > +local proper_order = {
: > +    t_any,
: > +    t_unsigned,
: > +    t_string,
: > +    t_double,
: > +    t_integer,
: > +    t_boolean,
: > +    t_varbinary,
: > +    t_number,
: > +    t_decimal,
: > +    t_uuid,
: > +    -- t_date,
: > +    -- t_time,
: > +    -- t_timestamp,
: > +    -- t_interval,


: 9. I think it is better to check them for "wrong type name" or remove these
: types.

I don't quite get what is "check for 'wrong type name'"? Could you please elaborate?
(Those unused type will be added in the nearest times, in any case)

: 
: > +    t_array,
: > +    t_map,
: > +    t_scalar,
: > +}
: > +
: > +local type_names = {
: > +    [t_any]       = 'any',
: > +    [t_unsigned]  = 'unsigned',
: > +    [t_string]    = 'string',
: > +    [t_double]    = 'double',
: > +    [t_integer]   = 'integer',
: > +    [t_boolean]   = 'boolean',
: > +    [t_varbinary] = 'varbinary',
: > +    [t_number]    = 'number',
: > +    [t_decimal]   = 'decimal',
: > +    [t_uuid]      = 'uuid',
: > +    -- [t_date]      = 'date',
: > +    -- [t_time]      = 'time',
: > +    -- [t_timestamp] = 'timestamp',
: > +    -- [t_interval]  = 'interval',
: > +    [t_array]     = 'array',
: > +    [t_map]       = 'map',
: > +    [t_scalar]    = 'scalar',
: > +}
: > +
: > +-- not all types implemented/enabled at the moment
: > +-- but we do keep their projected status in the
: > +-- spec table
: > +local enabled_type = {
: > +    [t_any]       = false, -- there is no way in SQL to instantiate ANY
: type expression
: > +    [t_unsigned]  = true,
: > +    [t_string]    = true,
: > +    [t_double]    = true,
: > +    [t_integer]   = true,
: > +    [t_boolean]   = true,
: > +    [t_varbinary] = true,
: > +    [t_number]    = true,
: > +    [t_decimal]   = false,
: > +    [t_uuid]      = false,
: > +    -- [t_date]     = false,
: > +    -- [t_time]     = false,
: > +    -- [t_timestamp]= false,
: > +    -- [t_interval] = False,
: > +    [t_array]     = false,
: > +    [t_map]       = false,
: > +    [t_scalar]    = true,
: > +}
: > +
: > +-- table of _TSV_ (tab separated values)
: > +-- copied from sql-lua-tables-v5.xls // TNT implicit today
: > +local explicit_casts_table_spec = {
: > +    [t_any] =     {"Y", "S", "Y", "S", "S", "S", "S", "S", "S", "S", "S",
: "S", "S"},
: > +    [t_unsigned]= {"Y", "Y", "Y", "Y", "Y", "" , "" , "Y", "Y", "" , "" ,
: "" , "Y"},
: > +    [t_string] =  {"Y", "S", "Y", "S", "S", "S", "Y", "S", "S", "S", "S",
: "S", "Y"},
: > +    [t_double] =  {"Y", "S", "Y", "Y", "S", "" , "" , "Y", "Y", "" , "" ,
: "" , "Y"},
: > +    [t_integer] = {"Y", "S", "Y", "Y", "Y", "" , "" , "Y", "Y", "" , "" ,
: "" , "Y"},
: > +    [t_boolean] = {"Y", "" , "Y", "" , "" , "Y", "" , "" , "" , "" , "" ,
: "" , "Y"},
: > +    [t_varbinary]={"Y", "" , "Y", "N", "" , "" , "Y", "" , "" , "S", "" ,
: "" , "Y"},
: > +    [t_number] =  {"Y", "S", "Y", "Y", "S", "" , "" , "Y", "Y", "" , "" ,
: "" , "Y"},
: > +    [t_decimal] = {"Y", "S", "Y", "S", "S", "" , "" , "Y", "Y", "" , "" ,
: "" , "Y"},
: > +    [t_uuid] =    {"Y", "" , "Y", "" , "" , "" , "Y", "" , "" , "Y", "" ,
: "" , "Y"},
: > +    [t_array] =   {"Y", "N", "Y", "N", "N", "N", "" , "" , "" , "" , "Y",
: "" , "N"},
: > +    [t_map] =     {"Y", "N", "Y", "N", "N", "N", "" , "" , "" , "" , "" ,
: "Y", "N"},
: > +    [t_scalar] =  {"Y", "S", "Y", "S", "S", "S", "S", "S", "S", "S", "" ,
: "" , "Y"},
: > +}
: 10. Why there is 'N' and '' both in table? Can't be make it use only one
: of these?

Because this is actual copy-paste from original xls on which RFC was based on.
:)

[See attachments in https://github.com/tsafin/tarantool/wiki/SQL-Lua-Consistent-Type-Specification]

: 
: > +
: > +-- local extra_checks = false
: > +local explicit_casts = {}
: > +-- local implicit_casts = {}
: > +
: > +-- implicit conversion table is considered consistent if
: > +-- it's sort of symmetric against diagonal
: > +-- (not necessary that always/sometimes are matching
: > +-- but at least something should be presented)
: > +
: > +--[[ local function check_table_consistency(table)
: > +    for _, i in ipairs(proper_order) do
: > +        local string = ''
: > +        for _, j in ipairs(proper_order) do
: > +            print(i, j)
: > +            -- local item = implicit_casts[i][j]
: > +            -- string = string .. (xlat[item] or ' ')
: > +        end
: > +        print(string)
: > +    end
: > +end
: > +]]
: > +
: > +    -- if there is enabled extra checks then check ocnsistency of input
: tables
: > +    -- just to make sure their sanity
: > +--[[     if extra_checks then
: > +        check_table_consistency()
: > +    end
: > + ]]
: > +


: 11. Please drop commented part of code.

This is cleaned up in the current version, including implicit casts support, 
see the next version of patchset.

: 
: > +local c_no = 0
: > +local c_maybe = 1
: > +local c_yes = 2
: > +
: > +local function normalize_cast(v)
: > +    local xlat =  {
: > +        ['Y'] = c_yes,
: > +        ['S'] = c_maybe,
: > +        ['N'] = c_no,
: > +    }
: > +    return xlat[v ~= nil and v or 'N']
: > +end
: > +
: > +local function human_cast(v)
: > +    local xlat = {
: > +        [c_yes] = 'Y',
: > +        [c_maybe] = 'S',
: > +        [c_no] = ' '
: > +    }
: > +    return xlat[v ~= nil and v or c_no]
: > +end
: > +
: > +local function load_casts_spec(spec_table)
: > +    local casts = {}
: > +    for i, t_from  in ipairs(proper_order) do
: > +        local row = spec_table[t_from]
: > +        casts[t_from] = {}
: > +        for j, t_to  in ipairs(proper_order) do
: > +            if enabled_type[t_from] and enabled_type[t_to] then
: > +                casts[t_from][t_to] =
: normalize_cast(spec_table[t_from][j])
: > +            end
: > +        end
: > +    end
: > +    -- if there is enabled extra checks then check ocnsistency of input
: tables
: > +    -- just to make sure their sanity
: > +--[[     if extra_checks then
: > +        check_table_consistency()
: > +    end ]]
: > +
: > +    return casts
: > +end
: > +
: > +explicit_casts = load_casts_spec(explicit_casts_table_spec)
: > +
: > +if verbose > 0 then
: > +    local function show_casts_table(table)


: 12. I believe word 'table' is reserved for in Lua. Not sure that it is good
: idea to overwrite this reserved word.

Apparently it's not reserved and we could reuse it as identifier in
local context. But thanks for reminding - that would be very confusing 
the moment I'd need to use table.insert or something. (Strange that luacheck
which is always activated in my vscode editor did not complain here a lot)

: 
: > +        local max_len = #"12. varbinary" + 1
: > +
: > +        -- show banner
: > +        local col_names = ''
: > +        for i, t_val in ipairs(proper_order) do
: > +            col_names = col_names .. string.format("%2d |", t_val)
: > +        end
: > +        col_names = string.sub(col_names, 1, #col_names-1)
: > +        print(string.format("%"..max_len.."s|%s|", "", col_names))
: > +        -- show splitter
: > +        local banner = '+---+---+---+---+---+---+---+---+---+---+---+---
: +---+'
: > +        print(string.format("%"..max_len.."s%s", "", banner))
: > +
: > +        for i, from in ipairs(proper_order) do
: > +            local line = ''
: > +            for j, to in ipairs(proper_order) do
: > +                line = line .. string.format("%2s |",
: human_cast(table[from][to]))
: > +            end
: > +            line = string.sub(line, 1, #line-1)
: > +            local s = string.format("%2d.%10s |%s|", from,
: type_names[from], line)
: > +            print(s)
: > +        end
: > +        print(string.format("%"..max_len.."s%s", "", banner))
: > +    end
: > +
: > +    show_casts_table(explicit_casts)
: > +end
: > +
: > +local function merge_tables(...)
: > +    local n = select('#', ...)
: > +    local tables = {...}
: > +    local result = {}
: > +
: > +    for i=1,n do
: > +        local t = tables[i]
: > +        --print(yaml.encode(t))
: > +        assert(type(tables[i]) == 'table')
: > +        for j,v in pairs(t) do
: > +            table.insert(result, v)
: > +        end
: > +    end
: > +    return result
: > +end
: > +
: > +local gen_type_samples = {
: > +        [t_unsigned]  = {"0", "1", "2"},
: > +        [t_integer]   = {"-678", "-1", "0", "1", "2", "3", "678"},
: > +        [t_double]    = {"0.0", "123.4", "-567.8"},
: > +        [t_string]    = {"''", "'1'", "'abc'", "'def'", "'TRUE'",
: "'FALSE'"},
: > +        [t_boolean]   = {"false", "true", "null"},
: > +        [t_varbinary] = {"X'312E3233'", "X'2D392E3837'",
: "X'302E30303031'"},
: > +}
: > +
: > +local function gen_type_exprs(type)
: > +    if type == t_number then
: > +        return merge_tables(gen_type_samples[t_unsigned],
: > +                            gen_type_samples[t_integer],
: > +                            gen_type_samples[t_double])
: > +    end
: > +    if type == t_scalar then
: > +        return merge_tables(gen_type_samples[t_unsigned],
: > +                            gen_type_samples[t_integer],
: > +                            gen_type_samples[t_double],
: > +                            gen_type_samples[t_string],
: > +                            gen_type_samples[t_boolean],
: > +                            gen_type_samples[t_varbinary])
: > +    end
: > +    return gen_type_samples[type] or {}
: > +end

: 13. Do you need this function? You can just expand gen_type_samples
: using its
: code.

Function allows to avoid copy-paste for values from meta types such as
Number and scalar. 

: 
: This comment can be implemented for some other functions, for example
: the next
: one. I think it will make code simpler to understand.

[If I correctly understood your sentence] Nope, copy paste will make 
code much more verbose, and less observable.

: 
: > +
: > +local function gen_sql_cast_from_to(t_from, t_to)
: > +    local queries = {}
: > +    local from_exprs = gen_type_exprs(t_from)
: > +    local to_typename = type_names[t_to]
: > +    for _, expr in pairs(from_exprs) do
: > +        local query = string.format([[ select cast(%s as %s); ]], expr,
: to_typename)
: > +        table.insert(queries, query)
: > +    end
: > +    return queries
: > +end
: > +
: > +local function catch_query(query)
: > +    local result = {pcall(box.execute, query)}
: > +
: > +    if not result[1] or result[3] ~= nil then
: > +        return false, result[3]
: > +    end
: > +    return true, result[2]
: > +end
: > +
: > +local function label_for(from, to, query)
: > +    local parent_frame = debug.getinfo(2, "nSl")
: > +    local filename = parent_frame.source:sub(1,1) == "@" and
: parent_frame.source:sub(2)
: > +    local line = parent_frame.currentline
: > +    return string.format("%s+%d:[%s,%s] %s", filename, line,
: > +                         type_names[from], type_names[to], query)
: > +end
: > +
: > +for i, from in ipairs(proper_order) do
: > +    for j, to in ipairs(proper_order) do
: > +        -- skip ANY, DECIMAL, UUID, etc.
: > +        if enabled_type[from] and enabled_type[to] then
: > +            local cell = explicit_casts[from][to]
: > +            local gen = gen_sql_cast_from_to(from, to)
: > +            local failures = {}
: > +            local successes = {}
: > +            local castable = false
: > +            local expected = explicit_casts[from][to]
: > +            if verbose > 0 then
: > +                print(expected, yaml.encode(gen))
: > +            end
: > +            for i, v in pairs(gen) do
: > +                local ok, result
: > +                ok, result = catch_query(v)
: > +                if verbose > 0 then
: > +                    print(string.format("ok = %s, result = %s, query =
: %s",
: > +                         ok, result, v))
: > +
: > +                end
: > +                -- print(v, 'ok'..yaml.encode(ok),
: 'result'..yaml.encode(result))
: > +                if expected == c_yes then
: > +                    test:ok(true == ok, label_for(from, to, v))
: > +                elseif expected == c_no then
: > +                    test:ok(false == ok, label_for(from, to, v))
: > +                else
: > +                -- we can't report immediately for c_maybe because some
: > +                -- cases allowed to fail, so postpone decision
: > +                    if ok then
: > +                        castable = true
: > +                        table.insert(successes, {result, v})
: > +                    else
: > +                        table.insert(failures, {result, v})
: > +                    end
: > +                end
: > +            end
: > +
: > +            -- ok, we aggregated stats for c_maybe mode - check it now
: > +            if expected == c_maybe then
: > +                    test:ok(castable, label_for(from, to, #gen and gen[1]
: or ''),
: > +                            failures)
: > +            end
: > +        end
: > +    end
: > +end
: > +
: > +
: > +test:finish_test()
: > --
: > 2.29.2
: >


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables
  2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
  2021-06-02 21:03     ` Timur Safin via Tarantool-patches
@ 2021-06-02 21:10     ` Timur Safin via Tarantool-patches
  1 sibling, 0 replies; 13+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-02 21:10 UTC (permalink / raw)
  To: 'Mergen Imeev'; +Cc: tarantool-patches

Pushed "Send" button too early. Continue...

: From: Timur Safin <tsafin@tarantool.org>
: Sent: Thursday, June 3, 2021 12:04 AM
: To: 'Mergen Imeev' <imeevma@tarantool.org>
: Cc: 'tarantool-patches@dev.tarantool.org' <tarantool-
: patches@dev.tarantool.org>
: Subject: RE: [PATCH 1/3] sql: fixes for boolean expressions in explicit
: converstion tables
: 
...

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

Good point. I've updated this result file via --update-result before 
I realized the more accurate way in the different 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.

Are you serious here? Updates of older tests and extensive matrix in e_casts.test.lua 
are tests for this patch. There is no point to create any kind of result-based test,
neither manual tap test, at the moment when we have full conversion table covered in
e_casts.test.lua.

Timur


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-06-02 21:10 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-25  9:00 [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Timur Safin via Tarantool-patches
2021-05-25  9:01 ` [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables Timur Safin via Tarantool-patches
2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
2021-06-02 21:03     ` Timur Safin via Tarantool-patches
2021-06-02 21:10     ` Timur Safin via Tarantool-patches
2021-05-25  9:01 ` [Tarantool-patches] [PATCH 2/3] sql: enabled ANY as target for explicit conversions Timur Safin via Tarantool-patches
2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
2021-06-02 21:04     ` Timur Safin via Tarantool-patches
2021-05-25  9:01 ` [Tarantool-patches] [PATCH 3/3] sql: introduced explicit casts test e_casts.test.lua Timur Safin via Tarantool-patches
2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
2021-06-02 21:04     ` Timur Safin via Tarantool-patches
2021-06-01 14:02 ` [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Mergen Imeev via Tarantool-patches
2021-06-02 21:04   ` Timur Safin via Tarantool-patches

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git