* [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables
@ 2021-06-11 7:48 Timur Safin via Tarantool-patches
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines Timur Safin via Tarantool-patches
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-11 7:48 UTC (permalink / raw)
To: imeevma; +Cc: tarantool-patches
Issues:
https://github.com/tarantool/tarantool/issues/4470
https://github.com/tarantool/tarantool/issues/6010
Branch: https://github.com/tarantool/tarantool/tree/tsafin/gh-4470-explicit-implicit-conversions-V2
This patchset containes of 2 major part and 1 bonus. Major parts
are changes in explicit and implicit conversion tables, with all
accompanying tests we have so far (and 1 extensive test introduced).
The bonus part is for improvements in the testing system tap.lua
which became apparent during debugging of this patches.
Relates to #5910, #6009
Part of #4470
Fixes #6010
Explicit table
==============
As we discovered harder way, there is no need to introduce so much changes
to the current explicit conversion table, because it's mostly compliant
already:
- most of changes we had to do was about `BOOLEAN` conversions, which are
stricter now than before;
- there are addition of to `ANY` conversion, which we have decided to make
behaving similar to `SCALAR` conversions;
Those changes could be visualized via this table:
| 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
+---+---+---+---+---+---+---+---+---+---+---+---+---+
0. any | | | | | | | | | | | | | |
1. unsigned | | . | . | . | . | - | | . | | | | | Y |
2. string | | . | . | . | . | S | . | . | | | | | Y |
4. double | | . | . | . | . | - | | . | | | | | Y |
5. integer | | . | . | . | . | - | | . | | | | | Y |
6. boolean | | - | Y | - | - | Y | | | | | | | Y |
7. varbinary | | | . | | | - | . | | | | | | Y |
3. number | | . | . | . | . | - | | . | | | | | Y |
9. decimal | | | | | | | | | | | | | |
10. uuid | | | | | | | | | | | | | |
11. array | | | | | | | | | | | | | |
12. map | | | | | | | | | | | | | |
8. scalar | | . | . | . | . | S | . | . | | | | | Y |
+---+---+---+---+---+---+---+---+---+---+---+---+---+
This table above represent all possible combinations of explicit cast
from type (row) to another type (column). Numbering of types might look
confusing (it's not sequential) but the order is what we see in the
consistency specification RFC, and we should live with that. Values are
actual enum values from the code. Unfortunately, changing of order is
almost impossible because of massive rework for the spec it would require.
How to interpret this table, e.g.:
- for the explicit cast from `BOOLEAN` (6) to `BOOLEAN` (6) we should
always allow cast (and make it noop), thus intersection is `Y` (yes);
- `STRING` (2) to `BOOLEAN` (6) may work sometimes (if string represents
TRUE or FALSE literals), but may fail otherwise, thus there is `S` (sometimes);
- We have prohibited majority of ther directions for `BOOLEAN` thus there are
`-` (minus) in such cells, i.e. `BOOLEAN` (6) to `INTEGER` (5);
- Untouched entries in the table marked with '.'. Assumption is -
we already have correct conversion rule here activated well before;
- Worth to mention that empty cell means that this conversion
direction is prohibited.
For obvious reasons those chnaged conversion rules made us modify
expected results for many tests - they are fixed.
But to cover all possible conversion combinations (minus those, not yet
implemented in SQL types like DECIMAL, ARRAY and MAP) we have created
special test which check _all combinations of CASTs()_ and verifies
results against table rules defined in the RFC.
There is e_casts.test.lua created for that purpose.
Implicit table
==============
Similarly to explicit conversion rules we have update implicit conversion
rules, and this is 2nd part of patchset.
We will not draw changed values of implicit conversion table (please see
it's final state in the consistemcy types RFC), but verbally we have
modified following directions of conversions:
- string to/from double
- double to/from integer
- varbinary to/from string
In addition to modifciation of all relevant test results, we have also
extended e_casts.test.lua introduced above for checking of all possible
directions of implicit conversions.
Approach to check it though is different than for explicit cast, we do
not use simple expression with implicit cast activated (like we might be
using `CAST()` for explicit casts), but rather we insert value of original
type into columns of expected target type.
Relates to #5910, #6009
Closes #4470
Bonus - better error reporting in tap tests
===========================================
Also, during debugging of explicit/implicitit conversions test it was
discovered the harder way, that TAP infrastructure does not report correctly
origin source line of and error, but rather report :0 line as location.
This has been fixed.
Closes #6134
Timur Safin (3):
test: corrected reported error lines
sql: updated explicit conversion table
sql: updated implicit conversion table
extra/mkkeywordhash.c | 3 +-
src/box/sql/mem.c | 188 +++--------
src/box/sql/mem.h | 6 -
src/box/sql/parse.y | 9 +-
src/box/sql/util.c | 3 +-
src/box/sql/vdbe.c | 8 +-
src/box/sql/vdbeaux.c | 2 +-
src/lua/tap.lua | 2 +-
test/sql-tap/cse.test.lua | 12 +-
test/sql-tap/e_casts.test.lua | 474 +++++++++++++++++++++++++++
test/sql-tap/e_select1.test.lua | 2 +-
test/sql-tap/in1.test.lua | 8 +-
test/sql-tap/in4.test.lua | 19 +-
test/sql-tap/misc3.test.lua | 2 +-
test/sql-tap/numcast.test.lua | 2 +-
test/sql-tap/tkt-9a8b09f8e6.test.lua | 40 +--
test/sql/boolean.result | 38 +--
test/sql/types.result | 132 ++++----
18 files changed, 654 insertions(+), 296 deletions(-)
create mode 100755 test/sql-tap/e_casts.test.lua
--
2.29.2
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines
2021-06-11 7:48 [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Timur Safin via Tarantool-patches
@ 2021-06-11 7:48 ` Timur Safin via Tarantool-patches
2021-06-20 18:57 ` Igor Munkin via Tarantool-patches
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 2/3] sql: updated explicit conversion table Timur Safin via Tarantool-patches
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-11 7:48 UTC (permalink / raw)
To: alexander.turenko; +Cc: tarantool-patches
It always was a problem that reported source line was not
pointing to the actual callee line number, but rather to
the start of file, i.e. we have seen:
```
[001] sql-tap/tkt-9a8b09f8e6.test.lua memtx
[001] not ok 22 - 4.3 #
[001] Traceback:
[001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:123>
[001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:0>
[001]
[001] not ok 23 - 4.4 #
[001] Traceback:
[001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:123>
[001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:0>
```
(see the :0 part)
Instead of correct line numbers:
```
[001] sql-tap/tkt-9a8b09f8e6.test.lua memtx
[001] not ok 22 - 4.3 #
[001] Traceback:
[001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:142>
[001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:242>
[001]
[001] not ok 23 - 4.4 #
[001] Traceback:
[001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:142>
[001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:252>
```
The problem was due to `.linedefined` used, instead of source line in `.currentline`.
Closes #6134
---
src/lua/tap.lua | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lua/tap.lua b/src/lua/tap.lua
index 346724d84..77fd8d096 100644
--- a/src/lua/tap.lua
+++ b/src/lua/tap.lua
@@ -23,7 +23,7 @@ local function traceback(level)
local frame = {
source = info.source;
src = info.short_src;
- line = info.linedefined or 0;
+ line = info.currentline or info.linedefined or 0;
what = info.what;
name = info.name;
namewhat = info.namewhat;
--
2.29.2
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH v2 2/3] sql: updated explicit conversion table
2021-06-11 7:48 [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Timur Safin via Tarantool-patches
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines Timur Safin via Tarantool-patches
@ 2021-06-11 7:48 ` Timur Safin via Tarantool-patches
2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 3/3] sql: updated implicit " Timur Safin via Tarantool-patches
2021-06-20 18:52 ` [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Mergen Imeev via Tarantool-patches
3 siblings, 1 reply; 18+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-11 7:48 UTC (permalink / raw)
To: imeevma; +Cc: tarantool-patches
* fixes for `BOOLEAN` expressions in explicit converstion tables
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 used to be allowed before.
We updated all relevant tests we already have for some combinations.
* introduce `ANY` as target for explicit conversions
For consistency sake it was decided to provide
`CAST(anything TO ANY)` as allowed, but still noop.
Note, that there is no direct way in SQL to create literal and expression
of type `ANY`, thus there is no defined CAST from ANY to anything
else, only to `ANY`.
* We had to rename %wildcard token to ANYTHING, since we needed
token ANY for real life usage.
* As a byproduct, we fixed #6010 to make cast to unsigned behaving reasonably
* To make sure that all consistency checks are systematic, we have introduced
special test for checking conversion rules - e_casts.test.lua. This
patch introduces explicit table part:
* e_casts.test.lua is generating expressions of `CAST(input AS output)`
form. All combinations check whether we expectedly succeeded or failed,
given the explicit conversion table from RFC 5910-consistent-sql-lua-types.md;
* At the moment we skip DECIMALs, ARRAYs and MAPs as not yet
fully supported. Need to be revisited later;
* NB! there is `-verbose` mode one may activate to enable more detailed
reporting during debugging.
Relates to #5910, #6009
Part of #4470
Fixes #6010
---
| 3 +-
src/box/sql/mem.c | 81 +++-----
src/box/sql/parse.y | 9 +-
test/sql-tap/cse.test.lua | 12 +-
test/sql-tap/e_casts.test.lua | 340 ++++++++++++++++++++++++++++++++
test/sql-tap/e_select1.test.lua | 2 +-
test/sql-tap/in1.test.lua | 8 +-
test/sql-tap/in4.test.lua | 2 +-
test/sql-tap/misc3.test.lua | 2 +-
test/sql/boolean.result | 38 +---
test/sql/types.result | 28 ++-
11 files changed, 406 insertions(+), 119 deletions(-)
create mode 100755 test/sql-tap/e_casts.test.lua
--git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index 0d998506c..00f65ce8e 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_ANY", true },
{ "AND", "TK_AND", true },
{ "AS", "TK_AS", true },
{ "ASC", "TK_ASC", true },
@@ -179,7 +180,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 6f3bf52e5..8bb6b672c 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -574,17 +574,6 @@ int_to_str0(struct Mem *mem)
return mem_copy_str0(mem, str);
}
-static inline int
-int_to_bool(struct Mem *mem)
-{
- assert((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0);
- mem->u.b = mem->u.i != 0;
- mem->type = MEM_TYPE_BOOL;
- assert(mem->flags == 0);
- mem->field_type = FIELD_TYPE_BOOLEAN;
- return 0;
-}
-
static inline int
str_to_str0(struct Mem *mem)
{
@@ -810,28 +799,6 @@ double_to_str0(struct Mem *mem)
return 0;
}
-static inline int
-double_to_bool(struct Mem *mem)
-{
- assert(mem->type == MEM_TYPE_DOUBLE);
- mem->u.b = mem->u.r != 0.;
- mem->type = MEM_TYPE_BOOL;
- assert(mem->flags == 0);
- mem->field_type = FIELD_TYPE_BOOLEAN;
- return 0;
-}
-
-static inline int
-bool_to_int(struct Mem *mem)
-{
- assert(mem->type == MEM_TYPE_BOOL);
- mem->u.u = (uint64_t)mem->u.b;
- mem->type = MEM_TYPE_UINT;
- assert(mem->flags == 0);
- mem->field_type = FIELD_TYPE_UNSIGNED;
- return 0;
-}
-
static inline int
bool_to_str0(struct Mem *mem)
{
@@ -872,18 +839,37 @@ uuid_to_bin(struct Mem *mem)
return mem_copy_bin(mem, (char *)&mem->u.uuid, UUID_LEN);
}
+int
+mem_to_uint(struct Mem *mem)
+{
+ assert(mem->type < MEM_TYPE_INVALID);
+ if (mem->type == MEM_TYPE_UINT)
+ return 0;
+ if (mem->type == MEM_TYPE_INT) {
+ mem_set_uint(mem, (uint64_t)mem->u.i);
+ return 0;
+ }
+ if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0)
+ return bytes_to_uint(mem);
+ if (mem->type == MEM_TYPE_DOUBLE)
+ return double_to_uint(mem);
+ return -1;
+}
+
int
mem_to_int(struct Mem *mem)
{
assert(mem->type < MEM_TYPE_INVALID);
- if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
+ if (mem->type == MEM_TYPE_INT)
+ return 0;
+ if (mem->type == MEM_TYPE_UINT) {
+ mem_set_int(mem, (int64_t)mem->u.u, false);
return 0;
+ }
if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0)
return bytes_to_int(mem);
if (mem->type == MEM_TYPE_DOUBLE)
return double_to_int(mem);
- if (mem->type == MEM_TYPE_BOOL)
- return bool_to_int(mem);
return -1;
}
@@ -919,8 +905,6 @@ mem_to_number(struct Mem *mem)
assert(mem->type < MEM_TYPE_INVALID);
if (mem_is_num(mem))
return 0;
- if (mem->type == MEM_TYPE_BOOL)
- return bool_to_int(mem);
if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0) {
if (bytes_to_int(mem) == 0)
return 0;
@@ -994,19 +978,7 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
}
switch (type) {
case FIELD_TYPE_UNSIGNED:
- switch (mem->type) {
- case MEM_TYPE_UINT:
- return 0;
- case MEM_TYPE_STR:
- case MEM_TYPE_BIN:
- return bytes_to_uint(mem);
- case MEM_TYPE_DOUBLE:
- return double_to_int(mem);
- case MEM_TYPE_BOOL:
- return bool_to_int(mem);
- default:
- return -1;
- }
+ return mem_to_uint(mem);
case FIELD_TYPE_STRING:
return mem_to_str(mem);
case FIELD_TYPE_DOUBLE:
@@ -1017,13 +989,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
switch (mem->type) {
case MEM_TYPE_BOOL:
return 0;
- case MEM_TYPE_INT:
- case MEM_TYPE_UINT:
- return int_to_bool(mem);
case MEM_TYPE_STR:
return str_to_bool(mem);
- case MEM_TYPE_DOUBLE:
- return double_to_bool(mem);
default:
return -1;
}
@@ -1049,6 +1016,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
if ((mem->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 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 bd041e862..fbff6ffa7 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 UUID
.
-%wildcard ANY.
+%wildcard ANYTHING.
// And "ids" is an identifer-or-string.
@@ -1113,7 +1113,7 @@ expr(A) ::= expr(A) COLLATE id(C). {
A.zEnd = &C.z[C.n];
}
-expr(A) ::= CAST(X) LP expr(E) AS typedef(T) RP(Y). {
+expr(A) ::= CAST(X) LP expr(E) AS cast_typedef(T) RP(Y). {
spanSet(&A,&X,&Y); /*A-overwrites-X*/
A.pExpr = sql_expr_new_dequoted(pParse->db, TK_CAST, NULL);
if (A.pExpr == NULL) {
@@ -1887,3 +1887,8 @@ number_typedef(A) ::= UNSIGNED . { A.type = FIELD_TYPE_UNSIGNED; }
* (void) C;
*}
*/
+
+// cast_typedef is almost typedef but with ANY type enabled
+%type cast_typedef { struct type_def }
+cast_typedef(A) ::= typedef(T) . { A = T; }
+cast_typedef(A) ::= ANY . { A.type = FIELD_TYPE_ANY; }
diff --git a/test/sql-tap/cse.test.lua b/test/sql-tap/cse.test.lua
index e09f955a0..932f83545 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_casts.test.lua b/test/sql-tap/e_casts.test.lua
new file mode 100755
index 000000000..32d7e8e0c
--- /dev/null
+++ b/test/sql-tap/e_casts.test.lua
@@ -0,0 +1,340 @@
+#!/usr/bin/env tarantool
+local tap = require("tap")
+local test = tap.test("errno")
+
+test:plan(1)
+
+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] = true,
+ -- [t_date] = false,
+ -- [t_time] = false,
+ -- [t_timestamp]= false,
+ -- [t_interval] = False,
+ [t_array] = false,
+ [t_map] = false,
+ [t_scalar] = true,
+}
+
+-- Enabled types which may be targets for explicit casts
+local enabled_type_cast = table.deepcopy(enabled_type)
+enabled_type_cast[t_any] = true
+
+-- table of _TSV_ (tab separated values)
+-- copied from sql-lua-tables-v5.xls
+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 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, enabled_from, enabled_to)
+ local casts = {}
+ for _, t_from in ipairs(proper_order) do
+ casts[t_from] = {}
+ for j, t_to in ipairs(proper_order) do
+ if enabled_from[t_from] and enabled_to[t_to] then
+ casts[t_from][t_to] = normalize_cast(spec_table[t_from][j])
+ end
+ end
+ end
+ return casts
+end
+
+local function label_for(from, to, title)
+ 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], title)
+end
+
+local function show_casts_table(table)
+ local max_len = #"12. varbinary" + 1
+
+ -- show banner
+ local col_names = ''
+ for _, 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 _, from in ipairs(proper_order) do
+ local line = ''
+ for _, 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
+
+local explicit_casts = load_casts_spec(explicit_casts_table_spec, enabled_type, enabled_type_cast)
+
+if verbose > 0 then
+ 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]
+ assert(type(tables[i]) == 'table')
+ for _, 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'", "'123.4'", "'-1.5'", "'abc'", "'def'",
+ "'TRUE'", "'FALSE'", "'22222222-1111-1111-1111-111111111111'"},
+ [t_boolean] = {"false", "true", "null"},
+ [t_varbinary] = {"X'312E3233'", "X'2D392E3837'", "X'302E30303031'",
+ "x'22222222111111111111111111111111'"},
+ [t_uuid] = {'uuid()'},
+}
+
+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
+
+-- explicit
+local function gen_explicit_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
+ table.insert(queries,
+ string.format([[ select cast(%s as %s); ]], expr, to_typename))
+ 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
+
+-- 1. Check explicit casts table
+local function test_check_explicit_casts(test)
+ -- checking validity of all `CAST(from AS to)` combinations
+ test:plan(322)
+ for _, from in ipairs(proper_order) do
+ for _, to in ipairs(proper_order) do
+ -- skip ANY, DECIMAL, UUID, etc.
+ if enabled_type[from] and enabled_type_cast[to] then
+ local gen = gen_explicit_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 _, v in pairs(gen) do
+ local ok, result
+ ok, result = catch_query(v)
+
+ if verbose > 0 then
+ print(string.format("V> ok = %s, result = %s, query = %s",
+ ok, result, v))
+ end
+
+ local title = string.format("%s => %s", v, human_cast(expected))
+ if expected == c_yes then
+ test:ok(true == ok, label_for(from, to, title))
+ elseif expected == c_no then
+ test:ok(false == ok, label_for(from, to, title))
+ 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
+ local title = string.format("%s => %s",
+ #gen and gen[1]..'...' or '',
+ human_cast(expected))
+ test:ok(castable, label_for(from, to, title),
+ failures)
+ end
+ end
+ end
+ end
+end
+
+test:test("e_casts - check explicit casts", test_check_explicit_casts)
+
+test:check()
+os.exit()
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..da3c07768 100755
--- a/test/sql-tap/in1.test.lua
+++ b/test/sql-tap/in1.test.lua
@@ -100,7 +100,7 @@ test:do_execsql_test(
test:do_execsql_test(
"in-1.7",
[[
- SELECT a+ 100*CAST((a BETWEEN 1 and 3) AS INTEGER) FROM t1 ORDER BY b
+ SELECT a+ 100*(CASE WHEN (a BETWEEN 1 and 3) THEN 1 ELSE 0 END) FROM t1 ORDER BY b
]], {
-- <in-1.7>
101, 102, 103, 4, 5, 6, 7, 8, 9, 10
@@ -157,7 +157,7 @@ test:do_execsql_test(
test:do_execsql_test(
"in-2.5",
[[
- SELECT a+100*(CAST(b IN (8,16,24) AS INTEGER)) FROM t1 ORDER BY b
+ SELECT a+100*(CASE WHEN b IN (8,16,24) THEN 1 ELSE 0 END) FROM t1 ORDER BY b
]], {
-- <in-2.5>
1, 2, 103, 104, 5, 6, 7, 8, 9, 10
@@ -207,7 +207,7 @@ test:do_execsql_test(
test:do_execsql_test(
"in-2.10",
[[
- SELECT a FROM t1 WHERE LEAST(0, CAST(b IN (a,30) AS INT)) <> 0
+ SELECT a FROM t1 WHERE LEAST(0, CASE WHEN (b IN (a,30)) THEN 1 ELSE 0 END) <> 0
]], {
-- <in-2.10>
@@ -253,7 +253,7 @@ test:do_execsql_test(
test:do_execsql_test(
"in-3.3",
[[
- SELECT a + 100*(CAST(b IN (SELECT b FROM t1 WHERE a<5) AS INTEGER)) FROM t1 ORDER BY b
+ SELECT a + 100*(CASE WHEN (b IN (SELECT b FROM t1 WHERE a<5)) THEN 1 ELSE 0 END) FROM t1 ORDER BY b
]], {
-- <in-3.3>
101, 102, 103, 104, 5, 6, 7, 8, 9, 10
diff --git a/test/sql-tap/in4.test.lua b/test/sql-tap/in4.test.lua
index 8442944b9..588daefec 100755
--- a/test/sql-tap/in4.test.lua
+++ b/test/sql-tap/in4.test.lua
@@ -133,7 +133,7 @@ test:do_execsql_test(
SELECT b FROM t2 WHERE a IN (1.0, 2.1)
]], {
-- <in4-2.6>
- "one"
+ "one", "two"
-- </in4-2.6>
})
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..90a8bc5ec 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);")
---
@@ -1084,8 +1081,11 @@ box.execute("SELECT CAST(123 AS UNSIGNED);")
...
box.execute("SELECT CAST(-123 AS UNSIGNED);")
---
-- null
-- 'Type mismatch: can not convert -123 to unsigned'
+- metadata:
+ - name: COLUMN_1
+ type: unsigned
+ rows:
+ - [18446744073709551493]
...
box.execute("SELECT CAST(1.5 AS UNSIGNED);")
---
@@ -1097,19 +1097,13 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
...
box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
---
-- metadata:
- - name: COLUMN_1
- type: unsigned
- rows:
- - [-1]
+- null
+- 'Type mismatch: can not convert -1.5 to 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] 18+ messages in thread
* [Tarantool-patches] [PATCH v2 3/3] sql: updated implicit conversion table
2021-06-11 7:48 [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Timur Safin via Tarantool-patches
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines Timur Safin via Tarantool-patches
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 2/3] sql: updated explicit conversion table Timur Safin via Tarantool-patches
@ 2021-06-11 7:48 ` Timur Safin via Tarantool-patches
2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches
2021-06-20 18:52 ` [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Mergen Imeev via Tarantool-patches
3 siblings, 1 reply; 18+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-11 7:48 UTC (permalink / raw)
To: imeevma; +Cc: tarantool-patches
* Changed implicit casts table according to consistent types RFC
* fixed following directions for implicit conversions
- string to double
- double to integer
- varbinary to string
* got rid of mem_cast_implicit_old() as unnecessary, use always
the updated mem_cast_implicit()
* in addition to update of all relevant tests there is extended
e_casts.test.lua used for checking of implicit conversion rules:
* there is table information sanity check - e_casts.test.lua checks
that implicit table content is sane, i.e. it's sort of symmetric
for all defined combinations [from, to]
* for the test we use inserts in specially crafted table as checks
availability of implicit conversions between type values
* Temporary TCASTS table used with sequence primary key
* All fields of a form `{name = "unsigned", type = "unsigned", is_nullable = true}`
used for all enabled types
Relates to #5910, #6009
Closes #4470
* Additionally we have fixed unexpected results for sum of implicitly
converted numbers
Fixing unexpected results after implicit conversion to
the signed or unsigned integer from well-formed string.
```
tarantool> box.execute([[select '1' + '2';]])
---
- metadata:
- name: COLUMN_1
type: scalar
rows:
- [3]
...
tarantool> box.execute([[select '1' + '-2';]])
---
- metadata:
- name: COLUMN_1
type: scalar
rows:
- [18446744073709551615]
...
tarantool> box.execute([[select '-1' + '-2';]])
---
- null
- 'Failed to execute SQL statement: integer is overflowed'
...
```
Fixes #5756
---
src/box/sql/mem.c | 107 ++++-----------------
src/box/sql/mem.h | 6 --
src/box/sql/util.c | 3 +-
src/box/sql/vdbe.c | 8 +-
src/box/sql/vdbeaux.c | 2 +-
test/sql-tap/e_casts.test.lua | 136 ++++++++++++++++++++++++++-
test/sql-tap/in4.test.lua | 17 +++-
test/sql-tap/numcast.test.lua | 2 +-
test/sql-tap/tkt-9a8b09f8e6.test.lua | 40 ++++----
test/sql/types.result | 104 ++++++++++----------
10 files changed, 248 insertions(+), 177 deletions(-)
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 8bb6b672c..d6b114a81 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -1033,95 +1033,24 @@ mem_cast_implicit(struct Mem *mem, enum field_type type)
}
switch (type) {
case FIELD_TYPE_UNSIGNED:
- if (mem->type == MEM_TYPE_UINT)
- return 0;
- if (mem->type == MEM_TYPE_DOUBLE)
- return double_to_uint(mem);
- return -1;
- case FIELD_TYPE_STRING:
- if (mem->type == MEM_TYPE_STR)
- return 0;
- if (mem->type == MEM_TYPE_UUID)
- return uuid_to_str0(mem);
- return -1;
- case FIELD_TYPE_DOUBLE:
- if (mem->type == MEM_TYPE_DOUBLE)
- return 0;
- if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
- return int_to_double(mem);
- return -1;
- case FIELD_TYPE_INTEGER:
if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
return 0;
- if (mem->type == MEM_TYPE_DOUBLE)
- return double_to_int(mem);
- return -1;
- case FIELD_TYPE_BOOLEAN:
- if (mem->type == MEM_TYPE_BOOL)
- return 0;
- return -1;
- case FIELD_TYPE_VARBINARY:
- if ((mem->type & (MEM_TYPE_BIN | MEM_TYPE_MAP |
- MEM_TYPE_ARRAY)) != 0)
- return 0;
- if (mem->type == MEM_TYPE_UUID)
- return uuid_to_bin(mem);
- return -1;
- case FIELD_TYPE_NUMBER:
- if (mem_is_num(mem))
- return 0;
- return -1;
- case FIELD_TYPE_MAP:
- if (mem->type == MEM_TYPE_MAP)
- return 0;
- return -1;
- case FIELD_TYPE_ARRAY:
- if (mem->type == MEM_TYPE_ARRAY)
- return 0;
- return -1;
- case FIELD_TYPE_SCALAR:
- if ((mem->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 0)
- return -1;
- return 0;
- case FIELD_TYPE_UUID:
- if (mem->type == MEM_TYPE_UUID)
- return 0;
- if (mem->type == MEM_TYPE_STR)
- return str_to_uuid(mem);
- if (mem->type == MEM_TYPE_BIN)
- return bin_to_uuid(mem);
- return -1;
- case FIELD_TYPE_ANY:
- return 0;
- default:
- break;
- }
- return -1;
-}
-
-int
-mem_cast_implicit_old(struct Mem *mem, enum field_type type)
-{
- if (mem->type == MEM_TYPE_NULL)
- return 0;
- switch (type) {
- case FIELD_TYPE_UNSIGNED:
- if (mem->type == MEM_TYPE_UINT)
- return 0;
if (mem->type == MEM_TYPE_DOUBLE)
return double_to_uint_precise(mem);
if (mem->type == MEM_TYPE_STR)
return bytes_to_uint(mem);
return -1;
case FIELD_TYPE_STRING:
- if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0)
+ if (mem->type == MEM_TYPE_STR)
return 0;
+ if (mem->type == MEM_TYPE_UUID)
+ return uuid_to_str0(mem);
if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
return int_to_str0(mem);
if (mem->type == MEM_TYPE_DOUBLE)
return double_to_str0(mem);
- if (mem->type == MEM_TYPE_UUID)
- return uuid_to_str0(mem);
+ if (mem->type == MEM_TYPE_BIN)
+ return bin_to_str(mem);
return -1;
case FIELD_TYPE_DOUBLE:
if (mem->type == MEM_TYPE_DOUBLE)
@@ -1129,25 +1058,28 @@ mem_cast_implicit_old(struct Mem *mem, enum field_type type)
if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
return int_to_double(mem);
if (mem->type == MEM_TYPE_STR)
- return bin_to_str(mem);
+ return bytes_to_double(mem);
return -1;
case FIELD_TYPE_INTEGER:
if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
return 0;
- if (mem->type == MEM_TYPE_STR)
- return bytes_to_int(mem);
if (mem->type == MEM_TYPE_DOUBLE)
return double_to_int_precise(mem);
+ if (mem->type == MEM_TYPE_STR)
+ return bytes_to_int(mem);
return -1;
case FIELD_TYPE_BOOLEAN:
if (mem->type == MEM_TYPE_BOOL)
return 0;
return -1;
case FIELD_TYPE_VARBINARY:
- if (mem->type == MEM_TYPE_BIN)
+ if ((mem->type & (MEM_TYPE_BIN | MEM_TYPE_MAP |
+ MEM_TYPE_ARRAY)) != 0)
return 0;
if (mem->type == MEM_TYPE_UUID)
return uuid_to_bin(mem);
+ if (mem->type == MEM_TYPE_STR)
+ return str_to_bin(mem);
return -1;
case FIELD_TYPE_NUMBER:
if (mem_is_num(mem))
@@ -1156,11 +1088,11 @@ mem_cast_implicit_old(struct Mem *mem, enum field_type type)
return mem_to_number(mem);
return -1;
case FIELD_TYPE_MAP:
- if (mem->type == MEM_TYPE_MAP)
+ if (mem_is_map(mem))
return 0;
return -1;
case FIELD_TYPE_ARRAY:
- if (mem->type == MEM_TYPE_ARRAY)
+ if (mem_is_array(mem))
return 0;
return -1;
case FIELD_TYPE_SCALAR:
@@ -1175,6 +1107,8 @@ mem_cast_implicit_old(struct Mem *mem, enum field_type type)
if (mem->type == MEM_TYPE_BIN)
return bin_to_uuid(mem);
return -1;
+ case FIELD_TYPE_ANY:
+ return 0;
default:
break;
}
@@ -1460,7 +1394,7 @@ get_number(const struct Mem *mem, struct sql_num *number)
if (mem->type == MEM_TYPE_INT) {
number->i = mem->u.i;
number->type = MEM_TYPE_INT;
- number->is_neg = true;
+ number->is_neg = mem->u.i < 0;
return 0;
}
if (mem->type == MEM_TYPE_UINT) {
@@ -1473,13 +1407,6 @@ get_number(const struct Mem *mem, struct sql_num *number)
return -1;
if (sql_atoi64(mem->z, &number->i, &number->is_neg, mem->n) == 0) {
number->type = number->is_neg ? MEM_TYPE_INT : MEM_TYPE_UINT;
- /*
- * The next line should be removed along with the is_neg field
- * of struct sql_num. The integer type tells us about the sign.
- * However, if it is removed, the behavior of arithmetic
- * operations will change.
- */
- number->is_neg = false;
return 0;
}
if (sqlAtoF(mem->z, &number->d, mem->n) != 0) {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index b3cd5c545..706cad43c 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -753,12 +753,6 @@ mem_cast_explicit(struct Mem *mem, enum field_type type);
int
mem_cast_implicit(struct Mem *mem, enum field_type type);
-/**
- * Convert the given MEM to given type according to legacy implicit cast rules.
- */
-int
-mem_cast_implicit_old(struct Mem *mem, enum field_type type);
-
/**
* Return value for MEM of INTEGER type. For MEM of all other types convert
* value of the MEM to INTEGER if possible and return converted value. Original
diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index c556b9815..69b1a3937 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -958,9 +958,8 @@ sql_add_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg,
*res = lhs + rhs;
return 0;
}
- *is_res_neg = is_rhs_neg ? (uint64_t)(-rhs) > (uint64_t) lhs :
- (uint64_t)(-lhs) > (uint64_t) rhs;
*res = lhs + rhs;
+ *is_res_neg = *res < 0;
return 0;
}
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 32d02d96e..0dc28e2d6 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1660,7 +1660,7 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
} else if (type == FIELD_TYPE_STRING) {
if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) {
const char *str =
- mem_cast_implicit_old(pIn3, type) != 0 ?
+ mem_cast_implicit(pIn3, type) != 0 ?
mem_str(pIn3) : mem_str(pIn1);
diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
"string");
@@ -1671,7 +1671,7 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
type = FIELD_TYPE_NUMBER;
if (mem_cmp_num(pIn3, pIn1, &res) != 0) {
const char *str =
- mem_cast_implicit_old(pIn3, type) != 0 ?
+ mem_cast_implicit(pIn3, type) != 0 ?
mem_str(pIn3) : mem_str(pIn1);
diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
"numeric");
@@ -1682,7 +1682,7 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
assert(mem_is_str(pIn3) && mem_is_same_type(pIn3, pIn1));
if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) {
const char *str =
- mem_cast_implicit_old(pIn3, type) != 0 ?
+ mem_cast_implicit(pIn3, type) != 0 ?
mem_str(pIn3) : mem_str(pIn1);
diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
"string");
@@ -2218,7 +2218,7 @@ case OP_MakeRecord: {
if (types != NULL) {
pRec = pData0;
do {
- mem_cast_implicit_old(pRec++, *(types++));
+ mem_cast_implicit(pRec++, *(types++));
} while(types[0] != field_type_MAX);
}
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 4a1fdb637..c8e242c2f 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2332,7 +2332,7 @@ sqlVdbeGetBoundValue(Vdbe * v, int iVar, u8 aff)
sql_value *pRet = sqlValueNew(v->db);
if (pRet) {
mem_copy(pRet, pMem);
- mem_cast_implicit_old(pRet, aff);
+ mem_cast_implicit(pRet, aff);
}
return pRet;
}
diff --git a/test/sql-tap/e_casts.test.lua b/test/sql-tap/e_casts.test.lua
index 32d7e8e0c..952572921 100755
--- a/test/sql-tap/e_casts.test.lua
+++ b/test/sql-tap/e_casts.test.lua
@@ -2,7 +2,7 @@
local tap = require("tap")
local test = tap.test("errno")
-test:plan(1)
+test:plan(3)
local yaml = require("yaml")
local ffi = require("ffi")
@@ -137,6 +137,22 @@ local explicit_casts_table_spec = {
[t_scalar] = {"Y", "S", "Y", "S", "S", "S", "S", "S", "S", "S", "" , "" , "Y"},
}
+local implicit_casts_table_spec = {
+ [t_any] = {"Y", "S", "S", "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", "" , "Y", "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"},
+ [t_varbinary]={"Y", "" , "Y", "" , "" , "" , "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", "" , "" , "" , "" , "" , "" , "" , "" , "" , "Y", "" , "" },
+ [t_map] = {"Y", "" , "" , "" , "" , "" , "" , "" , "" , "" , "" , "Y", "" },
+ [t_scalar] = {"Y", "S", "S", "S", "S", "S", "S", "S", "S", "S", "" , "" , "S"},
+}
+
local c_no = 0
local c_maybe = 1
local c_yes = 2
@@ -207,9 +223,31 @@ local function show_casts_table(table)
end
local explicit_casts = load_casts_spec(explicit_casts_table_spec, enabled_type, enabled_type_cast)
+local implicit_casts = load_casts_spec(implicit_casts_table_spec, enabled_type, enabled_type)
if verbose > 0 then
show_casts_table(explicit_casts)
+ show_casts_table(implicit_casts)
+end
+
+-- 0. check consistency of input conversion table
+
+-- 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 test_check_table_consistency(test)
+ test:plan(169)
+ for _, from in ipairs(proper_order) do
+ for _, to in ipairs(proper_order) do
+ test:ok((normalize_cast(implicit_casts[from][to]) ~= c_no) ==
+ (normalize_cast(implicit_casts[to][from]) ~= c_no),
+ label_for(from, to,
+ string.format("%s ~= %s",
+ implicit_casts[from][to],
+ implicit_casts[to][from])))
+ end
+ end
end
local function merge_tables(...)
@@ -334,7 +372,103 @@ local function test_check_explicit_casts(test)
end
end
+local table_name = 'TCASTS'
+
+local function _created_formatted_space(name)
+ local space = box.schema.space.create(name)
+ space:create_index('pk', {sequence = true})
+ local format = {{name = 'ID', type = 'unsigned', is_nullable = false}}
+ for _, type_id in ipairs(proper_order) do
+ if enabled_type[type_id] then
+ local type_name = type_names[type_id]
+ table.insert(format, {name = type_name, type = type_name, is_nullable = true})
+ end
+ end
+ if #format > 0 then
+ space:format(format)
+ end
+ return space
+end
+
+local function _cleanup_space(space)
+ space:drop()
+end
+
+-- implicit
+local function gen_implicit_insert_from_to(table_name, from, to)
+ local queries = {}
+ local from_exprs = gen_type_exprs(from)
+ for _, from_e in pairs(from_exprs) do
+ table.insert(queries,
+ string.format([[ insert into %s("%s") values(%s); ]],
+ table_name, type_names[to], from_e))
+ end
+ return queries
+end
+
+
+-- 2. Check implicit casts table
+local function test_check_implicit_casts(test)
+ test:plan(186)
+ local space = _created_formatted_space(table_name)
+ -- checking validity of all `from binop to` combinations
+ for _, from in ipairs(proper_order) do
+ for _, to in ipairs(proper_order) do
+ -- skip ANY, DECIMAL, UUID, etc.
+ if enabled_type[from] and enabled_type[to] then
+ local gen = gen_implicit_insert_from_to(table_name, from, to)
+ local failures = {}
+ local successes = {}
+ local castable = false
+ local expected = implicit_casts[from][to]
+
+ if verbose > 0 then
+ print(expected, yaml.encode(gen))
+ end
+
+ for _, v in pairs(gen) do
+ local ok, result
+ ok, result = catch_query(v)
+
+ if verbose > 0 then
+ print(string.format("V> ok = %s, result = %s, query = %s",
+ ok, result, v))
+ end
+
+ local title = string.format("%s => %s", v, human_cast(expected))
+ if expected == c_yes then
+ test:ok(true == ok, label_for(from, to, title))
+ elseif expected == c_no then
+ test:ok(false == ok, label_for(from, to, title))
+ 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
+ local title = string.format("%s => %s",
+ #gen and gen[1]..'...' or '',
+ human_cast(expected))
+ test:ok(castable, label_for(from, to, title),
+ failures)
+ end
+ end
+ end
+ end
+ _cleanup_space(space)
+end
+
+test:test("e_casts - check consistency of implicit conversion table", test_check_table_consistency)
test:test("e_casts - check explicit casts", test_check_explicit_casts)
+test:test("e_casts - check implicit casts", test_check_implicit_casts)
test:check()
os.exit()
diff --git a/test/sql-tap/in4.test.lua b/test/sql-tap/in4.test.lua
index 588daefec..6aeaff5d5 100755
--- a/test/sql-tap/in4.test.lua
+++ b/test/sql-tap/in4.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
local test = require("sqltester")
-test:plan(52)
+test:plan(53)
--!./tcltestrunner.lua
-- 2008 September 1
@@ -128,15 +128,26 @@ test:do_execsql_test(
})
test:do_execsql_test(
- "in4-2.6",
+ "in4-2.6.0",
[[
- SELECT b FROM t2 WHERE a IN (1.0, 2.1)
+ SELECT b FROM t2 WHERE a IN (1.0, 2.0)
]], {
-- <in4-2.6>
"one", "two"
-- </in4-2.6>
})
+-- FIXME - IN [2.1] should convert to expected type of a
+test:do_execsql_test(
+ "in4-2.6.1",
+ [[
+ SELECT b FROM t2 WHERE a IN (1.0, 2.1)
+ ]], {
+ -- <in4-2.6.1>
+ "one"
+ -- </in4-2.6.1>
+ })
+
test:do_execsql_test(
"in4-2.7",
[[
diff --git a/test/sql-tap/numcast.test.lua b/test/sql-tap/numcast.test.lua
index 6ca1316d5..798b2dc77 100755
--- a/test/sql-tap/numcast.test.lua
+++ b/test/sql-tap/numcast.test.lua
@@ -139,7 +139,7 @@ test:do_catchsql_test(
test:do_execsql_test(
"cast-2.9",
[[
- INSERT INTO t VALUES(2.1);
+ INSERT INTO t VALUES(2.0);
SELECT * FROM t;
]], {
2, 9223372036854775808ULL, 18000000000000000000ULL
diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-9a8b09f8e6.test.lua
index 67d6a1ccd..8ecf9f914 100755
--- a/test/sql-tap/tkt-9a8b09f8e6.test.lua
+++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua
@@ -239,23 +239,23 @@ test:do_execsql_test(
-- </4.2>
})
-test:do_catchsql_test(
+test:do_execsql_test(
4.3,
[[
SELECT x FROM t3 WHERE x IN ('1');
]], {
-- <4.3>
- 1, "Type mismatch: can not convert 1 to number"
+ 1.0
-- </4.3>
})
-test:do_catchsql_test(
+test:do_execsql_test(
4.4,
[[
SELECT x FROM t3 WHERE x IN ('1.0');
]], {
-- <4.4>
- 1, "Type mismatch: can not convert 1.0 to number"
+ 1.0
-- </4.4>
})
@@ -279,23 +279,23 @@ test:do_execsql_test(
-- </4.6>
})
-test:do_catchsql_test(
+test:do_execsql_test(
4.7,
[[
SELECT x FROM t3 WHERE '1' IN (x);
]], {
-- <4.7>
- 1, "Type mismatch: can not convert 1 to number"
+ 1.0
-- </4.7>
})
-test:do_catchsql_test(
+test:do_execsql_test(
4.8,
[[
SELECT x FROM t3 WHERE '1.0' IN (x);
]], {
-- <4.8>
- 1, "Type mismatch: can not convert 1.0 to number"
+ 1.0
-- </4.8>
})
@@ -319,23 +319,23 @@ test:do_execsql_test(
-- </5.2>
})
-test:do_catchsql_test(
+test:do_execsql_test(
5.3,
[[
SELECT x FROM t4 WHERE x IN ('1');
]], {
-- <5.3>
- 1, "Type mismatch: can not convert 1 to number"
+
-- </5.3>
})
-test:do_catchsql_test(
+test:do_execsql_test(
5.4,
[[
SELECT x FROM t4 WHERE x IN ('1.0');
]], {
-- <5.4>
- 1, "Type mismatch: can not convert 1.0 to number"
+
-- </5.4>
})
@@ -349,13 +349,13 @@ test:do_execsql_test(
-- </5.5>
})
-test:do_catchsql_test(
+test:do_execsql_test(
5.6,
[[
SELECT x FROM t4 WHERE x IN ('1.11');
]], {
-- <5.6>
- 1, "Type mismatch: can not convert 1.11 to number"
+ 1.11
-- </5.6>
})
@@ -379,23 +379,23 @@ test:do_execsql_test(
-- </5.8>
})
-test:do_catchsql_test(
+test:do_execsql_test(
5.9,
[[
SELECT x FROM t4 WHERE '1' IN (x);
]], {
-- <5.9>
- 1, "Type mismatch: can not convert 1 to number"
+
-- </5.9>
})
-test:do_catchsql_test(
+test:do_execsql_test(
5.10,
[[
SELECT x FROM t4 WHERE '1.0' IN (x);
]], {
-- <5.10>
- 1, "Type mismatch: can not convert 1.0 to number"
+
-- </5.10>
})
@@ -409,13 +409,13 @@ test:do_execsql_test(
-- </5.11>
})
-test:do_catchsql_test(
+test:do_execsql_test(
5.12,
[[
SELECT x FROM t4 WHERE '1.11' IN (x);
]], {
-- <5.12>
- 1, "Type mismatch: can not convert 1.11 to number"
+ 1.11
-- </5.12>
})
diff --git a/test/sql/types.result b/test/sql/types.result
index 90a8bc5ec..20dbd2073 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1059,7 +1059,8 @@ box.execute("INSERT INTO t1 VALUES (0), (1), (2);")
box.execute("INSERT INTO t1 VALUES (-3);")
---
- null
-- 'Type mismatch: can not convert -3 to unsigned'
+- 'Tuple field 1 (ID) type does not match one required by operation: expected unsigned,
+ got integer'
...
box.execute("SELECT id FROM t1;")
---
@@ -1224,12 +1225,13 @@ box.execute("INSERT INTO t VALUES(1, true);")
...
box.execute("INSERT INTO t VALUES(1, 'asd');")
---
-- null
-- 'Type mismatch: can not convert asd to varbinary'
+- row_count: 1
...
box.execute("INSERT INTO t VALUES(1, x'616263');")
---
-- row_count: 1
+- null
+- Duplicate key exists in unique index "pk_unnamed_T_1" in space "T" with old tuple
+ - [1, "asd"] and new tuple - [1, "abc"]
...
box.execute("SELECT * FROM t WHERE v = 1")
---
@@ -1253,8 +1255,7 @@ box.execute("SELECT * FROM t WHERE v = x'616263'")
type: integer
- name: V
type: varbinary
- rows:
- - [1, 'abc']
+ rows: []
...
box.execute("SELECT sum(v) FROM t;")
---
@@ -1277,7 +1278,7 @@ box.execute("SELECT min(v) FROM t;")
- name: COLUMN_1
type: scalar
rows:
- - ['abc']
+ - ['asd']
...
box.execute("SELECT max(v) FROM t;")
---
@@ -1285,7 +1286,7 @@ box.execute("SELECT max(v) FROM t;")
- name: COLUMN_1
type: scalar
rows:
- - ['abc']
+ - ['asd']
...
box.execute("SELECT count(v) FROM t;")
---
@@ -1301,7 +1302,7 @@ box.execute("SELECT group_concat(v) FROM t;")
- name: COLUMN_1
type: string
rows:
- - ['abc']
+ - ['asd']
...
box.execute("SELECT lower(v) FROM t;")
---
@@ -1332,7 +1333,7 @@ box.execute("SELECT quote(v) FROM t;")
- name: COLUMN_1
type: string
rows:
- - ['X''616263''']
+ - ['X''617364''']
...
box.execute("SELECT LEAST(v, x'') FROM t;")
---
@@ -1351,8 +1352,7 @@ box.execute("SELECT v FROM t WHERE v = x'616263';")
- metadata:
- name: V
type: varbinary
- rows:
- - ['abc']
+ rows: []
...
box.execute("SELECT v FROM t ORDER BY v;")
---
@@ -1360,11 +1360,11 @@ box.execute("SELECT v FROM t ORDER BY v;")
- name: V
type: varbinary
rows:
- - ['abc']
+ - ['asd']
...
box.execute("UPDATE t SET v = x'636261' WHERE v = x'616263';")
---
-- row_count: 1
+- row_count: 0
...
box.execute("SELECT v FROM t;")
---
@@ -1372,7 +1372,7 @@ box.execute("SELECT v FROM t;")
- name: V
type: varbinary
rows:
- - ['cba']
+ - ['asd']
...
box.execute("CREATE TABLE parent (id INT PRIMARY KEY, a VARBINARY UNIQUE);")
---
@@ -2206,8 +2206,9 @@ box.execute([[INSERT INTO ti(i) VALUES (true);]])
...
box.execute([[INSERT INTO ti(i) VALUES ('33');]])
---
-- null
-- 'Type mismatch: can not convert 33 to integer'
+- autoincrement_ids:
+ - 4
+ row_count: 1
...
box.execute([[INSERT INTO ti(i) VALUES (X'3434');]])
---
@@ -2225,6 +2226,7 @@ box.execute([[SELECT * FROM ti;]])
- [1, null]
- [2, 11]
- [3, 33]
+ - [4, 33]
...
box.execute([[INSERT INTO td(d) VALUES (NULL);]])
---
@@ -2256,8 +2258,9 @@ box.execute([[INSERT INTO td(d) VALUES (true);]])
...
box.execute([[INSERT INTO td(d) VALUES ('33');]])
---
-- null
-- 'Type mismatch: can not convert 33 to double'
+- autoincrement_ids:
+ - 4
+ row_count: 1
...
box.execute([[INSERT INTO td(d) VALUES (X'3434');]])
---
@@ -2275,6 +2278,7 @@ box.execute([[SELECT * FROM td;]])
- [1, null]
- [2, 11]
- [3, 22.2]
+ - [4, 33]
...
box.execute([[INSERT INTO tb(b) VALUES (NULL);]])
---
@@ -2327,13 +2331,15 @@ box.execute([[INSERT INTO tt(t) VALUES (NULL);]])
...
box.execute([[INSERT INTO tt(t) VALUES (11);]])
---
-- null
-- 'Type mismatch: can not convert 11 to string'
+- autoincrement_ids:
+ - 2
+ row_count: 1
...
box.execute([[INSERT INTO tt(t) VALUES (22.2);]])
---
-- null
-- 'Type mismatch: can not convert 22.2 to string'
+- autoincrement_ids:
+ - 3
+ row_count: 1
...
box.execute([[INSERT INTO tt(t) VALUES (true);]])
---
@@ -2343,13 +2349,14 @@ box.execute([[INSERT INTO tt(t) VALUES (true);]])
box.execute([[INSERT INTO tt(t) VALUES ('33');]])
---
- autoincrement_ids:
- - 2
+ - 4
row_count: 1
...
box.execute([[INSERT INTO tt(t) VALUES (X'3434');]])
---
-- null
-- 'Type mismatch: can not convert varbinary to string'
+- autoincrement_ids:
+ - 5
+ row_count: 1
...
box.execute([[SELECT * FROM tt;]])
---
@@ -2360,7 +2367,10 @@ box.execute([[SELECT * FROM tt;]])
type: string
rows:
- [1, null]
- - [2, '33']
+ - [2, '11']
+ - [3, '22.2']
+ - [4, '33']
+ - [5, '44']
...
box.execute([[INSERT INTO tv(v) VALUES (NULL);]])
---
@@ -2385,13 +2395,14 @@ box.execute([[INSERT INTO tv(v) VALUES (true);]])
...
box.execute([[INSERT INTO tv(v) VALUES ('33');]])
---
-- null
-- 'Type mismatch: can not convert 33 to varbinary'
+- autoincrement_ids:
+ - 2
+ row_count: 1
...
box.execute([[INSERT INTO tv(v) VALUES (X'3434');]])
---
- autoincrement_ids:
- - 2
+ - 3
row_count: 1
...
box.execute([[SELECT * FROM tv;]])
@@ -2403,7 +2414,8 @@ box.execute([[SELECT * FROM tv;]])
type: varbinary
rows:
- [1, null]
- - [2, '44']
+ - [2, '33']
+ - [3, '44']
...
box.execute([[INSERT INTO ts(s) VALUES (NULL);]])
---
@@ -2459,11 +2471,11 @@ box.execute([[SELECT * FROM ts;]])
-- Check for UPDATE.
box.execute([[DELETE FROM ti;]])
---
-- row_count: 3
+- row_count: 4
...
box.execute([[DELETE FROM td;]])
---
-- row_count: 3
+- row_count: 4
...
box.execute([[DELETE FROM tb;]])
---
@@ -2471,11 +2483,11 @@ box.execute([[DELETE FROM tb;]])
...
box.execute([[DELETE FROM tt;]])
---
-- row_count: 2
+- row_count: 5
...
box.execute([[DELETE FROM tv;]])
---
-- row_count: 2
+- row_count: 3
...
box.execute([[DELETE FROM ts;]])
---
@@ -2559,8 +2571,7 @@ box.execute([[UPDATE ti SET i = true WHERE a = 1;]])
...
box.execute([[UPDATE ti SET i = '33' WHERE a = 1;]])
---
-- null
-- 'Type mismatch: can not convert 33 to integer'
+- row_count: 1
...
box.execute([[UPDATE ti SET i = X'3434' WHERE a = 1;]])
---
@@ -2600,8 +2611,7 @@ box.execute([[UPDATE td SET d = true WHERE a = 1;]])
...
box.execute([[UPDATE td SET d = '33' WHERE a = 1;]])
---
-- null
-- 'Type mismatch: can not convert 33 to double'
+- row_count: 1
...
box.execute([[UPDATE td SET d = X'3434' WHERE a = 1;]])
---
@@ -2616,7 +2626,7 @@ box.execute([[SELECT * FROM td;]])
- name: D
type: double
rows:
- - [1, 22.2]
+ - [1, 33]
...
box.execute([[UPDATE tb SET b = NULL WHERE a = 1;]])
---
@@ -2662,13 +2672,11 @@ box.execute([[UPDATE tt SET t = NULL WHERE a = 1;]])
...
box.execute([[UPDATE tt SET t = 11 WHERE a = 1;]])
---
-- null
-- 'Type mismatch: can not convert 11 to string'
+- row_count: 1
...
box.execute([[UPDATE tt SET t = 22.2 WHERE a = 1;]])
---
-- null
-- 'Type mismatch: can not convert 22.2 to string'
+- row_count: 1
...
box.execute([[UPDATE tt SET t = true WHERE a = 1;]])
---
@@ -2681,8 +2689,7 @@ box.execute([[UPDATE tt SET t = '33' WHERE a = 1;]])
...
box.execute([[UPDATE tt SET t = X'3434' WHERE a = 1;]])
---
-- null
-- 'Type mismatch: can not convert varbinary to string'
+- row_count: 1
...
box.execute([[SELECT * FROM tt;]])
---
@@ -2692,7 +2699,7 @@ box.execute([[SELECT * FROM tt;]])
- name: T
type: string
rows:
- - [1, '33']
+ - [1, '44']
...
box.execute([[UPDATE tv SET v = NULL WHERE a = 1;]])
---
@@ -2715,8 +2722,7 @@ box.execute([[UPDATE tv SET v = true WHERE a = 1;]])
...
box.execute([[UPDATE tv SET v = '33' WHERE a = 1;]])
---
-- null
-- 'Type mismatch: can not convert 33 to varbinary'
+- row_count: 1
...
box.execute([[UPDATE tv SET v = X'3434' WHERE a = 1;]])
---
--
2.29.2
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables
2021-06-11 7:48 [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Timur Safin via Tarantool-patches
` (2 preceding siblings ...)
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 3/3] sql: updated implicit " Timur Safin via Tarantool-patches
@ 2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches
2021-06-27 23:29 ` Timur Safin via Tarantool-patches
3 siblings, 1 reply; 18+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-06-20 18:52 UTC (permalink / raw)
To: Timur Safin; +Cc: tarantool-patches, alexander.turenko
Hi! Thank you for the patch-set. See 7 comments below.
0. Please include you answers to my previous comments right here. Same
about all other letters.
On Fri, Jun 11, 2021 at 10:48:10AM +0300, Timur Safin wrote:
> Issues:
> https://github.com/tarantool/tarantool/issues/4470
> https://github.com/tarantool/tarantool/issues/6010
> Branch: https://github.com/tarantool/tarantool/tree/tsafin/gh-4470-explicit-implicit-conversions-V2
>
> This patchset containes of 2 major part and 1 bonus. Major parts
> are changes in explicit and implicit conversion tables, with all
> accompanying tests we have so far (and 1 extensive test introduced).
> The bonus part is for improvements in the testing system tap.lua
> which became apparent during debugging of this patches.
1. Please divide this patch-set in two or three parts. I think the best
way will
be to divide it to three parts:
i. Patch about tap.lua.
ii. Patch-set about explicit casts.
iii. Patch-set about implicit casts.
Combining ii and iii is also acceptable, but I think the approaches to
ii and
iii should be completely different. I don't see any connection between the
tap.lua patch and all the other patches in this patchset.
I think it is strange, that only part of the patch-set is reviewed by
me. Is it
even possible for me to give LGTM to such patch-set?
>
> Relates to #5910, #6009
> Part of #4470
> Fixes #6010
>
> Explicit table
> ==============
>
> As we discovered harder way, there is no need to introduce so much changes
> to the current explicit conversion table, because it's mostly compliant
> already:
> - most of changes we had to do was about `BOOLEAN` conversions, which are
> stricter now than before;
> - there are addition of to `ANY` conversion, which we have decided to make
> behaving similar to `SCALAR` conversions;
2. As I previously said, I see no reason to include cast to field type
ANY in
this patch-set. Also, if we follow your logic, why you didn't added casts to
array and map?
>
> Those changes could be visualized via this table:
>
>
> | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+
> 0. any | | | | | | | | | | | | | |
> 1. unsigned | | . | . | . | . | - | | . | | | | | Y |
> 2. string | | . | . | . | . | S | . | . | | | | | Y |
> 4. double | | . | . | . | . | - | | . | | | | | Y |
> 5. integer | | . | . | . | . | - | | . | | | | | Y |
> 6. boolean | | - | Y | - | - | Y | | | | | | | Y |
> 7. varbinary | | | . | | | - | . | | | | | | Y |
> 3. number | | . | . | . | . | - | | . | | | | | Y |
> 9. decimal | | | | | | | | | | | | | |
> 10. uuid | | | | | | | | | | | | | |
> 11. array | | | | | | | | | | | | | |
> 12. map | | | | | | | | | | | | | |
> 8. scalar | | . | . | . | . | S | . | . | | | | | Y |
> +---+---+---+---+---+---+---+---+---+---+---+---+---+
3. Since you do not want to change numbers here, I suggest to arrange field
types according to their number. Also, why you described BOOLEAN, but didn't
describe ANY and UUID?
>
> This table above represent all possible combinations of explicit cast
> from type (row) to another type (column). Numbering of types might look
> confusing (it's not sequential) but the order is what we see in the
> consistency specification RFC, and we should live with that. Values are
> actual enum values from the code. Unfortunately, changing of order is
> almost impossible because of massive rework for the spec it would require.
>
> How to interpret this table, e.g.:
> - for the explicit cast from `BOOLEAN` (6) to `BOOLEAN` (6) we should
> always allow cast (and make it noop), thus intersection is `Y` (yes);
>
> - `STRING` (2) to `BOOLEAN` (6) may work sometimes (if string represents
> TRUE or FALSE literals), but may fail otherwise, thus there is `S` (sometimes);
>
4. "Sometimes" still sounds quite not right here.
> - We have prohibited majority of ther directions for `BOOLEAN` thus there are
> `-` (minus) in such cells, i.e. `BOOLEAN` (6) to `INTEGER` (5);
>
> - Untouched entries in the table marked with '.'. Assumption is -
> we already have correct conversion rule here activated well before;
>
> - Worth to mention that empty cell means that this conversion
> direction is prohibited.
>
5. Does it mean that cast from UUID to UUID is prohibited? Also, I see
no need
of '.' since you sometimes use 'Y' or '-' or ' ' instead of it. I
believe you
should describe table for all explicit cast conversions here and not
only for
BOOLEAN.
> For obvious reasons those chnaged conversion rules made us modify
> expected results for many tests - they are fixed.
>
> But to cover all possible conversion combinations (minus those, not yet
> implemented in SQL types like DECIMAL, ARRAY and MAP) we have created
> special test which check _all combinations of CASTs()_ and verifies
> results against table rules defined in the RFC.
>
> There is e_casts.test.lua created for that purpose.
6. I still does not understand where this name came from. I believe you said
something about renaming it.
>
> Implicit table
> ==============
>
> Similarly to explicit conversion rules we have update implicit conversion
> rules, and this is 2nd part of patchset.
>
> We will not draw changed values of implicit conversion table (please see
> it's final state in the consistemcy types RFC), but verbally we have
> modified following directions of conversions:
> - string to/from double
> - double to/from integer
> - varbinary to/from string
>
> In addition to modifciation of all relevant test results, we have also
> extended e_casts.test.lua introduced above for checking of all possible
> directions of implicit conversions.
>
> Approach to check it though is different than for explicit cast, we do
> not use simple expression with implicit cast activated (like we might be
> using `CAST()` for explicit casts), but rather we insert value of original
> type into columns of expected target type.
7. Implicit casts includes a lot more than just an insert, I believe.
>
> Relates to #5910, #6009
> Closes #4470
>
>
> Bonus - better error reporting in tap tests
> ===========================================
>
> Also, during debugging of explicit/implicitit conversions test it was
> discovered the harder way, that TAP infrastructure does not report correctly
> origin source line of and error, but rather report :0 line as location.
> This has been fixed.
>
> Closes #6134
>
>
> Timur Safin (3):
> test: corrected reported error lines
> sql: updated explicit conversion table
> sql: updated implicit conversion table
>
> extra/mkkeywordhash.c | 3 +-
> src/box/sql/mem.c | 188 +++--------
> src/box/sql/mem.h | 6 -
> src/box/sql/parse.y | 9 +-
> src/box/sql/util.c | 3 +-
> src/box/sql/vdbe.c | 8 +-
> src/box/sql/vdbeaux.c | 2 +-
> src/lua/tap.lua | 2 +-
> test/sql-tap/cse.test.lua | 12 +-
> test/sql-tap/e_casts.test.lua | 474 +++++++++++++++++++++++++++
> test/sql-tap/e_select1.test.lua | 2 +-
> test/sql-tap/in1.test.lua | 8 +-
> test/sql-tap/in4.test.lua | 19 +-
> test/sql-tap/misc3.test.lua | 2 +-
> test/sql-tap/numcast.test.lua | 2 +-
> test/sql-tap/tkt-9a8b09f8e6.test.lua | 40 +--
> test/sql/boolean.result | 38 +--
> test/sql/types.result | 132 ++++----
> 18 files changed, 654 insertions(+), 296 deletions(-)
> create mode 100755 test/sql-tap/e_casts.test.lua
>
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/3] sql: updated explicit conversion table
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 2/3] sql: updated explicit conversion table Timur Safin via Tarantool-patches
@ 2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches
2021-06-25 21:26 ` Timur Safin via Tarantool-patches
2021-06-25 21:26 ` [Tarantool-patches] Отзыв: " Timur Safin via Tarantool-patches
0 siblings, 2 replies; 18+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-06-20 18:52 UTC (permalink / raw)
To: Timur Safin; +Cc: tarantool-patches
Thank you for the fixes you did. See my 27 comments below.
1. In general I do not think that it is good idea to squash all previous
three
patches to one patch. I think that the first of previous patches was
quite good,
the second was useless and the third had some issues, but now all these
patches
are squashed.
On Fri, Jun 11, 2021 at 10:48:12AM +0300, Timur Safin wrote:
> * fixes for `BOOLEAN` expressions in explicit converstion tables
>
> 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 still do not think that path to RFC is needed here. Also, what
about '.'?
>
> 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 used to be allowed before.
>
> We updated all relevant tests we already have for some combinations.
>
> * introduce `ANY` as target for explicit conversions
>
> For consistency sake it was decided to provide
> `CAST(anything TO ANY)` as allowed, but still noop.
>
3. I still do not like that you added cast to unsupported field.
4. Why typeof() of value cast to any returns 'any'?
tarantool> box.execute('select typeof(cast(1 AS any));')
---
- metadata:
- name: COLUMN_1
type: string
rows:
- ['any']
...
tarantool> box.execute('select typeof(cast(1 AS scalar));')
---
- metadata:
- name: COLUMN_1
type: string
rows:
- ['integer']
...
> Note, that there is no direct way in SQL to create literal and expression
> of type `ANY`, thus there is no defined CAST from ANY to anything
> else, only to `ANY`.
>
> * We had to rename %wildcard token to ANYTHING, since we needed
> token ANY for real life usage.
>
5. Just a thought - why you cannot use 'ANYTHING' for field type ANY?
Or 'ANY_KW', how it was already done for INTEGER?
> * As a byproduct, we fixed #6010 to make cast to unsigned behaving reasonably
>
6. I think this change is good enough to have its own patch.
> * To make sure that all consistency checks are systematic, we have introduced
> special test for checking conversion rules - e_casts.test.lua. This
> patch introduces explicit table part:
>
> * e_casts.test.lua is generating expressions of `CAST(input AS output)`
> form. All combinations check whether we expectedly succeeded or failed,
> given the explicit conversion table from RFC 5910-consistent-sql-lua-types.md;
>
> * At the moment we skip DECIMALs, ARRAYs and MAPs as not yet
> fully supported. Need to be revisited later;
>
> * NB! there is `-verbose` mode one may activate to enable more detailed
> reporting during debugging.
>
7. I used './test-run.py sql-tap/e_casts.test.lua --verbose -j1' and saw
this:
[001] sql-tap/e_casts.test.lua memtx [001] TAP
version 13
[001] 1..3
[001] # e_casts - check consistency of implicit conversion table
[001] 1..169
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,any] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,unsigned] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,string] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,double] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,integer] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,boolean] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,varbinary] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,number] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,decimal] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,uuid] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,array] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,map] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[any,scalar] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,any] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,unsigned] 2 ~= 2
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,string] 2 ~= 1
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,double] 2 ~= 1
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,integer] 2 ~= 1
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,boolean] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,varbinary] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,number] 2 ~= 1
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,decimal] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,uuid] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,array] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,map] nil ~= nil
[001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
[unsigned,scalar] 2 ~= 1
...
What is it?
> Relates to #5910, #6009
> Part of #4470
> Fixes #6010
> ---
> extra/mkkeywordhash.c | 3 +-
> src/box/sql/mem.c | 81 +++-----
> src/box/sql/parse.y | 9 +-
> test/sql-tap/cse.test.lua | 12 +-
> test/sql-tap/e_casts.test.lua | 340 ++++++++++++++++++++++++++++++++
> test/sql-tap/e_select1.test.lua | 2 +-
> test/sql-tap/in1.test.lua | 8 +-
> test/sql-tap/in4.test.lua | 2 +-
> test/sql-tap/misc3.test.lua | 2 +-
> test/sql/boolean.result | 38 +---
> test/sql/types.result | 28 ++-
> 11 files changed, 406 insertions(+), 119 deletions(-)
> create mode 100755 test/sql-tap/e_casts.test.lua
>
> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
> index 0d998506c..00f65ce8e 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_ANY", true },
> { "AND", "TK_AND", true },
> { "AS", "TK_AS", true },
> { "ASC", "TK_ASC", true },
> @@ -179,7 +180,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 6f3bf52e5..8bb6b672c 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -574,17 +574,6 @@ int_to_str0(struct Mem *mem)
> return mem_copy_str0(mem, str);
> }
>
> -static inline int
> -int_to_bool(struct Mem *mem)
> -{
> - assert((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0);
> - mem->u.b = mem->u.i != 0;
> - mem->type = MEM_TYPE_BOOL;
> - assert(mem->flags == 0);
> - mem->field_type = FIELD_TYPE_BOOLEAN;
> - return 0;
> -}
> -
> static inline int
> str_to_str0(struct Mem *mem)
> {
> @@ -810,28 +799,6 @@ double_to_str0(struct Mem *mem)
> return 0;
> }
>
> -static inline int
> -double_to_bool(struct Mem *mem)
> -{
> - assert(mem->type == MEM_TYPE_DOUBLE);
> - mem->u.b = mem->u.r != 0.;
> - mem->type = MEM_TYPE_BOOL;
> - assert(mem->flags == 0);
> - mem->field_type = FIELD_TYPE_BOOLEAN;
> - return 0;
> -}
> -
> -static inline int
> -bool_to_int(struct Mem *mem)
> -{
> - assert(mem->type == MEM_TYPE_BOOL);
> - mem->u.u = (uint64_t)mem->u.b;
> - mem->type = MEM_TYPE_UINT;
> - assert(mem->flags == 0);
> - mem->field_type = FIELD_TYPE_UNSIGNED;
> - return 0;
> -}
> -
> static inline int
> bool_to_str0(struct Mem *mem)
> {
> @@ -872,18 +839,37 @@ uuid_to_bin(struct Mem *mem)
> return mem_copy_bin(mem, (char *)&mem->u.uuid, UUID_LEN);
> }
>
> +int
> +mem_to_uint(struct Mem *mem)
> +{
> + assert(mem->type < MEM_TYPE_INVALID);
> + if (mem->type == MEM_TYPE_UINT)
> + return 0;
> + if (mem->type == MEM_TYPE_INT) {
> + mem_set_uint(mem, (uint64_t)mem->u.i);
8. Could you explain to me, what it this? Since when -1 can be cast to
UNSIGNED?
Where it is described? Why this is allowed?
tarantool> box.execute('select CAST(-1 as unsigned);')
---
- metadata:
- name: COLUMN_1
type: unsigned
rows:
- [18446744073709551615]
...
> + return 0;
> + }
> + if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0)
> + return bytes_to_uint(mem);
> + if (mem->type == MEM_TYPE_DOUBLE)
> + return double_to_uint(mem);
> + return -1;
> +}
> +
> int
> mem_to_int(struct Mem *mem)
> {
> assert(mem->type < MEM_TYPE_INVALID);
> - if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
> + if (mem->type == MEM_TYPE_INT)
> + return 0;
> + if (mem->type == MEM_TYPE_UINT) {
> + mem_set_int(mem, (int64_t)mem->u.u, false);
9. So, you want to set unsigned value as integer. Could you tell me what
does
this change do?
> return 0;
> + }
> if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0)
> return bytes_to_int(mem);
> if (mem->type == MEM_TYPE_DOUBLE)
> return double_to_int(mem);
> - if (mem->type == MEM_TYPE_BOOL)
> - return bool_to_int(mem);
> return -1;
> }
>
> @@ -919,8 +905,6 @@ mem_to_number(struct Mem *mem)
> assert(mem->type < MEM_TYPE_INVALID);
> if (mem_is_num(mem))
> return 0;
> - if (mem->type == MEM_TYPE_BOOL)
> - return bool_to_int(mem);
> if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0) {
> if (bytes_to_int(mem) == 0)
> return 0;
> @@ -994,19 +978,7 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
> }
> switch (type) {
> case FIELD_TYPE_UNSIGNED:
> - switch (mem->type) {
> - case MEM_TYPE_UINT:
> - return 0;
> - case MEM_TYPE_STR:
> - case MEM_TYPE_BIN:
> - return bytes_to_uint(mem);
> - case MEM_TYPE_DOUBLE:
> - return double_to_int(mem);
> - case MEM_TYPE_BOOL:
> - return bool_to_int(mem);
> - default:
> - return -1;
> - }
> + return mem_to_uint(mem);
> case FIELD_TYPE_STRING:
> return mem_to_str(mem);
> case FIELD_TYPE_DOUBLE:
> @@ -1017,13 +989,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
> switch (mem->type) {
10. Just a thought. I see no harm to have this switch here, but it has
only two
options. Why not change it to 'if'?
> case MEM_TYPE_BOOL:
> return 0;
> - case MEM_TYPE_INT:
> - case MEM_TYPE_UINT:
> - return int_to_bool(mem);
> case MEM_TYPE_STR:
> return str_to_bool(mem);
> - case MEM_TYPE_DOUBLE:
> - return double_to_bool(mem);
> default:
> return -1;
> }
> @@ -1049,6 +1016,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
> if ((mem->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 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 bd041e862..fbff6ffa7 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 UUID
> .
> -%wildcard ANY.
> +%wildcard ANYTHING.
>
>
> // And "ids" is an identifer-or-string.
> @@ -1113,7 +1113,7 @@ expr(A) ::= expr(A) COLLATE id(C). {
> A.zEnd = &C.z[C.n];
> }
>
> -expr(A) ::= CAST(X) LP expr(E) AS typedef(T) RP(Y). {
> +expr(A) ::= CAST(X) LP expr(E) AS cast_typedef(T) RP(Y). {
> spanSet(&A,&X,&Y); /*A-overwrites-X*/
> A.pExpr = sql_expr_new_dequoted(pParse->db, TK_CAST, NULL);
> if (A.pExpr == NULL) {
> @@ -1887,3 +1887,8 @@ number_typedef(A) ::= UNSIGNED . { A.type = FIELD_TYPE_UNSIGNED; }
> * (void) C;
> *}
> */
> +
> +// cast_typedef is almost typedef but with ANY type enabled
> +%type cast_typedef { struct type_def }
> +cast_typedef(A) ::= typedef(T) . { A = T; }
> +cast_typedef(A) ::= ANY . { A.type = FIELD_TYPE_ANY; }
> diff --git a/test/sql-tap/cse.test.lua b/test/sql-tap/cse.test.lua
> index e09f955a0..932f83545 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_casts.test.lua b/test/sql-tap/e_casts.test.lua
> new file mode 100755
> index 000000000..32d7e8e0c
> --- /dev/null
> +++ b/test/sql-tap/e_casts.test.lua
> @@ -0,0 +1,340 @@
> +#!/usr/bin/env tarantool
> +local tap = require("tap")
> +local test = tap.test("errno")
> +
> +test:plan(1)
> +
> +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
> + };
> +]]
11. Since this cdef is only copy-paste from current actual enum, I still see
not reason to keep it here. Or you could use one from module.h, if you want
to keep it.
> +
> +-- 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] = true,
> + -- [t_date] = false,
> + -- [t_time] = false,
> + -- [t_timestamp]= false,
> + -- [t_interval] = False,
> + [t_array] = false,
> + [t_map] = false,
> + [t_scalar] = true,
> +}
> +
> +-- Enabled types which may be targets for explicit casts
> +local enabled_type_cast = table.deepcopy(enabled_type)
> +enabled_type_cast[t_any] = true
> +
> +-- table of _TSV_ (tab separated values)
> +-- copied from sql-lua-tables-v5.xls
> +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"},
> +}
12. Again, why there is 'N' and '' both in table? Could you show me where in
RFC we have this table that contains both '' and 'N'?
> +
> +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, enabled_from, enabled_to)
> + local casts = {}
> + for _, t_from in ipairs(proper_order) do
> + casts[t_from] = {}
> + for j, t_to in ipairs(proper_order) do
> + if enabled_from[t_from] and enabled_to[t_to] then
> + casts[t_from][t_to] = normalize_cast(spec_table[t_from][j])
> + end
> + end
> + end
> + return casts
> +end
> +
> +local function label_for(from, to, title)
> + 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], title)
> +end
> +
> +local function show_casts_table(table)
> + local max_len = #"12. varbinary" + 1
> +
> + -- show banner
> + local col_names = ''
> + for _, 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 _, from in ipairs(proper_order) do
> + local line = ''
> + for _, 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
13. Since you use '-verbose' mode only in your debug, can you move these
functions out from here? I still do not see any use of this code.
> +
> +local explicit_casts = load_casts_spec(explicit_casts_table_spec, enabled_type, enabled_type_cast)
> +
> +if verbose > 0 then
> + 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]
> + assert(type(tables[i]) == 'table')
> + for _, v in pairs(t) do
> + table.insert(result, v)
> + end
> + end
> + return result
> +end
> +
> +local gen_type_samples = {
> + [t_unsigned] = {"0", "1", "2"},
14. I believe you once said something about testing values that are close to
all limits of numeric types. Why I cannot see them here?
> + [t_integer] = {"-678", "-1", "0", "1", "2", "3", "678"},
> + [t_double] = {"0.0", "123.4", "-567.8"},
> + [t_string] = {"''", "'1'", "'123.4'", "'-1.5'", "'abc'", "'def'",
> + "'TRUE'", "'FALSE'", "'22222222-1111-1111-1111-111111111111'"},
> + [t_boolean] = {"false", "true", "null"},
> + [t_varbinary] = {"X'312E3233'", "X'2D392E3837'", "X'302E30303031'",
> + "x'22222222111111111111111111111111'"},
> + [t_uuid] = {'uuid()'},
> +}
> +
> +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
> +
> +-- explicit
> +local function gen_explicit_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
> + table.insert(queries,
> + string.format([[ select cast(%s as %s); ]], expr, to_typename))
> + 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
15. Since you use all these functions and do not want to remove them, I
suggest
to add comments to them.
> +
> +-- 1. Check explicit casts table
> +local function test_check_explicit_casts(test)
> + -- checking validity of all `CAST(from AS to)` combinations
> + test:plan(322)
> + for _, from in ipairs(proper_order) do
> + for _, to in ipairs(proper_order) do
> + -- skip ANY, DECIMAL, UUID, etc.
16. Why UUID is skipped? Also, why ANY is skipped? You can get values of
type
ANY by using CAST(values AS ANY), I believe. Or I am wrong?
> + if enabled_type[from] and enabled_type_cast[to] then
> + local gen = gen_explicit_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 _, v in pairs(gen) do
> + local ok, result
> + ok, result = catch_query(v)
> +
> + if verbose > 0 then
> + print(string.format("V> ok = %s, result = %s, query = %s",
> + ok, result, v))
> + end
> +
> + local title = string.format("%s => %s", v, human_cast(expected))
> + if expected == c_yes then
> + test:ok(true == ok, label_for(from, to, title))
> + elseif expected == c_no then
> + test:ok(false == ok, label_for(from, to, title))
> + 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
> + local title = string.format("%s => %s",
> + #gen and gen[1]..'...' or '',
> + human_cast(expected))
> + test:ok(castable, label_for(from, to, title),
> + failures)
> + end
16. I already said my expectations about these tests. Since you know
(and print)
all values used in the test, why cannot you determine if the cast should
fail?
For example, this test is actually useless if `CAST(1.0 AS UNSIGNED)`
fails, since
this cast is marked as 'sometimes'. If you do so, you can just print 'Y'
or 'N'.
Also, why your tests does not check unallowed casts? Or, at least, they
are not
shown.
> + end
> + end
> + end
> +end
> +
> +test:test("e_casts - check explicit casts", test_check_explicit_casts)
> +
> +test:check()
> +os.exit()
> 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..da3c07768 100755
> --- a/test/sql-tap/in1.test.lua
> +++ b/test/sql-tap/in1.test.lua
> @@ -100,7 +100,7 @@ test:do_execsql_test(
> test:do_execsql_test(
> "in-1.7",
> [[
> - SELECT a+ 100*CAST((a BETWEEN 1 and 3) AS INTEGER) FROM t1 ORDER BY b
> + SELECT a+ 100*(CASE WHEN (a BETWEEN 1 and 3) THEN 1 ELSE 0 END) FROM t1 ORDER BY b
17. Since you already change this line, add spaces before and after all
operations with two operands.
> ]], {
> -- <in-1.7>
> 101, 102, 103, 4, 5, 6, 7, 8, 9, 10
> @@ -157,7 +157,7 @@ test:do_execsql_test(
> test:do_execsql_test(
> "in-2.5",
> [[
> - SELECT a+100*(CAST(b IN (8,16,24) AS INTEGER)) FROM t1 ORDER BY b
> + SELECT a+100*(CASE WHEN b IN (8,16,24) THEN 1 ELSE 0 END) FROM t1 ORDER BY b
18. Same.
> ]], {
> -- <in-2.5>
> 1, 2, 103, 104, 5, 6, 7, 8, 9, 10
> @@ -207,7 +207,7 @@ test:do_execsql_test(
> test:do_execsql_test(
> "in-2.10",
> [[
> - SELECT a FROM t1 WHERE LEAST(0, CAST(b IN (a,30) AS INT)) <> 0
> + SELECT a FROM t1 WHERE LEAST(0, CASE WHEN (b IN (a,30)) THEN 1 ELSE 0 END) <> 0
19. I think a space should be after ',' in '(a,30)'.
> ]], {
> -- <in-2.10>
>
> @@ -253,7 +253,7 @@ test:do_execsql_test(
> test:do_execsql_test(
> "in-3.3",
> [[
> - SELECT a + 100*(CAST(b IN (SELECT b FROM t1 WHERE a<5) AS INTEGER)) FROM t1 ORDER BY b
> + SELECT a + 100*(CASE WHEN (b IN (SELECT b FROM t1 WHERE a<5)) THEN 1 ELSE 0 END) FROM t1 ORDER BY b
20. Same as 17.
> ]], {
> -- <in-3.3>
> 101, 102, 103, 104, 5, 6, 7, 8, 9, 10
> diff --git a/test/sql-tap/in4.test.lua b/test/sql-tap/in4.test.lua
> index 8442944b9..588daefec 100755
> --- a/test/sql-tap/in4.test.lua
> +++ b/test/sql-tap/in4.test.lua
> @@ -133,7 +133,7 @@ test:do_execsql_test(
> SELECT b FROM t2 WHERE a IN (1.0, 2.1)
> ]], {
> -- <in4-2.6>
> - "one"
> + "one", "two"
21. Why the result changed?
> -- </in4-2.6>
> })
>
> 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')
22. What does this test checks now and what it checked before?
> ]], {
> -- <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'
23. Since all these CAST are now fails, I think it is better to divide this
query into three. Otherwise only the first cast is checked.
> | ...
> 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'
24. Same.
> | ...
> -- 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'
> | ...
25. Same.
> 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'
> | ...
26. Same.
> SELECT cast('true' AS BOOLEAN), cast('false' AS BOOLEAN);
> | ---
> diff --git a/test/sql/types.result b/test/sql/types.result
> index 687ca3b15..90a8bc5ec 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);")
> ---
> @@ -1084,8 +1081,11 @@ box.execute("SELECT CAST(123 AS UNSIGNED);")
> ...
> box.execute("SELECT CAST(-123 AS UNSIGNED);")
> ---
> -- null
> -- 'Type mismatch: can not convert -123 to unsigned'
> +- metadata:
> + - name: COLUMN_1
> + type: unsigned
> + rows:
> + - [18446744073709551493]
> ...
27. Where does such behaviour is described?
> box.execute("SELECT CAST(1.5 AS UNSIGNED);")
> ---
> @@ -1097,19 +1097,13 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
> ...
> box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
> ---
> -- metadata:
> - - name: COLUMN_1
> - type: unsigned
> - rows:
> - - [-1]
> +- null
> +- 'Type mismatch: can not convert -1.5 to 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] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/3] sql: updated implicit conversion table
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 3/3] sql: updated implicit " Timur Safin via Tarantool-patches
@ 2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches
2021-06-28 0:06 ` Timur Safin via Tarantool-patches
0 siblings, 1 reply; 18+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-06-20 18:52 UTC (permalink / raw)
To: Timur Safin; +Cc: tarantool-patches
Thank you for the patch. See 33 comments below.
On Fri, Jun 11, 2021 at 10:48:13AM +0300, Timur Safin wrote:
> * Changed implicit casts table according to consistent types RFC
>
> * fixed following directions for implicit conversions
> - string to double
> - double to integer
> - varbinary to string
>
> * got rid of mem_cast_implicit_old() as unnecessary, use always
> the updated mem_cast_implicit()
>
> * in addition to update of all relevant tests there is extended
> e_casts.test.lua used for checking of implicit conversion rules:
>
> * there is table information sanity check - e_casts.test.lua checks
> that implicit table content is sane, i.e. it's sort of symmetric
> for all defined combinations [from, to]
>
> * for the test we use inserts in specially crafted table as checks
> availability of implicit conversions between type values
> * Temporary TCASTS table used with sequence primary key
> * All fields of a form `{name = "unsigned", type = "unsigned", is_nullable = true}`
> used for all enabled types
1. Since you said that you fix 'implicit cast', not just 'implicit cast for
insert', please fix all implicit casts. Also, your changes added
implicit cast
for comparison in some cases, even though it should be removed according
to RFC.
>
> Relates to #5910, #6009
> Closes #4470
>
> * Additionally we have fixed unexpected results for sum of implicitly
> converted numbers
2. I believe this change should be moved to its own patch.
>
> Fixing unexpected results after implicit conversion to
> the signed or unsigned integer from well-formed string.
>
> ```
> tarantool> box.execute([[select '1' + '2';]])
> ---
> - metadata:
> - name: COLUMN_1
> type: scalar
> rows:
> - [3]
> ...
>
> tarantool> box.execute([[select '1' + '-2';]])
> ---
> - metadata:
> - name: COLUMN_1
> type: scalar
> rows:
> - [18446744073709551615]
> ...
>
> tarantool> box.execute([[select '-1' + '-2';]])
> ---
> - null
> - 'Failed to execute SQL statement: integer is overflowed'
> ...
> ```
3. So, this is how it works now? I do not see any description.
>
> Fixes #5756
> ---
> src/box/sql/mem.c | 107 ++++-----------------
> src/box/sql/mem.h | 6 --
> src/box/sql/util.c | 3 +-
> src/box/sql/vdbe.c | 8 +-
> src/box/sql/vdbeaux.c | 2 +-
> test/sql-tap/e_casts.test.lua | 136 ++++++++++++++++++++++++++-
> test/sql-tap/in4.test.lua | 17 +++-
> test/sql-tap/numcast.test.lua | 2 +-
> test/sql-tap/tkt-9a8b09f8e6.test.lua | 40 ++++----
> test/sql/types.result | 104 ++++++++++----------
> 10 files changed, 248 insertions(+), 177 deletions(-)
>
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 8bb6b672c..d6b114a81 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -1033,95 +1033,24 @@ mem_cast_implicit(struct Mem *mem, enum field_type type)
> }
> switch (type) {
> case FIELD_TYPE_UNSIGNED:
> - if (mem->type == MEM_TYPE_UINT)
> - return 0;
> - if (mem->type == MEM_TYPE_DOUBLE)
> - return double_to_uint(mem);
> - return -1;
> - case FIELD_TYPE_STRING:
> - if (mem->type == MEM_TYPE_STR)
> - return 0;
> - if (mem->type == MEM_TYPE_UUID)
> - return uuid_to_str0(mem);
> - return -1;
> - case FIELD_TYPE_DOUBLE:
> - if (mem->type == MEM_TYPE_DOUBLE)
> - return 0;
> - if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
> - return int_to_double(mem);
> - return -1;
> - case FIELD_TYPE_INTEGER:
> if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
> return 0;
4. I tried to insert negative integer and I got this:
tarantool> box.execute('create table t (u unsigned primary key);')
---
- row_count: 1
...
tarantool> box.execute('insert into t values (-1);')
---
- null
- 'Tuple field 1 (U) type does not match one required by operation:
expected unsigned,
got integer'
...
Is it right?
> - if (mem->type == MEM_TYPE_DOUBLE)
> - return double_to_int(mem);
> - return -1;
> - case FIELD_TYPE_BOOLEAN:
> - if (mem->type == MEM_TYPE_BOOL)
> - return 0;
> - return -1;
> - case FIELD_TYPE_VARBINARY:
> - if ((mem->type & (MEM_TYPE_BIN | MEM_TYPE_MAP |
> - MEM_TYPE_ARRAY)) != 0)
> - return 0;
> - if (mem->type == MEM_TYPE_UUID)
> - return uuid_to_bin(mem);
> - return -1;
> - case FIELD_TYPE_NUMBER:
> - if (mem_is_num(mem))
> - return 0;
> - return -1;
> - case FIELD_TYPE_MAP:
> - if (mem->type == MEM_TYPE_MAP)
> - return 0;
> - return -1;
> - case FIELD_TYPE_ARRAY:
> - if (mem->type == MEM_TYPE_ARRAY)
> - return 0;
> - return -1;
> - case FIELD_TYPE_SCALAR:
> - if ((mem->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 0)
> - return -1;
> - return 0;
> - case FIELD_TYPE_UUID:
> - if (mem->type == MEM_TYPE_UUID)
> - return 0;
> - if (mem->type == MEM_TYPE_STR)
> - return str_to_uuid(mem);
> - if (mem->type == MEM_TYPE_BIN)
> - return bin_to_uuid(mem);
> - return -1;
> - case FIELD_TYPE_ANY:
> - return 0;
> - default:
> - break;
> - }
> - return -1;
> -}
> -
> -int
> -mem_cast_implicit_old(struct Mem *mem, enum field_type type)
> -{
> - if (mem->type == MEM_TYPE_NULL)
> - return 0;
> - switch (type) {
> - case FIELD_TYPE_UNSIGNED:
> - if (mem->type == MEM_TYPE_UINT)
> - return 0;
> if (mem->type == MEM_TYPE_DOUBLE)
> return double_to_uint_precise(mem);
> if (mem->type == MEM_TYPE_STR)
> return bytes_to_uint(mem);
> return -1;
> case FIELD_TYPE_STRING:
> - if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0)
> + if (mem->type == MEM_TYPE_STR)
> return 0;
> + if (mem->type == MEM_TYPE_UUID)
> + return uuid_to_str0(mem);
> if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
> return int_to_str0(mem);
> if (mem->type == MEM_TYPE_DOUBLE)
> return double_to_str0(mem);
> - if (mem->type == MEM_TYPE_UUID)
> - return uuid_to_str0(mem);
5. Why you moved these lines?
> + if (mem->type == MEM_TYPE_BIN)
> + return bin_to_str(mem);
> return -1;
> case FIELD_TYPE_DOUBLE:
> if (mem->type == MEM_TYPE_DOUBLE)
> @@ -1129,25 +1058,28 @@ mem_cast_implicit_old(struct Mem *mem, enum field_type type)
> if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
> return int_to_double(mem);
> if (mem->type == MEM_TYPE_STR)
> - return bin_to_str(mem);
> + return bytes_to_double(mem);
> return -1;
> case FIELD_TYPE_INTEGER:
> if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
> return 0;
> - if (mem->type == MEM_TYPE_STR)
> - return bytes_to_int(mem);
6. Same.
> if (mem->type == MEM_TYPE_DOUBLE)
> return double_to_int_precise(mem);
> + if (mem->type == MEM_TYPE_STR)
> + return bytes_to_int(mem);
> return -1;
> case FIELD_TYPE_BOOLEAN:
> if (mem->type == MEM_TYPE_BOOL)
> return 0;
> return -1;
> case FIELD_TYPE_VARBINARY:
> - if (mem->type == MEM_TYPE_BIN)
> + if ((mem->type & (MEM_TYPE_BIN | MEM_TYPE_MAP |
> + MEM_TYPE_ARRAY)) != 0)
7. What about this? Where it is described?
> return 0;
> if (mem->type == MEM_TYPE_UUID)
> return uuid_to_bin(mem);
> + if (mem->type == MEM_TYPE_STR)
> + return str_to_bin(mem);
> return -1;
> case FIELD_TYPE_NUMBER:
> if (mem_is_num(mem))
> @@ -1156,11 +1088,11 @@ mem_cast_implicit_old(struct Mem *mem, enum field_type type)
> return mem_to_number(mem);
> return -1;
> case FIELD_TYPE_MAP:
> - if (mem->type == MEM_TYPE_MAP)
> + if (mem_is_map(mem))
8. What is the point of this change? Please revert it.
> return 0;
> return -1;
> case FIELD_TYPE_ARRAY:
> - if (mem->type == MEM_TYPE_ARRAY)
> + if (mem_is_array(mem))
9. Same.
> return 0;
> return -1;
> case FIELD_TYPE_SCALAR:
> @@ -1175,6 +1107,8 @@ mem_cast_implicit_old(struct Mem *mem, enum field_type type)
> if (mem->type == MEM_TYPE_BIN)
> return bin_to_uuid(mem);
> return -1;
> + case FIELD_TYPE_ANY:
> + return 0;
> default:
> break;
> }
> @@ -1460,7 +1394,7 @@ get_number(const struct Mem *mem, struct sql_num *number)
> if (mem->type == MEM_TYPE_INT) {
> number->i = mem->u.i;
> number->type = MEM_TYPE_INT;
> - number->is_neg = true;
> + number->is_neg = mem->u.i < 0;
10. Why? All values of type MEM_TYPE_INT are negative by default.
> return 0;
> }
> if (mem->type == MEM_TYPE_UINT) {
> @@ -1473,13 +1407,6 @@ get_number(const struct Mem *mem, struct sql_num *number)
> return -1;
> if (sql_atoi64(mem->z, &number->i, &number->is_neg, mem->n) == 0) {
> number->type = number->is_neg ? MEM_TYPE_INT : MEM_TYPE_UINT;
> - /*
> - * The next line should be removed along with the is_neg field
> - * of struct sql_num. The integer type tells us about the sign.
> - * However, if it is removed, the behavior of arithmetic
> - * operations will change.
> - */
> - number->is_neg = false;
11. Why you did not removed 'is_neg' field from 'struct sql_number'?
What is the
point of having it there?
> return 0;
> }
> if (sqlAtoF(mem->z, &number->d, mem->n) != 0) {
> diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
> index b3cd5c545..706cad43c 100644
> --- a/src/box/sql/mem.h
> +++ b/src/box/sql/mem.h
> @@ -753,12 +753,6 @@ mem_cast_explicit(struct Mem *mem, enum field_type type);
> int
> mem_cast_implicit(struct Mem *mem, enum field_type type);
>
> -/**
> - * Convert the given MEM to given type according to legacy implicit cast rules.
> - */
> -int
> -mem_cast_implicit_old(struct Mem *mem, enum field_type type);
> -
> /**
> * Return value for MEM of INTEGER type. For MEM of all other types convert
> * value of the MEM to INTEGER if possible and return converted value. Original
> diff --git a/src/box/sql/util.c b/src/box/sql/util.c
> index c556b9815..69b1a3937 100644
> --- a/src/box/sql/util.c
> +++ b/src/box/sql/util.c
> @@ -958,9 +958,8 @@ sql_add_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg,
> *res = lhs + rhs;
> return 0;
> }
> - *is_res_neg = is_rhs_neg ? (uint64_t)(-rhs) > (uint64_t) lhs :
> - (uint64_t)(-lhs) > (uint64_t) rhs;
> *res = lhs + rhs;
> + *is_res_neg = *res < 0;
12. Is this right?
tarantool> box.execute('select 18446744073709551606+-1;')
---
- metadata:
- name: COLUMN_1
type: integer
rows:
- [-11]
...
tarantool> box.execute('select -1+18446744073709551606;')
---
- metadata:
- name: COLUMN_1
type: integer
rows:
- [-11]
...
Before this change:
tarantool> box.execute('select -1+18446744073709551606;')
---
- metadata:
- name: COLUMN_1
type: integer
rows:
- [18446744073709551605]
...
tarantool> box.execute('select 18446744073709551606+-1;')
---
- metadata:
- name: COLUMN_1
type: integer
rows:
- [18446744073709551605]
...
> return 0;
> }
>
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 32d02d96e..0dc28e2d6 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1660,7 +1660,7 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
> } else if (type == FIELD_TYPE_STRING) {
> if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) {
> const char *str =
> - mem_cast_implicit_old(pIn3, type) != 0 ?
> + mem_cast_implicit(pIn3, type) != 0 ?
13. Is this right?
tarantool> box.execute('create table t2(s string primary key);')
---
- row_count: 1
...
tarantool> box.execute([[insert into t2 values('0');]])
---
- row_count: 1
...
tarantool> box.execute('select * from t2 where s < 1;')
---
- metadata:
- name: S
type: string
rows:
- ['0']
...
> mem_str(pIn3) : mem_str(pIn1);
> diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
> "string");
> @@ -1671,7 +1671,7 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
> type = FIELD_TYPE_NUMBER;
> if (mem_cmp_num(pIn3, pIn1, &res) != 0) {
> const char *str =
> - mem_cast_implicit_old(pIn3, type) != 0 ?
> + mem_cast_implicit(pIn3, type) != 0 ?
14. Is this right?
tarantool> box.execute('create table t3(n number primary key);')
---
- row_count: 1
...
tarantool> box.execute([[insert into t3 values(0);]])
---
- row_count: 1
...
tarantool> box.execute([[select * from t3 where n < '1';]])
---
- metadata:
- name: N
type: number
rows:
- [0]
...
> mem_str(pIn3) : mem_str(pIn1);
> diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
> "numeric");
> @@ -1682,7 +1682,7 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
> assert(mem_is_str(pIn3) && mem_is_same_type(pIn3, pIn1));
> if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) {
> const char *str =
> - mem_cast_implicit_old(pIn3, type) != 0 ?
> + mem_cast_implicit(pIn3, type) != 0 ?
15. What changes after this?
> mem_str(pIn3) : mem_str(pIn1);
> diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
> "string");
> @@ -2218,7 +2218,7 @@ case OP_MakeRecord: {
> if (types != NULL) {
> pRec = pData0;
> do {
> - mem_cast_implicit_old(pRec++, *(types++));
> + mem_cast_implicit(pRec++, *(types++));
16. Same, what changes?
> } while(types[0] != field_type_MAX);
> }
>
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 4a1fdb637..c8e242c2f 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -2332,7 +2332,7 @@ sqlVdbeGetBoundValue(Vdbe * v, int iVar, u8 aff)
> sql_value *pRet = sqlValueNew(v->db);
> if (pRet) {
> mem_copy(pRet, pMem);
> - mem_cast_implicit_old(pRet, aff);
> + mem_cast_implicit(pRet, aff);
17. Same, what changes?
> }
> return pRet;
> }
> diff --git a/test/sql-tap/e_casts.test.lua b/test/sql-tap/e_casts.test.lua
> index 32d7e8e0c..952572921 100755
> --- a/test/sql-tap/e_casts.test.lua
> +++ b/test/sql-tap/e_casts.test.lua
> @@ -2,7 +2,7 @@
> local tap = require("tap")
> local test = tap.test("errno")
>
> -test:plan(1)
> +test:plan(3)
>
> local yaml = require("yaml")
> local ffi = require("ffi")
> @@ -137,6 +137,22 @@ local explicit_casts_table_spec = {
> [t_scalar] = {"Y", "S", "Y", "S", "S", "S", "S", "S", "S", "S", "" , "" , "Y"},
> }
>
> +local implicit_casts_table_spec = {
> + [t_any] = {"Y", "S", "S", "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", "" , "Y", "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"},
> + [t_varbinary]={"Y", "" , "Y", "" , "" , "" , "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", "" , "" , "" , "" , "" , "" , "" , "" , "" , "Y", "" , "" },
> + [t_map] = {"Y", "" , "" , "" , "" , "" , "" , "" , "" , "" , "" , "Y", "" },
> + [t_scalar] = {"Y", "S", "S", "S", "S", "S", "S", "S", "S", "S", "" , "" , "S"},
> +}
> +
> local c_no = 0
> local c_maybe = 1
> local c_yes = 2
> @@ -207,9 +223,31 @@ local function show_casts_table(table)
> end
>
> local explicit_casts = load_casts_spec(explicit_casts_table_spec, enabled_type, enabled_type_cast)
> +local implicit_casts = load_casts_spec(implicit_casts_table_spec, enabled_type, enabled_type)
>
> if verbose > 0 then
> show_casts_table(explicit_casts)
> + show_casts_table(implicit_casts)
> +end
> +
> +-- 0. check consistency of input conversion table
> +
> +-- 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 test_check_table_consistency(test)
> + test:plan(169)
> + for _, from in ipairs(proper_order) do
> + for _, to in ipairs(proper_order) do
> + test:ok((normalize_cast(implicit_casts[from][to]) ~= c_no) ==
> + (normalize_cast(implicit_casts[to][from]) ~= c_no),
> + label_for(from, to,
> + string.format("%s ~= %s",
> + implicit_casts[from][to],
> + implicit_casts[to][from])))
> + end
> + end
> end
>
> local function merge_tables(...)
> @@ -334,7 +372,103 @@ local function test_check_explicit_casts(test)
> end
> end
>
> +local table_name = 'TCASTS'
> +
> +local function _created_formatted_space(name)
> + local space = box.schema.space.create(name)
> + space:create_index('pk', {sequence = true})
> + local format = {{name = 'ID', type = 'unsigned', is_nullable = false}}
> + for _, type_id in ipairs(proper_order) do
> + if enabled_type[type_id] then
> + local type_name = type_names[type_id]
> + table.insert(format, {name = type_name, type = type_name, is_nullable = true})
> + end
> + end
> + if #format > 0 then
> + space:format(format)
> + end
> + return space
> +end
> +
> +local function _cleanup_space(space)
> + space:drop()
> +end
> +
> +-- implicit
> +local function gen_implicit_insert_from_to(table_name, from, to)
> + local queries = {}
> + local from_exprs = gen_type_exprs(from)
> + for _, from_e in pairs(from_exprs) do
> + table.insert(queries,
> + string.format([[ insert into %s("%s") values(%s); ]],
> + table_name, type_names[to], from_e))
> + end
> + return queries
> +end
> +
> +
> +-- 2. Check implicit casts table
> +local function test_check_implicit_casts(test)
> + test:plan(186)
> + local space = _created_formatted_space(table_name)
> + -- checking validity of all `from binop to` combinations
> + for _, from in ipairs(proper_order) do
> + for _, to in ipairs(proper_order) do
> + -- skip ANY, DECIMAL, UUID, etc.
> + if enabled_type[from] and enabled_type[to] then
> + local gen = gen_implicit_insert_from_to(table_name, from, to)
> + local failures = {}
> + local successes = {}
> + local castable = false
> + local expected = implicit_casts[from][to]
> +
> + if verbose > 0 then
> + print(expected, yaml.encode(gen))
> + end
> +
> + for _, v in pairs(gen) do
> + local ok, result
> + ok, result = catch_query(v)
> +
> + if verbose > 0 then
> + print(string.format("V> ok = %s, result = %s, query = %s",
> + ok, result, v))
> + end
> +
> + local title = string.format("%s => %s", v, human_cast(expected))
> + if expected == c_yes then
> + test:ok(true == ok, label_for(from, to, title))
> + elseif expected == c_no then
> + test:ok(false == ok, label_for(from, to, title))
> + 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
> + local title = string.format("%s => %s",
> + #gen and gen[1]..'...' or '',
> + human_cast(expected))
> + test:ok(castable, label_for(from, to, title),
> + failures)
> + end
> + end
> + end
> + end
> + _cleanup_space(space)
> +end
> +
> +test:test("e_casts - check consistency of implicit conversion table", test_check_table_consistency)
> test:test("e_casts - check explicit casts", test_check_explicit_casts)
> +test:test("e_casts - check implicit casts", test_check_implicit_casts)
18. Same comments as in previous letter.
>
> test:check()
> os.exit()
> diff --git a/test/sql-tap/in4.test.lua b/test/sql-tap/in4.test.lua
> index 588daefec..6aeaff5d5 100755
> --- a/test/sql-tap/in4.test.lua
> +++ b/test/sql-tap/in4.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> local test = require("sqltester")
> -test:plan(52)
> +test:plan(53)
>
> --!./tcltestrunner.lua
> -- 2008 September 1
> @@ -128,15 +128,26 @@ test:do_execsql_test(
> })
>
> test:do_execsql_test(
> - "in4-2.6",
> + "in4-2.6.0",
> [[
> - SELECT b FROM t2 WHERE a IN (1.0, 2.1)
> + SELECT b FROM t2 WHERE a IN (1.0, 2.0)
> ]], {
> -- <in4-2.6>
> "one", "two"
> -- </in4-2.6>
> })
>
> +-- FIXME - IN [2.1] should convert to expected type of a
> +test:do_execsql_test(
> + "in4-2.6.1",
> + [[
> + SELECT b FROM t2 WHERE a IN (1.0, 2.1)
> + ]], {
> + -- <in4-2.6.1>
> + "one"
> + -- </in4-2.6.1>
> + })
> +
19. In the previous patch you changed the result and now you changed the
test.
Why?
> test:do_execsql_test(
> "in4-2.7",
> [[
> diff --git a/test/sql-tap/numcast.test.lua b/test/sql-tap/numcast.test.lua
> index 6ca1316d5..798b2dc77 100755
> --- a/test/sql-tap/numcast.test.lua
> +++ b/test/sql-tap/numcast.test.lua
> @@ -139,7 +139,7 @@ test:do_catchsql_test(
> test:do_execsql_test(
> "cast-2.9",
> [[
> - INSERT INTO t VALUES(2.1);
> + INSERT INTO t VALUES(2.0);
> SELECT * FROM t;
> ]], {
> 2, 9223372036854775808ULL, 18000000000000000000ULL
> diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-9a8b09f8e6.test.lua
> index 67d6a1ccd..8ecf9f914 100755
> --- a/test/sql-tap/tkt-9a8b09f8e6.test.lua
> +++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua
> @@ -239,23 +239,23 @@ test:do_execsql_test(
> -- </4.2>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> 4.3,
> [[
> SELECT x FROM t3 WHERE x IN ('1');
> ]], {
> -- <4.3>
> - 1, "Type mismatch: can not convert 1 to number"
> + 1.0
20. Why this does not throw an error now? Where this behaviour is described?
> -- </4.3>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> 4.4,
> [[
> SELECT x FROM t3 WHERE x IN ('1.0');
> ]], {
> -- <4.4>
> - 1, "Type mismatch: can not convert 1.0 to number"
> + 1.0
21. Same.
> -- </4.4>
> })
>
> @@ -279,23 +279,23 @@ test:do_execsql_test(
> -- </4.6>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> 4.7,
> [[
> SELECT x FROM t3 WHERE '1' IN (x);
> ]], {
> -- <4.7>
> - 1, "Type mismatch: can not convert 1 to number"
> + 1.0
22. Same.
> -- </4.7>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> 4.8,
> [[
> SELECT x FROM t3 WHERE '1.0' IN (x);
> ]], {
> -- <4.8>
> - 1, "Type mismatch: can not convert 1.0 to number"
> + 1.0
23. Same.
> -- </4.8>
> })
>
> @@ -319,23 +319,23 @@ test:do_execsql_test(
> -- </5.2>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> 5.3,
> [[
> SELECT x FROM t4 WHERE x IN ('1');
> ]], {
> -- <5.3>
> - 1, "Type mismatch: can not convert 1 to number"
> +
24. Same.
> -- </5.3>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> 5.4,
> [[
> SELECT x FROM t4 WHERE x IN ('1.0');
> ]], {
> -- <5.4>
> - 1, "Type mismatch: can not convert 1.0 to number"
> +
25. Same.
> -- </5.4>
> })
>
> @@ -349,13 +349,13 @@ test:do_execsql_test(
> -- </5.5>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> 5.6,
> [[
> SELECT x FROM t4 WHERE x IN ('1.11');
> ]], {
> -- <5.6>
> - 1, "Type mismatch: can not convert 1.11 to number"
> + 1.11
26. Same.
> -- </5.6>
> })
>
> @@ -379,23 +379,23 @@ test:do_execsql_test(
> -- </5.8>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> 5.9,
> [[
> SELECT x FROM t4 WHERE '1' IN (x);
> ]], {
> -- <5.9>
> - 1, "Type mismatch: can not convert 1 to number"
> +
27. Same.
> -- </5.9>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> 5.10,
> [[
> SELECT x FROM t4 WHERE '1.0' IN (x);
> ]], {
> -- <5.10>
> - 1, "Type mismatch: can not convert 1.0 to number"
> +
28. Same.
> -- </5.10>
> })
>
> @@ -409,13 +409,13 @@ test:do_execsql_test(
> -- </5.11>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> 5.12,
> [[
> SELECT x FROM t4 WHERE '1.11' IN (x);
> ]], {
> -- <5.12>
> - 1, "Type mismatch: can not convert 1.11 to number"
> + 1.11
29. Same.
> -- </5.12>
> })
>
> diff --git a/test/sql/types.result b/test/sql/types.result
> index 90a8bc5ec..20dbd2073 100644
> --- a/test/sql/types.result
> +++ b/test/sql/types.result
> @@ -1059,7 +1059,8 @@ box.execute("INSERT INTO t1 VALUES (0), (1), (2);")
> box.execute("INSERT INTO t1 VALUES (-3);")
> ---
> - null
> -- 'Type mismatch: can not convert -3 to unsigned'
> +- 'Tuple field 1 (ID) type does not match one required by operation: expected unsigned,
> + got integer'
30. Why the error changed?
> ...
> box.execute("SELECT id FROM t1;")
> ---
> @@ -1224,12 +1225,13 @@ box.execute("INSERT INTO t VALUES(1, true);")
> ...
> box.execute("INSERT INTO t VALUES(1, 'asd');")
> ---
> -- null
> -- 'Type mismatch: can not convert asd to varbinary'
> +- row_count: 1
> ...
> box.execute("INSERT INTO t VALUES(1, x'616263');")
> ---
> -- row_count: 1
> +- null
> +- Duplicate key exists in unique index "pk_unnamed_T_1" in space "T" with old tuple
> + - [1, "asd"] and new tuple - [1, "abc"]
31. Can you avoid this error? For example by changing 1 to 2. This would
allow
to avoid some changes below.
> ...
> box.execute("SELECT * FROM t WHERE v = 1")
> ---
> @@ -1253,8 +1255,7 @@ box.execute("SELECT * FROM t WHERE v = x'616263'")
> type: integer
> - name: V
> type: varbinary
> - rows:
> - - [1, 'abc']
> + rows: []
> ...
> box.execute("SELECT sum(v) FROM t;")
> ---
> @@ -1277,7 +1278,7 @@ box.execute("SELECT min(v) FROM t;")
> - name: COLUMN_1
> type: scalar
> rows:
> - - ['abc']
> + - ['asd']
> ...
> box.execute("SELECT max(v) FROM t;")
> ---
> @@ -1285,7 +1286,7 @@ box.execute("SELECT max(v) FROM t;")
> - name: COLUMN_1
> type: scalar
> rows:
> - - ['abc']
> + - ['asd']
> ...
> box.execute("SELECT count(v) FROM t;")
> ---
> @@ -1301,7 +1302,7 @@ box.execute("SELECT group_concat(v) FROM t;")
> - name: COLUMN_1
> type: string
> rows:
> - - ['abc']
> + - ['asd']
> ...
> box.execute("SELECT lower(v) FROM t;")
> ---
> @@ -1332,7 +1333,7 @@ box.execute("SELECT quote(v) FROM t;")
> - name: COLUMN_1
> type: string
> rows:
> - - ['X''616263''']
> + - ['X''617364''']
> ...
> box.execute("SELECT LEAST(v, x'') FROM t;")
> ---
> @@ -1351,8 +1352,7 @@ box.execute("SELECT v FROM t WHERE v = x'616263';")
> - metadata:
> - name: V
> type: varbinary
> - rows:
> - - ['abc']
> + rows: []
> ...
> box.execute("SELECT v FROM t ORDER BY v;")
> ---
> @@ -1360,11 +1360,11 @@ box.execute("SELECT v FROM t ORDER BY v;")
> - name: V
> type: varbinary
> rows:
> - - ['abc']
> + - ['asd']
> ...
> box.execute("UPDATE t SET v = x'636261' WHERE v = x'616263';")
> ---
> -- row_count: 1
> +- row_count: 0
> ...
> box.execute("SELECT v FROM t;")
> ---
> @@ -1372,7 +1372,7 @@ box.execute("SELECT v FROM t;")
> - name: V
> type: varbinary
> rows:
> - - ['cba']
> + - ['asd']
> ...
> box.execute("CREATE TABLE parent (id INT PRIMARY KEY, a VARBINARY UNIQUE);")
> ---
> @@ -2206,8 +2206,9 @@ box.execute([[INSERT INTO ti(i) VALUES (true);]])
> ...
> box.execute([[INSERT INTO ti(i) VALUES ('33');]])
> ---
> -- null
> -- 'Type mismatch: can not convert 33 to integer'
> +- autoincrement_ids:
> + - 4
> + row_count: 1
> ...
> box.execute([[INSERT INTO ti(i) VALUES (X'3434');]])
> ---
> @@ -2225,6 +2226,7 @@ box.execute([[SELECT * FROM ti;]])
> - [1, null]
> - [2, 11]
> - [3, 33]
> + - [4, 33]
> ...
> box.execute([[INSERT INTO td(d) VALUES (NULL);]])
> ---
> @@ -2256,8 +2258,9 @@ box.execute([[INSERT INTO td(d) VALUES (true);]])
> ...
> box.execute([[INSERT INTO td(d) VALUES ('33');]])
> ---
> -- null
> -- 'Type mismatch: can not convert 33 to double'
> +- autoincrement_ids:
> + - 4
> + row_count: 1
> ...
> box.execute([[INSERT INTO td(d) VALUES (X'3434');]])
> ---
> @@ -2275,6 +2278,7 @@ box.execute([[SELECT * FROM td;]])
> - [1, null]
> - [2, 11]
> - [3, 22.2]
> + - [4, 33]
> ...
> box.execute([[INSERT INTO tb(b) VALUES (NULL);]])
> ---
> @@ -2327,13 +2331,15 @@ box.execute([[INSERT INTO tt(t) VALUES (NULL);]])
> ...
> box.execute([[INSERT INTO tt(t) VALUES (11);]])
> ---
> -- null
> -- 'Type mismatch: can not convert 11 to string'
> +- autoincrement_ids:
> + - 2
> + row_count: 1
> ...
> box.execute([[INSERT INTO tt(t) VALUES (22.2);]])
> ---
> -- null
> -- 'Type mismatch: can not convert 22.2 to string'
> +- autoincrement_ids:
> + - 3
> + row_count: 1
> ...
> box.execute([[INSERT INTO tt(t) VALUES (true);]])
> ---
> @@ -2343,13 +2349,14 @@ box.execute([[INSERT INTO tt(t) VALUES (true);]])
> box.execute([[INSERT INTO tt(t) VALUES ('33');]])
> ---
> - autoincrement_ids:
> - - 2
> + - 4
> row_count: 1
> ...
> box.execute([[INSERT INTO tt(t) VALUES (X'3434');]])
> ---
> -- null
> -- 'Type mismatch: can not convert varbinary to string'
> +- autoincrement_ids:
> + - 5
> + row_count: 1
> ...
> box.execute([[SELECT * FROM tt;]])
> ---
> @@ -2360,7 +2367,10 @@ box.execute([[SELECT * FROM tt;]])
> type: string
> rows:
> - [1, null]
> - - [2, '33']
> + - [2, '11']
> + - [3, '22.2']
> + - [4, '33']
> + - [5, '44']
> ...
> box.execute([[INSERT INTO tv(v) VALUES (NULL);]])
> ---
> @@ -2385,13 +2395,14 @@ box.execute([[INSERT INTO tv(v) VALUES (true);]])
> ...
> box.execute([[INSERT INTO tv(v) VALUES ('33');]])
> ---
> -- null
> -- 'Type mismatch: can not convert 33 to varbinary'
> +- autoincrement_ids:
> + - 2
> + row_count: 1
32. Why this does not fails? I think RFC says this this cast is not allowed.
> ...
> box.execute([[INSERT INTO tv(v) VALUES (X'3434');]])
> ---
> - autoincrement_ids:
> - - 2
> + - 3
> row_count: 1
> ...
> box.execute([[SELECT * FROM tv;]])
> @@ -2403,7 +2414,8 @@ box.execute([[SELECT * FROM tv;]])
> type: varbinary
> rows:
> - [1, null]
> - - [2, '44']
> + - [2, '33']
> + - [3, '44']
> ...
> box.execute([[INSERT INTO ts(s) VALUES (NULL);]])
> ---
> @@ -2459,11 +2471,11 @@ box.execute([[SELECT * FROM ts;]])
> -- Check for UPDATE.
> box.execute([[DELETE FROM ti;]])
> ---
> -- row_count: 3
> +- row_count: 4
> ...
> box.execute([[DELETE FROM td;]])
> ---
> -- row_count: 3
> +- row_count: 4
> ...
> box.execute([[DELETE FROM tb;]])
> ---
> @@ -2471,11 +2483,11 @@ box.execute([[DELETE FROM tb;]])
> ...
> box.execute([[DELETE FROM tt;]])
> ---
> -- row_count: 2
> +- row_count: 5
> ...
> box.execute([[DELETE FROM tv;]])
> ---
> -- row_count: 2
> +- row_count: 3
> ...
> box.execute([[DELETE FROM ts;]])
> ---
> @@ -2559,8 +2571,7 @@ box.execute([[UPDATE ti SET i = true WHERE a = 1;]])
> ...
> box.execute([[UPDATE ti SET i = '33' WHERE a = 1;]])
> ---
> -- null
> -- 'Type mismatch: can not convert 33 to integer'
> +- row_count: 1
> ...
> box.execute([[UPDATE ti SET i = X'3434' WHERE a = 1;]])
> ---
> @@ -2600,8 +2611,7 @@ box.execute([[UPDATE td SET d = true WHERE a = 1;]])
> ...
> box.execute([[UPDATE td SET d = '33' WHERE a = 1;]])
> ---
> -- null
> -- 'Type mismatch: can not convert 33 to double'
> +- row_count: 1
> ...
> box.execute([[UPDATE td SET d = X'3434' WHERE a = 1;]])
> ---
> @@ -2616,7 +2626,7 @@ box.execute([[SELECT * FROM td;]])
> - name: D
> type: double
> rows:
> - - [1, 22.2]
> + - [1, 33]
> ...
> box.execute([[UPDATE tb SET b = NULL WHERE a = 1;]])
> ---
> @@ -2662,13 +2672,11 @@ box.execute([[UPDATE tt SET t = NULL WHERE a = 1;]])
> ...
> box.execute([[UPDATE tt SET t = 11 WHERE a = 1;]])
> ---
> -- null
> -- 'Type mismatch: can not convert 11 to string'
> +- row_count: 1
> ...
> box.execute([[UPDATE tt SET t = 22.2 WHERE a = 1;]])
> ---
> -- null
> -- 'Type mismatch: can not convert 22.2 to string'
> +- row_count: 1
> ...
> box.execute([[UPDATE tt SET t = true WHERE a = 1;]])
> ---
> @@ -2681,8 +2689,7 @@ box.execute([[UPDATE tt SET t = '33' WHERE a = 1;]])
> ...
> box.execute([[UPDATE tt SET t = X'3434' WHERE a = 1;]])
> ---
> -- null
> -- 'Type mismatch: can not convert varbinary to string'
> +- row_count: 1
> ...
> box.execute([[SELECT * FROM tt;]])
> ---
> @@ -2692,7 +2699,7 @@ box.execute([[SELECT * FROM tt;]])
> - name: T
> type: string
> rows:
> - - [1, '33']
> + - [1, '44']
> ...
> box.execute([[UPDATE tv SET v = NULL WHERE a = 1;]])
> ---
> @@ -2715,8 +2722,7 @@ box.execute([[UPDATE tv SET v = true WHERE a = 1;]])
> ...
> box.execute([[UPDATE tv SET v = '33' WHERE a = 1;]])
> ---
> -- null
> -- 'Type mismatch: can not convert 33 to varbinary'
> +- row_count: 1
33. Same.
> ...
> box.execute([[UPDATE tv SET v = X'3434' WHERE a = 1;]])
> ---
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines Timur Safin via Tarantool-patches
@ 2021-06-20 18:57 ` Igor Munkin via Tarantool-patches
2021-06-23 21:01 ` Alexander Turenko via Tarantool-patches
2021-06-27 23:16 ` Timur Safin via Tarantool-patches
0 siblings, 2 replies; 18+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-06-20 18:57 UTC (permalink / raw)
To: Timur Safin; +Cc: alexander.turenko, tarantool-patches
Timur,
Thanks for the patch! Though the change is quite trivial, the patch is
not OK right from the top. Please adjust the commit subject according to
our contribution guidelines[1].
Regarding the changes itself: why did you send it within absolutely
unrelated changeset? This is not required for the following patches.
This is not related to SQL at all. This patch should be on the another
branch, and the next patches should be rebased on that branch (if you
need it so much).
You can argue that this is a trivial oneliner and I am making lot of ado
about nothing, *but* this is your point of view. And now let me share
mine one with you:
* This patch should be backported to all longterm branches, since I
consider this as a bug. Since this patch is on the bottom of the
series, I need to make more actions for this. For what? I see no
reason for it.
* You've chosen Sasha as a *first* reviewer for the first patch of the
series. However, I'm sure every member of our "L" team can carefully
review such a simple fix. Hence, you decided to stall the whole series
until Sasha approves this tiny patch?
* On the other hand if this patch has enough LGTMs, it can be applied
"out of order". And when it is applied, you will need to rebase your
working branch with SQL-related changes on master. As a result this
*optimization* has no sense at all and only makes maintainer's life
much worse.
Finally, if there was not a nit regarding this patch, I would suggest
you a deal: leave the patch as is and consider everything written above
for the further patches. However, there are several nits to be resolved,
so please move this patch out from this series in a separate one.
On 11.06.21, Timur Safin via Tarantool-patches wrote:
> It always was a problem that reported source line was not
> pointing to the actual callee line number, but rather to
Don't get why you are talking about callee here: regardless the line to
be reported (the current one or the one where the function is defined),
it is a *caller* for the <traceback> function.
So, the actual problem you're writing is that the line with function
signature is used instead of the line where the function execution is
stopped at the moment of the <traceback> call.
As for me, these are the different reasons.
> the start of file, i.e. we have seen:
> ```
> [001] sql-tap/tkt-9a8b09f8e6.test.lua memtx
> [001] not ok 22 - 4.3 #
> [001] Traceback:
> [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:123>
> [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:0>
> [001]
> [001] not ok 23 - 4.4 #
> [001] Traceback:
> [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:123>
> [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:0>
> ```
> (see the :0 part)
Minor: Strictly saying :123 part is also broken. The only difference
between them is that :0 line is used for so called "main" function.
> Instead of correct line numbers:
> ```
> [001] sql-tap/tkt-9a8b09f8e6.test.lua memtx
> [001] not ok 22 - 4.3 #
> [001] Traceback:
> [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:142>
> [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:242>
> [001]
> [001] not ok 23 - 4.4 #
> [001] Traceback:
> [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:142>
> [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:252>
> ```
>
> The problem was due to `.linedefined` used, instead of source line in `.currentline`.
Typo: The line exceeds 72 chars.
>
> Closes #6134
> ---
> src/lua/tap.lua | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/lua/tap.lua b/src/lua/tap.lua
> index 346724d84..77fd8d096 100644
> --- a/src/lua/tap.lua
> +++ b/src/lua/tap.lua
> @@ -23,7 +23,7 @@ local function traceback(level)
> local frame = {
> source = info.source;
> src = info.short_src;
> - line = info.linedefined or 0;
> + line = info.currentline or info.linedefined or 0;
Why did you leave such a complex code here? I believe you can use just
info.currentline here.
> what = info.what;
> name = info.name;
> namewhat = info.namewhat;
> --
> 2.29.2
>
[1]: https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/#how-to-write-a-commit-message
--
Best regards,
IM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines
2021-06-20 18:57 ` Igor Munkin via Tarantool-patches
@ 2021-06-23 21:01 ` Alexander Turenko via Tarantool-patches
2021-06-27 23:16 ` Timur Safin via Tarantool-patches
1 sibling, 0 replies; 18+ messages in thread
From: Alexander Turenko via Tarantool-patches @ 2021-06-23 21:01 UTC (permalink / raw)
To: Timur Safin; +Cc: tarantool-patches
Good catch!
All comments from Igor look meaningful for me. After fixes according to
those comments -- no objections from me.
> <..> Please adjust the commit subject according to
> our contribution guidelines[1].
I would highlight that the 'test' prefix is used for pure testing
changes, but here we touch the built-in module. I would use 'lua' here.
WBR, Alexander Turenko.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/3] sql: updated explicit conversion table
2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches
@ 2021-06-25 21:26 ` Timur Safin via Tarantool-patches
2021-06-25 21:26 ` [Tarantool-patches] Отзыв: " Timur Safin via Tarantool-patches
1 sibling, 0 replies; 18+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-25 21:26 UTC (permalink / raw)
To: 'Mergen Imeev'; +Cc: tarantool-patches
: From: Mergen Imeev <imeevma@tarantool.org>
: Subject: Re: [PATCH v2 2/3] sql: updated explicit conversion table
:
: Thank you for the fixes you did. See my 27 comments below.
:
: 1. In general I do not think that it is good idea to squash all previous
: three
: patches to one patch. I think that the first of previous patches was
: quite good,
: the second was useless and the third had some issues, but now all these
: patches
: are squashed.
The idea was to split modification to implicit and explicit changes.
And taking into account that changes in behavior should be at the same
commit with modified tests those patches grew a bit. If we split
code and test then commits are not passing build/test separately.
:
: On Fri, Jun 11, 2021 at 10:48:12AM +0300, Timur Safin wrote:
: > * fixes for `BOOLEAN` expressions in explicit converstion tables
: >
: > 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 still do not think that path to RFC is needed here.
It's matter of taste.
: Also, what about '.'?
They mark unchanged cell entries (as I said). Or I've not understood your clearly.
: > * introduce `ANY` as target for explicit conversions
: >
: > For consistency sake it was decided to provide
: > `CAST(anything TO ANY)` as allowed, but still noop.
: >
: 3. I still do not like that you added cast to unsupported field.
That was part of RFC we all agreed upon, not my own invention.
:
: 4. Why typeof() of value cast to any returns 'any'?
: tarantool> box.execute('select typeof(cast(1 AS any));')
: ---
: - metadata:
: - name: COLUMN_1
: type: string
: rows:
: - ['any']
: ...
:
: tarantool> box.execute('select typeof(cast(1 AS scalar));')
: ---
: - metadata:
: - name: COLUMN_1
: type: string
: rows:
: - ['integer']
: ...
Good observation, thanks! `cast(1 as scalar)` should return expression
attributed as `scalar` also. (Because why this cast then? Values should
be left untouched, only type of expression reattributed.
:
: > * We had to rename %wildcard token to ANYTHING, since we needed
: > token ANY for real life usage.
: >
: 5. Just a thought - why you cannot use 'ANYTHING' for field type ANY?
: Or 'ANY_KW', how it was already done for INTEGER?
As %wildcard token we should use separate lexem, not actually used.
:
: > * As a byproduct, we fixed #6010 to make cast to unsigned behaving
: reasonably
: >
: 6. I think this change is good enough to have its own patch.
It's still part of implicit casts fixes, and affect some(same) set of tests.
[i.e. those tests will be modified multiple times along a patches flow in
The same patchset]
But yes, in this case there was separate ticket, withc specific test-case,
Thus it's easy to extract.
:
: > * To make sure that all consistency checks are systematic, we have
: introduced
: > special test for checking conversion rules - e_casts.test.lua. This
: > patch introduces explicit table part:
: >
: > * e_casts.test.lua is generating expressions of `CAST(input AS output)`
: > form. All combinations check whether we expectedly succeeded or
: failed,
: > given the explicit conversion table from RFC 5910-consistent-sql-lua-
: types.md;
: >
: > * At the moment we skip DECIMALs, ARRAYs and MAPs as not yet
: > fully supported. Need to be revisited later;
: >
: > * NB! there is `-verbose` mode one may activate to enable more detailed
: > reporting during debugging.
: >
: 7. I used './test-run.py sql-tap/e_casts.test.lua --verbose -j1' and saw
: this:
: [001] sql-tap/e_casts.test.lua memtx [001] TAP
: version 13
: [001] 1..3
: [001] # e_casts - check consistency of implicit conversion table
: [001] 1..169
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [any,any] nil ~= nil
...
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,number] 2 ~= 1
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,decimal] nil ~= nil
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,uuid] nil ~= nil
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,array] nil ~= nil
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,map] nil ~= nil
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,scalar] 2 ~= 1
: ...
:
: What is it?
Have you looked into code of this test? In this subtest we check that
implicit conversion table is symmetric in their cells (i.e. if there is
any sort of conversion from type A to B then there have to be
some kind sort of conversion reverse conversion from type B to A)
Agreed though, that 2 != 1, and especially nil ~= nil, look confusing
without looking into sources, and will provide more human-readable
output there.
:
: > Relates to #5910, #6009
: > Part of #4470
: > Fixes #6010
: >
: > +int
: > +mem_to_uint(struct Mem *mem)
: > +{
: > + assert(mem->type < MEM_TYPE_INVALID);
: > + if (mem->type == MEM_TYPE_UINT)
: > + return 0;
: > + if (mem->type == MEM_TYPE_INT) {
: > + mem_set_uint(mem, (uint64_t)mem->u.i);
: 8. Could you explain to me, what it this? Since when -1 can be cast to
: UNSIGNED?
This is rather usual reinterpret_cast<> for conversion of
negative signed integer to unsigned.
: Where it is described? Why this is allowed?
: tarantool> box.execute('select CAST(-1 as unsigned);')
: ---
: - metadata:
: - name: COLUMN_1
: type: unsigned
: rows:
: - [18446744073709551615]
: ...
Now looking into RFC table for explicit casts I do see it's
slightly extension of an agreed upon "conditional" conversion
(i.e. if signed integer has positive value then it should be
Convertible, and raise error otherwise).
So yes, thanks for catch!
:
:
: > + return 0;
: > + }
: > + if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0)
: > + return bytes_to_uint(mem);
: > + if (mem->type == MEM_TYPE_DOUBLE)
: > + return double_to_uint(mem);
: > + return -1;
: > +}
: > +
: > int
: > mem_to_int(struct Mem *mem)
: > {
: > assert(mem->type < MEM_TYPE_INVALID);
: > - if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
: > + if (mem->type == MEM_TYPE_INT)
: > + return 0;
: > + if (mem->type == MEM_TYPE_UINT) {
: > + mem_set_int(mem, (int64_t)mem->u.u, false);
: 9. So, you want to set unsigned value as integer. Could you tell me what
: does
: this change do?
At least it will change type attribution (that's important
Because of different allowed ranges of values).
: > @@ -1017,13 +989,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type
: type)
: > switch (mem->type) {
: 10. Just a thought. I see no harm to have this switch here, but it has
: only two
: options. Why not change it to 'if'?
Wanted to say that I kept switch to look consistent with code above and
below, but then realized that more complicated code with switch cases
has already been refactored to functions and this switch left alone,
and only if left around it.
Yep, will switch switch to if. That would look better.
...
: > diff --git a/test/sql-tap/e_casts.test.lua b/test/sql-tap/e_casts.test.lua
: > new file mode 100755
: > index 000000000..32d7e8e0c
: > --- /dev/null
: > +++ b/test/sql-tap/e_casts.test.lua
: > @@ -0,0 +1,340 @@
: > +#!/usr/bin/env tarantool
: > +local tap = require("tap")
: > +local test = tap.test("errno")
: > +
: > +test:plan(1)
: > +
: > +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
: > + };
: > +]]
: 11. Since this cdef is only copy-paste from current actual enum, I still see
: not reason to keep it here. Or you could use one from module.h, if you want
: to keep it.
Nope, we could not use module.h - it's been built much, much later in the
current build flow. Module_api and testing are currently independent targets.
and this unnecessary dependency looks redundant, copy-pastes are bad, but
sometimes they are excusable to avoid introduction of unnecessary troubles (in
already quite fragile build process)
: > +
: > +-- 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] = true,
: > + -- [t_date] = false,
: > + -- [t_time] = false,
: > + -- [t_timestamp]= false,
: > + -- [t_interval] = False,
: > + [t_array] = false,
: > + [t_map] = false,
: > + [t_scalar] = true,
: > +}
: > +
: > +-- Enabled types which may be targets for explicit casts
: > +local enabled_type_cast = table.deepcopy(enabled_type)
: > +enabled_type_cast[t_any] = true
: > +
: > +-- table of _TSV_ (tab separated values)
: > +-- copied from sql-lua-tables-v5.xls
: > +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"},
: > +}
: 12. Again, why there is 'N' and '' both in table? Could you show me where in
: RFC we have this table that contains both '' and 'N'?
This is also explained with - because of RFC! ;) Kinda. In this case that's the way
it looked in frozen materials (excel) used for RFC generation which I've published
as attachments to https://github.com/tarantool/tarantool/wiki/SQL-Lua-Consistent-Type-Specification
wiki page. OTOH ' ' and 'N' are equivalent, and could be easily massaged in the editor
so to make it look less confusing for others I'll change them to always be ' '.
...
: > +
: > + for _, from in ipairs(proper_order) do
: > + local line = ''
: > + for _, 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
: 13. Since you use '-verbose' mode only in your debug, can you move these
: functions out from here? I still do not see any use of this code.
Out from here to where? I do print implicit and explicit tables content
if ran with '--verbose' option. And this is great debugging helper while
you modify those tables.
Have you ever run this test outside of test-run infrastructure? Like
```
✔ ~/tarantool/test
18:57 $ cat ~/bin/sql-tap-run.sh
#/usr/bin/bash
root=$(git rev-parse --show-toplevel)
rm -f *.snap *.xlog
LUA_PATH="$root/test/sql-tap/lua/?.lua;$root/test/sql/lua/?.lua;" \
$prefix $root/build/src/tarantool $*
✔ ~/tarantool/test
18:57 $ ~/bin/sql-tap-run.sh sql-tap/e_casts.test.lua --verbose |& 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 | | | | | Y |
2. string | Y | S | Y | S | S | S | Y | S | | S | | | Y |
4. double | Y | S | Y | Y | S | | | Y | | | | | Y |
5. integer | Y | S | Y | Y | Y | | | Y | | | | | Y |
6. boolean | Y | | Y | | | Y | | | | | | | Y |
7. varbinary | Y | | Y | | | | Y | | | S | | | Y |
3. number | Y | S | Y | Y | S | | | Y | | | | | Y |
9. decimal | | | | | | | | | | | | | |
10. uuid | Y | | Y | | | | Y | | | Y | | | Y |
11. array | | | | | | | | | | | | | |
12. map | | | | | | | | | | | | | |
8. scalar | Y | S | Y | S | 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 | | 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 | | | S | | | Y |
3. number | | S | Y | Y | S | | | Y | | | | | Y |
9. decimal | | | | | | | | | | | | | |
10. uuid | | | Y | | | | Y | | | Y | | | Y |
11. array | | | | | | | | | | | | | |
12. map | | | | | | | | | | | | | |
8. scalar | | S | 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:245 [any,any] nil ~= nil
ok - sql-tap/e_casts.test.lua:245 [any,unsigned] nil ~= nil
ok - sql-tap/e_casts.test.lua:245 [any,string] nil ~= nil
ok - sql-tap/e_casts.test.lua:245 [any,double] nil ~= nil
```
Ain't them pretty? ;)
: > +
: > +local gen_type_samples = {
: > + [t_unsigned] = {"0", "1", "2"},
: 14. I believe you once said something about testing values that are close to
: all limits of numeric types. Why I cannot see them here?
This is very good point - I'll add some boundary values.
: > +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
: 15. Since you use all these functions and do not want to remove them, I
: suggest
: to add comments to them.
Yeah, these are much easier to use than those we have in sqltester.lua.
Will provide some comments.
:
: > +
: > +-- 1. Check explicit casts table
: > +local function test_check_explicit_casts(test)
: > + -- checking validity of all `CAST(from AS to)` combinations
: > + test:plan(322)
: > + for _, from in ipairs(proper_order) do
: > + for _, to in ipairs(proper_order) do
: > + -- skip ANY, DECIMAL, UUID, etc.
: 16. Why UUID is skipped? Also, why ANY is skipped?
No, in this test we do not skip uuids, they already been integrated
into test. That's rather stale/outdated comment.
: You can get values of type
: ANY by using CAST(values AS ANY), I believe. Or I am wrong?
Yes and no. Given expression of a form `CAST(x as ANY)` we do have
expression attributed with type ANY, but there is still no direct
way to write literal of type ANY.
Having said that we indeed could use more complicated expressions,
not only literals. So yes, we may use such expressions.
[But today I'm not actually certain we will not disable these kinds
of casts entirely, despite them agreed in the RFC. Along with all
possible changes many want to introduce for scalars. So let see
where we will go in the nearest future. Sigh. ]
:
: > + if enabled_type[from] and enabled_type_cast[to] then
: > + local gen = gen_explicit_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
: > +
: > +
: > + -- ok, we aggregated stats for c_maybe mode - check it
: now
: > + if expected == c_maybe then
: > + local title = string.format("%s => %s",
: > + #gen and
: gen[1]..'...' or '',
: > + human_cast(expected))
: > + test:ok(castable, label_for(from, to, title),
: > + failures)
: > + end
: 16. I already said my expectations about these tests. Since you know
: (and print)
: all values used in the test, why cannot you determine if the cast should
: fail?
: For example, this test is actually useless if `CAST(1.0 AS UNSIGNED)`
: fails, since
: this cast is marked as 'sometimes'. If you do so, you can just print 'Y'
: or 'N'.
: Also, why your tests does not check unallowed casts? Or, at least, they
: are not
: shown.
It's actually testing entire table (with only exception of unsupported
today types).
You see the line?
local expected = explicit_casts[from][to]
and then
if expected == c_yes then
test:ok(true == ok, label_for(from, to, title))
elseif expected == c_no then
test:ok(false == ok, label_for(from, to, title))
else
The problem is in TAP reporting - I did not find any way to have passing
test but have expected failures. They all look identical now, all looks
castable.
Will modify label_for() as promised above to clearly indicate that
some expectedly failed.
: > --- a/test/sql-tap/in1.test.lua
: > +++ b/test/sql-tap/in1.test.lua
: > @@ -100,7 +100,7 @@ test:do_execsql_test(
: > test:do_execsql_test(
: > "in-1.7",
: > [[
: > - SELECT a+ 100*CAST((a BETWEEN 1 and 3) AS INTEGER) FROM t1 ORDER
: BY b
: > + SELECT a+ 100*(CASE WHEN (a BETWEEN 1 and 3) THEN 1 ELSE 0 END)
: FROM t1 ORDER BY b
: 17. Since you already change this line, add spaces before and after all
: operations with two operands.
Hmm, ok
:
: > ]], {
: > -- <in-1.7>
: > 101, 102, 103, 4, 5, 6, 7, 8, 9, 10
: > @@ -157,7 +157,7 @@ test:do_execsql_test(
: > test:do_execsql_test(
: > "in-2.5",
: > [[
: > - SELECT a+100*(CAST(b IN (8,16,24) AS INTEGER)) FROM t1 ORDER BY b
: > + SELECT a+100*(CASE WHEN b IN (8,16,24) THEN 1 ELSE 0 END) FROM t1
: ORDER BY b
: 18. Same.
Ok
:
: > ]], {
: > -- <in-2.5>
: > 1, 2, 103, 104, 5, 6, 7, 8, 9, 10
: > @@ -207,7 +207,7 @@ test:do_execsql_test(
: > test:do_execsql_test(
: > "in-2.10",
: > [[
: > - SELECT a FROM t1 WHERE LEAST(0, CAST(b IN (a,30) AS INT)) <> 0
: > + SELECT a FROM t1 WHERE LEAST(0, CASE WHEN (b IN (a,30)) THEN 1
: ELSE 0 END) <> 0
: 19. I think a space should be after ',' in '(a,30)'.
Ok
: > diff --git a/test/sql-tap/in4.test.lua b/test/sql-tap/in4.test.lua
: > index 8442944b9..588daefec 100755
: > --- a/test/sql-tap/in4.test.lua
: > +++ b/test/sql-tap/in4.test.lua
: > @@ -133,7 +133,7 @@ test:do_execsql_test(
: > SELECT b FROM t2 WHERE a IN (1.0, 2.1)
: > ]], {
: > -- <in4-2.6>
: > - "one"
: > + "one", "two"
: 21. Why the result changed?
Good question! You see I've added case when implicit cast from double to
integer worked expectedly and brought us 2 rows, but here, by some reason
implicit cast from 2.1 didn't match integer 2. I need to investigate
reasons, at the moment I do not recollect why it was ok for me.
:
: > -- </in4-2.6>
: > })
: >
: > 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')
: 22. What does this test checks now and what it checked before?
That was regression test for SQLite bug #668
```
-- Ticket #668: VDBE stack overflow occurs when the left-hand side
-- of an IN expression is NULL and the result is used as an integer, not
-- as a jump.
```
:
: > ]], {
: > -- <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'
: 23. Since all these CAST are now fails, I think it is better to divide this
: query into three. Otherwise only the first cast is checked.
:
: > | ...
: > 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'
: 24. Same.
:
: > | ...
: > -- 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'
: > | ...
: 25. Same.
:
: > 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'
: > | ...
: 26. Same.
:
: > SELECT cast('true' AS BOOLEAN), cast('false' AS BOOLEAN);
: > | ---
: > diff --git a/test/sql/types.result b/test/sql/types.result
: > index 687ca3b15..90a8bc5ec 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);")
: > ---
: > @@ -1084,8 +1081,11 @@ box.execute("SELECT CAST(123 AS UNSIGNED);")
: > ...
: > box.execute("SELECT CAST(-123 AS UNSIGNED);")
: > ---
: > -- null
: > -- 'Type mismatch: can not convert -123 to unsigned'
: > +- metadata:
: > + - name: COLUMN_1
: > + type: unsigned
: > + rows:
: > + - [18446744073709551493]
: > ...
: 27. Where does such behaviour is described?
:
: > box.execute("SELECT CAST(1.5 AS UNSIGNED);")
: > ---
: > @@ -1097,19 +1097,13 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
: > ...
: > box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
: > ---
: > -- metadata:
: > - - name: COLUMN_1
: > - type: unsigned
: > - rows:
: > - - [-1]
: > +- null
: > +- 'Type mismatch: can not convert -1.5 to 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] 18+ messages in thread
* [Tarantool-patches] Отзыв: [PATCH v2 2/3] sql: updated explicit conversion table
@ 2021-06-25 21:26 ` Timur Safin via Tarantool-patches
2021-06-27 23:46 ` [Tarantool-patches] " Timur Safin via Tarantool-patches
0 siblings, 1 reply; 18+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-25 21:26 UTC (permalink / raw)
To: 'Mergen Imeev'; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 113 bytes --]
оНКЭГНБЮРЕКЭ Timur Safin УНРЕК АШ НРНГБЮРЭ ЯННАЫЕМХЕ, "[PATCH v2 2/3] sql:
updated explicit conversion table".
[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 1693 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines
2021-06-20 18:57 ` Igor Munkin via Tarantool-patches
2021-06-23 21:01 ` Alexander Turenko via Tarantool-patches
@ 2021-06-27 23:16 ` Timur Safin via Tarantool-patches
2021-06-29 16:21 ` Igor Munkin via Tarantool-patches
1 sibling, 1 reply; 18+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-27 23:16 UTC (permalink / raw)
To: 'Igor Munkin'; +Cc: alexander.turenko, tarantool-patches
: From: Igor Munkin <imun@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported
: error lines
:
: Timur,
:
: Thanks for the patch! Though the change is quite trivial, the patch is
: not OK right from the top. Please adjust the commit subject according to
: our contribution guidelines[1].
Much thanks Sasha for his useful proposal, not thanks to Igor for useless
referring to guidelines without suggestion.
...
: You can argue that this is a trivial oneliner and I am making lot of ado
: about nothing,
Exactly.
...
:
: However, there are several nits to be resolved,
: so please move this patch out from this series in a separate one.
Will do
:
: On 11.06.21, Timur Safin via Tarantool-patches wrote:
: > It always was a problem that reported source line was not
: > pointing to the actual callee line number, but rather to
:
: Don't get why you are talking about callee here: regardless the line to
: be reported (the current one or the one where the function is defined),
: it is a *caller* for the <traceback> function.
Yes, that was caller source line I was referring to. Thanks for correction.
:
: So, the actual problem you're writing is that the line with function
: signature is used instead of the line where the function execution is
: stopped at the moment of the <traceback> call.
:
: As for me, these are the different reasons.
:
: > the start of file, i.e. we have seen:
: > ```
: > [001] sql-tap/tkt-9a8b09f8e6.test.lua memtx
: > [001] not ok 22 - 4.3 #
: > [001] Traceback:
: > [001] [Lua ] function 'do_catchsql_test' at
: </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:123>
: > [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-
: 9a8b09f8e6.test.lua:0>
: > [001]
: > [001] not ok 23 - 4.4 #
: > [001] Traceback:
: > [001] [Lua ] function 'do_catchsql_test' at
: </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:123>
: > [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-
: 9a8b09f8e6.test.lua:0>
: > ```
: > (see the :0 part)
:
: Minor: Strictly saying :123 part is also broken.
O_o, why? Could you please clarify? We report it as line number in the
source file, in this case tkt-9a8b09f8e6.test.lua:123 was exactly the
caller location?
: The only difference
: between them is that :0 line is used for so called "main" function.
:
: > Instead of correct line numbers:
: > ```
: > [001] sql-tap/tkt-9a8b09f8e6.test.lua memtx
: > [001] not ok 22 - 4.3 #
: > [001] Traceback:
: > [001] [Lua ] function 'do_catchsql_test' at
: </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:142>
: > [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-
: 9a8b09f8e6.test.lua:242>
: > [001]
: > [001] not ok 23 - 4.4 #
: > [001] Traceback:
: > [001] [Lua ] function 'do_catchsql_test' at
: </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:142>
: > [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-
: 9a8b09f8e6.test.lua:252>
: > ```
: >
: > The problem was due to `.linedefined` used, instead of source line in
: `.currentline`.
:
: Typo: The line exceeds 72 chars.
Ok.
:
: >
: > Closes #6134
: > ---
: > src/lua/tap.lua | 2 +-
: > 1 file changed, 1 insertion(+), 1 deletion(-)
: >
: > diff --git a/src/lua/tap.lua b/src/lua/tap.lua
: > index 346724d84..77fd8d096 100644
: > --- a/src/lua/tap.lua
: > +++ b/src/lua/tap.lua
: > @@ -23,7 +23,7 @@ local function traceback(level)
: > local frame = {
: > source = info.source;
: > src = info.short_src;
: > - line = info.linedefined or 0;
: > + line = info.currentline or info.linedefined or 0;
:
: Why did you leave such a complex code here? I believe you can use just
: info.currentline here.
Just in case :), if currentline may be missing in the traceback object
for any reason. If it's impossible for the set of requested flags, then
I'll get rid of linedefined here.
:
: [1]:
: https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/#how-
: to-write-a-commit-message
:
: --
: Best regards,
: IM
Thanks,
Timur
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables
2021-06-20 18:52 ` [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Mergen Imeev via Tarantool-patches
@ 2021-06-27 23:29 ` Timur Safin via Tarantool-patches
0 siblings, 0 replies; 18+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-27 23:29 UTC (permalink / raw)
To: 'Mergen Imeev'; +Cc: tarantool-patches, alexander.turenko
: From: Mergen Imeev <imeevma@tarantool.org>
: Subject: Re: [PATCH v2 0/3] sql: modify explicit and implicit conversion
: tables
:
: Hi! Thank you for the patch-set. See 7 comments below.
:
: 0. Please include you answers to my previous comments right here. Same
: about all other letters.
???
:
: On Fri, Jun 11, 2021 at 10:48:10AM +0300, Timur Safin wrote:
: > Issues:
: > https://github.com/tarantool/tarantool/issues/4470
: > https://github.com/tarantool/tarantool/issues/6010
: > Branch: https://github.com/tarantool/tarantool/tree/tsafin/gh-4470-
: explicit-implicit-conversions-V2
: >
: > This patchset containes of 2 major part and 1 bonus. Major parts
: > are changes in explicit and implicit conversion tables, with all
: > accompanying tests we have so far (and 1 extensive test introduced).
: > The bonus part is for improvements in the testing system tap.lua
: > which became apparent during debugging of this patches.
: 1. Please divide this patch-set in two or three parts. I think the best
: way will
: be to divide it to three parts:
: i. Patch about tap.lua.
: ii. Patch-set about explicit casts.
: iii. Patch-set about implicit casts.
Ok, I'll exclude patch with trivial tap fix (which has helped me
debug conversion tables test, but never mind).
I'll think about a reasonable way to divide patches for implicit
casts. Looks like there is easy way to extract part connected to
separate ticket.
:
: Combining ii and iii is also acceptable, but I think the approaches to
: ii and
: iii should be completely different. I don't see any connection between the
: tap.lua patch and all the other patches in this patchset.
:
: I think it is strange, that only part of the patch-set is reviewed by
: me. Is it even possible for me to give LGTM to such patch-set?
Yes, this is normal - you react to the patch which you know,
and relay to others on other parts.
Let me explain it on some hypothetical example, not the current case.
Imagine that there is big refactoring of some subsystem (say
introduction of cmake to luajit, with thousands of accompanying
tests added), and say you are aware of cmake internals, and review
it and send LGTM to only patches changing it, but are unaware of changes
in luajit testing, then you just not ack this part, and refer to
others do their part.
Everything is possible, and circumstances may require attention of
various people, it's not problem if many people involved.
At the end of a day this calls "team work". :)
:
: >
: > Relates to #5910, #6009
: > Part of #4470
: > Fixes #6010
: >
: > Explicit table
: > ==============
: >
: > As we discovered harder way, there is no need to introduce so much changes
: > to the current explicit conversion table, because it's mostly compliant
: > already:
: > - most of changes we had to do was about `BOOLEAN` conversions, which are
: > stricter now than before;
: > - there are addition of to `ANY` conversion, which we have decided to make
: > behaving similar to `SCALAR` conversions;
: 2. As I previously said, I see no reason to include cast to field type
: ANY in this patch-set. Also, if we follow your logic, why you didn't
: added casts to array and map?
Explicit casts to ANY allowed as part of RFC ratified last quarter, and you
have been part of team developed that decision. It's ok if we implement something
you disagree with, but respect the team decision.
Arrays and maps not covered because they will be addressed later.
:
: >
: > Those changes could be visualized via this table:
: >
: >
: > | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
: > +---+---+---+---+---+---+---+---+---+---+---+---+---+
: > 0. any | | | | | | | | | | | | | |
: > 1. unsigned | | . | . | . | . | - | | . | | | | | Y |
: > 2. string | | . | . | . | . | S | . | . | | | | | Y |
: > 4. double | | . | . | . | . | - | | . | | | | | Y |
: > 5. integer | | . | . | . | . | - | | . | | | | | Y |
: > 6. boolean | | - | Y | - | - | Y | | | | | | | Y |
: > 7. varbinary | | | . | | | - | . | | | | | | Y |
: > 3. number | | . | . | . | . | - | | . | | | | | Y |
: > 9. decimal | | | | | | | | | | | | | |
: > 10. uuid | | | | | | | | | | | | | |
: > 11. array | | | | | | | | | | | | | |
: > 12. map | | | | | | | | | | | | | |
: > 8. scalar | | . | . | . | . | S | . | . | | | | | Y |
: > +---+---+---+---+---+---+---+---+---+---+---+---+---+
: 3. Since you do not want to change numbers here, I suggest to arrange field
: types according to their number.
I did not quite understand your sentence about numbers, could you please
clarify?
Also, I've already explained this strange order - this is exactly as table
described in RFC. I could reorder, but it would be harder to check against
original spec.
: Also, why you described BOOLEAN, but didn't
: describe ANY and UUID?
Good point - I will accumulate and highlight everything touched by the patch,
table was copied from original `boolean` specific commit.
:
: >
: > This table above represent all possible combinations of explicit cast
: > from type (row) to another type (column). Numbering of types might look
: > confusing (it's not sequential) but the order is what we see in the
: > consistency specification RFC, and we should live with that. Values are
: > actual enum values from the code. Unfortunately, changing of order is
: > almost impossible because of massive rework for the spec it would require.
: >
: > How to interpret this table, e.g.:
: > - for the explicit cast from `BOOLEAN` (6) to `BOOLEAN` (6) we should
: > always allow cast (and make it noop), thus intersection is `Y` (yes);
: >
: > - `STRING` (2) to `BOOLEAN` (6) may work sometimes (if string represents
: > TRUE or FALSE literals), but may fail otherwise, thus there is `S`
: (sometimes);
: >
: 4. "Sometimes" still sounds quite not right here.
Still not understand your complain here. For me it's quite ok.
:
: > - We have prohibited majority of ther directions for `BOOLEAN` thus there
: are
: > `-` (minus) in such cells, i.e. `BOOLEAN` (6) to `INTEGER` (5);
: >
: > - Untouched entries in the table marked with '.'. Assumption is -
: > we already have correct conversion rule here activated well before;
: >
: > - Worth to mention that empty cell means that this conversion
: > direction is prohibited.
: >
: 5. Does it mean that cast from UUID to UUID is prohibited?
Certainly it's not. Picture is incomplete, sorry for the confusion created.
I'll complete `boolean` only patch with all recent modifications.
[Now you probably see how visualization helps to narrow down problems?]
: Also, I see no need
: of '.' since you sometimes use 'Y' or '-' or ' ' instead of it. I
: believe you
: should describe table for all explicit cast conversions here and not
: only for
: BOOLEAN.
Yes, will accumulate everything changed. And '.' means unmodified 'Y'/'S'
since prior state (pre-RFC implementation).
:
:
: > For obvious reasons those chnaged conversion rules made us modify
: > expected results for many tests - they are fixed.
: >
: > But to cover all possible conversion combinations (minus those, not yet
: > implemented in SQL types like DECIMAL, ARRAY and MAP) we have created
: > special test which check _all combinations of CASTs()_ and verifies
: > results against table rules defined in the RFC.
: >
: > There is e_casts.test.lua created for that purpose.
: 6. I still does not understand where this name came from. I believe you said
: something about renaming it.
Yep, will do.
:
: >
: > Implicit table
: > ==============
: >
: > Similarly to explicit conversion rules we have update implicit conversion
: > rules, and this is 2nd part of patchset.
: >
: > We will not draw changed values of implicit conversion table (please see
: > it's final state in the consistemcy types RFC), but verbally we have
: > modified following directions of conversions:
: > - string to/from double
: > - double to/from integer
: > - varbinary to/from string
: >
: > In addition to modifciation of all relevant test results, we have also
: > extended e_casts.test.lua introduced above for checking of all possible
: > directions of implicit conversions.
: >
: > Approach to check it though is different than for explicit cast, we do
: > not use simple expression with implicit cast activated (like we might be
: > using `CAST()` for explicit casts), but rather we insert value of original
: > type into columns of expected target type.
: 7. Implicit casts includes a lot more than just an insert, I believe.
Yes, according to SQL-2003 standard "Implicit type conversion can occur in expressions, fetch operations, single row select operations, inserts, deletes, and updates."
Insert looks the simplest way possible in the generated code to check cast
against the expected result. I plan to extend test later, while adding more
casts for future types.
[At the moment there are no concise and clear type promotion rules when implicit
casts done in expressions. There is some hope that currently ongoing discussion
might provide such promotion rules as discussion side-effect, then we could
add those rules to the test. Not ready for such today. Yet]
Regards,
Timur
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/3] sql: updated explicit conversion table
2021-06-25 21:26 ` [Tarantool-patches] Отзыв: " Timur Safin via Tarantool-patches
@ 2021-06-27 23:46 ` Timur Safin via Tarantool-patches
0 siblings, 0 replies; 18+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-27 23:46 UTC (permalink / raw)
To: 'Mergen Imeev'; +Cc: tarantool-patches
: From: Mergen Imeev <imeevma@tarantool.org>
: Subject: Re: [PATCH v2 2/3] sql: updated explicit conversion table
:
: Thank you for the fixes you did. See my 27 comments below.
:
: 1. In general I do not think that it is good idea to squash all previous
: three
: patches to one patch. I think that the first of previous patches was
: quite good,
: the second was useless and the third had some issues, but now all these
: patches
: are squashed.
The idea was to split modification to implicit and explicit changes.
And taking into account that changes in behavior should be at the same
commit with modified tests those patches grew a bit. If we split
code and test then commits are not passing build/test separately.
:
: On Fri, Jun 11, 2021 at 10:48:12AM +0300, Timur Safin wrote:
: > * fixes for `BOOLEAN` expressions in explicit converstion tables
: >
: > 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 still do not think that path to RFC is needed here.
It's matter of taste.
: Also, what about '.'?
They mark unchanged 'Y'/'S' cells (as I said before). Or I've not
understood your clearly.
: > * introduce `ANY` as target for explicit conversions
: >
: > For consistency sake it was decided to provide
: > `CAST(anything TO ANY)` as allowed, but still noop.
: >
: 3. I still do not like that you added cast to unsupported field.
That was part of RFC we all agreed upon, not my own invention.
:
: 4. Why typeof() of value cast to any returns 'any'?
: tarantool> box.execute('select typeof(cast(1 AS any));')
: ---
: - metadata:
: - name: COLUMN_1
: type: string
: rows:
: - ['any']
: ...
:
: tarantool> box.execute('select typeof(cast(1 AS scalar));')
: ---
: - metadata:
: - name: COLUMN_1
: type: string
: rows:
: - ['integer']
: ...
Good observation, thanks! `cast(1 as scalar)` should return expression
attributed as `scalar` also. (Because why this cast then? Values should
be left untouched, only type of expression reattributed.
:
: > * We had to rename %wildcard token to ANYTHING, since we needed
: > token ANY for real life usage.
: >
: 5. Just a thought - why you cannot use 'ANYTHING' for field type ANY?
: Or 'ANY_KW', how it was already done for INTEGER?
As %wildcard token we should use separate, artificial lexem, not any
of actually used.
:
: > * As a byproduct, we fixed #6010 to make cast to unsigned behaving
: reasonably
: >
: 6. I think this change is good enough to have its own patch.
It's still part of implicit casts fixes, and affect some(same) set of tests.
[i.e. those tests will be modified multiple times along a patches flow in
The same patchset]
But yes, in this case there was separate ticket, with specific test-case,
Thus it's easy to extract.
:
: > * To make sure that all consistency checks are systematic, we have
: introduced
: > special test for checking conversion rules - e_casts.test.lua. This
: > patch introduces explicit table part:
: >
: > * e_casts.test.lua is generating expressions of `CAST(input AS output)`
: > form. All combinations check whether we expectedly succeeded or
: failed,
: > given the explicit conversion table from RFC 5910-consistent-sql-lua-
: types.md;
: >
: > * At the moment we skip DECIMALs, ARRAYs and MAPs as not yet
: > fully supported. Need to be revisited later;
: >
: > * NB! there is `-verbose` mode one may activate to enable more detailed
: > reporting during debugging.
: >
: 7. I used './test-run.py sql-tap/e_casts.test.lua --verbose -j1' and saw
: this:
: [001] sql-tap/e_casts.test.lua memtx [001] TAP
: version 13
: [001] 1..3
: [001] # e_casts - check consistency of implicit conversion table
: [001] 1..169
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [any,any] nil ~= nil
...
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,number] 2 ~= 1
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,decimal] nil ~= nil
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,uuid] nil ~= nil
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,array] nil ~= nil
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,map] nil ~= nil
: [001] ok - /home/mergen/work/dev/test/sql-tap/e_casts.test.lua:245
: [unsigned,scalar] 2 ~= 1
: ...
:
: What is it?
Have you looked into code of this test? In this subtest we check that
implicit conversion table is symmetric in their cells (i.e. if there is
any sort of conversion from type A to B then there have to be
some kind sort of reverse conversion from type B to A)
Agreed though, that 2 != 1, and especially nil ~= nil, look funny and
Confusing without looking into sources, and I will provide more human-readable
output there.
:
: > Relates to #5910, #6009
: > Part of #4470
: > Fixes #6010
: >
: > +int
: > +mem_to_uint(struct Mem *mem)
: > +{
: > + assert(mem->type < MEM_TYPE_INVALID);
: > + if (mem->type == MEM_TYPE_UINT)
: > + return 0;
: > + if (mem->type == MEM_TYPE_INT) {
: > + mem_set_uint(mem, (uint64_t)mem->u.i);
: 8. Could you explain to me, what it this? Since when -1 can be cast to
: UNSIGNED?
This is rather (usual) reinterpret_cast<> for conversion of
negative signed integer to unsigned.
: Where it is described? Why this is allowed?
: tarantool> box.execute('select CAST(-1 as unsigned);')
: ---
: - metadata:
: - name: COLUMN_1
: type: unsigned
: rows:
: - [18446744073709551615]
: ...
Now looking into RFC table for explicit casts I do see it's
slightly extension of an agreed upon "conditional" conversion
(i.e. if signed integer has positive value then it should be
convertible, but raise error otherwise).
So yes, thanks for catch!
:
:
: > + return 0;
: > + }
: > + if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0)
: > + return bytes_to_uint(mem);
: > + if (mem->type == MEM_TYPE_DOUBLE)
: > + return double_to_uint(mem);
: > + return -1;
: > +}
: > +
: > int
: > mem_to_int(struct Mem *mem)
: > {
: > assert(mem->type < MEM_TYPE_INVALID);
: > - if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
: > + if (mem->type == MEM_TYPE_INT)
: > + return 0;
: > + if (mem->type == MEM_TYPE_UINT) {
: > + mem_set_int(mem, (int64_t)mem->u.u, false);
: 9. So, you want to set unsigned value as integer. Could you tell me what
: does
: this change do?
At least it will change type attribution (that's important
because of different allowed ranges of values).
: > @@ -1017,13 +989,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type
: type)
: > switch (mem->type) {
: 10. Just a thought. I see no harm to have this switch here, but it has
: only two
: options. Why not change it to 'if'?
Wanted to say that I kept switch to look consistent with code above and
below, but then realized that more complicated code with switch cases
has already been refactored to functions and this switch left alone,
and only ifs lying around it.
So, yep, will switch switch to ifs. That would fit better to the context.
...
: > diff --git a/test/sql-tap/e_casts.test.lua b/test/sql-tap/e_casts.test.lua
: > new file mode 100755
: > index 000000000..32d7e8e0c
: > --- /dev/null
: > +++ b/test/sql-tap/e_casts.test.lua
: > @@ -0,0 +1,340 @@
: > +#!/usr/bin/env tarantool
: > +local tap = require("tap")
: > +local test = tap.test("errno")
: > +
: > +test:plan(1)
: > +
: > +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
: > + };
: > +]]
: 11. Since this cdef is only copy-paste from current actual enum, I still see
: not reason to keep it here. Or you could use one from module.h, if you want
: to keep it.
Nope, we could not use module.h - it's been built much, much later in the
current build flow. Module_api and testing are currently independent targets.
and this unnecessary dependency looks redundant, copy-pastes are bad, but
sometimes they are excusable to avoid introduction of unnecessary troubles (in
already quite fragile build process)
: > +
: > +-- 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] = true,
: > + -- [t_date] = false,
: > + -- [t_time] = false,
: > + -- [t_timestamp]= false,
: > + -- [t_interval] = False,
: > + [t_array] = false,
: > + [t_map] = false,
: > + [t_scalar] = true,
: > +}
: > +
: > +-- Enabled types which may be targets for explicit casts
: > +local enabled_type_cast = table.deepcopy(enabled_type)
: > +enabled_type_cast[t_any] = true
: > +
: > +-- table of _TSV_ (tab separated values)
: > +-- copied from sql-lua-tables-v5.xls
: > +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"},
: > +}
: 12. Again, why there is 'N' and '' both in table? Could you show me where in
: RFC we have this table that contains both '' and 'N'?
This is also explained with - because of RFC! ;) Kinda. In this case that's the way
it looked in frozen materials (excel) used for RFC generation which I've published
as attachments to https://github.com/tarantool/tarantool/wiki/SQL-Lua-Consistent-Type-Specification
wiki page. OTOH ' ' and 'N' are surely interchangeable, and could be easily massaged
in the editor, so I'll change them to always be ' ' (to make it less confusing for others).
...
: > +
: > + for _, from in ipairs(proper_order) do
: > + local line = ''
: > + for _, 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
: 13. Since you use '-verbose' mode only in your debug, can you move these
: functions out from here? I still do not see any use of this code.
Out from here to where? I don't get it. I do print implicit and explicit
tables content if ran with '--verbose' option. And this is great debugging helper while
you modify those tables.
Have you ever run this test outside of test-run infrastructure? Like
```
✔ ~/tarantool/test
18:57 $ cat ~/bin/sql-tap-run.sh
#/usr/bin/bash
root=$(git rev-parse --show-toplevel)
rm -f *.snap *.xlog
LUA_PATH="$root/test/sql-tap/lua/?.lua;$root/test/sql/lua/?.lua;" \
$prefix $root/build/src/tarantool $*
✔ ~/tarantool/test
18:57 $ ~/bin/sql-tap-run.sh sql-tap/e_casts.test.lua --verbose |& 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 | | | | | Y |
2. string | Y | S | Y | S | S | S | Y | S | | S | | | Y |
4. double | Y | S | Y | Y | S | | | Y | | | | | Y |
5. integer | Y | S | Y | Y | Y | | | Y | | | | | Y |
6. boolean | Y | | Y | | | Y | | | | | | | Y |
7. varbinary | Y | | Y | | | | Y | | | S | | | Y |
3. number | Y | S | Y | Y | S | | | Y | | | | | Y |
9. decimal | | | | | | | | | | | | | |
10. uuid | Y | | Y | | | | Y | | | Y | | | Y |
11. array | | | | | | | | | | | | | |
12. map | | | | | | | | | | | | | |
8. scalar | Y | S | Y | S | 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 | | 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 | | | S | | | Y |
3. number | | S | Y | Y | S | | | Y | | | | | Y |
9. decimal | | | | | | | | | | | | | |
10. uuid | | | Y | | | | Y | | | Y | | | Y |
11. array | | | | | | | | | | | | | |
12. map | | | | | | | | | | | | | |
8. scalar | | S | 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:245 [any,any] nil ~= nil
ok - sql-tap/e_casts.test.lua:245 [any,unsigned] nil ~= nil
ok - sql-tap/e_casts.test.lua:245 [any,string] nil ~= nil
ok - sql-tap/e_casts.test.lua:245 [any,double] nil ~= nil
```
Ain't them pretty? ;)
: > +
: > +local gen_type_samples = {
: > + [t_unsigned] = {"0", "1", "2"},
: 14. I believe you once said something about testing values that are close to
: all limits of numeric types. Why I cannot see them here?
This is very good point - I'll add some boundary values.
: > +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
: 15. Since you use all these functions and do not want to remove them, I
: suggest
: to add comments to them.
Yeah, these are much easier to use than those we have in sqltester.lua.
Will provide some comments.
:
: > +
: > +-- 1. Check explicit casts table
: > +local function test_check_explicit_casts(test)
: > + -- checking validity of all `CAST(from AS to)` combinations
: > + test:plan(322)
: > + for _, from in ipairs(proper_order) do
: > + for _, to in ipairs(proper_order) do
: > + -- skip ANY, DECIMAL, UUID, etc.
: 16. Why UUID is skipped? Also, why ANY is skipped?
No, in this test we do not skip uuids, they already been integrated
into test. That's rather stale/outdated comment. Sorry for the confusion.
: You can get values of type
: ANY by using CAST(values AS ANY), I believe. Or I am wrong?
Yes and no. Given expression of a form `CAST(x as ANY)` we do have
expression attributed with type ANY, but there is still no direct
way to write literal of type ANY.
Having said that we indeed could use more complicated expressions,
not only literals. So yes, we may use such expressions.
[But today I'm not actually certain we will not disable these kinds
of casts entirely, despite them agreed in the RFC. Along with all
possible changes many want to introduce for scalars. So let see
where we will go in the nearest future. Sigh. ]
:
: > + if enabled_type[from] and enabled_type_cast[to] then
: > + local gen = gen_explicit_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
: > +
: > +
: > + -- ok, we aggregated stats for c_maybe mode - check it
: now
: > + if expected == c_maybe then
: > + local title = string.format("%s => %s",
: > + #gen and
: gen[1]..'...' or '',
: > + human_cast(expected))
: > + test:ok(castable, label_for(from, to, title),
: > + failures)
: > + end
: 16. I already said my expectations about these tests. Since you know
: (and print)
: all values used in the test, why cannot you determine if the cast should
: fail?
: For example, this test is actually useless if `CAST(1.0 AS UNSIGNED)`
: fails, since
: this cast is marked as 'sometimes'. If you do so, you can just print 'Y'
: or 'N'.
: Also, why your tests does not check unallowed casts? Or, at least, they
: are not
: shown.
It's actually testing entire table (with only exception of unsupported
today types).
You see the line?
local expected = explicit_casts[from][to]
and then
if expected == c_yes then
test:ok(true == ok, label_for(from, to, title))
elseif expected == c_no then
test:ok(false == ok, label_for(from, to, title))
else
The problem is in TAP reporting - I did not find any way to have passing
test but displaying a number of expected failures. They all look identical
now (as passes), and all looks as castable, even if they are not.
Will modify label_for() as promised above to clearly indicate that
some of them expected to fail.
: > --- a/test/sql-tap/in1.test.lua
: > +++ b/test/sql-tap/in1.test.lua
: > @@ -100,7 +100,7 @@ test:do_execsql_test(
: > test:do_execsql_test(
: > "in-1.7",
: > [[
: > - SELECT a+ 100*CAST((a BETWEEN 1 and 3) AS INTEGER) FROM t1 ORDER
: BY b
: > + SELECT a+ 100*(CASE WHEN (a BETWEEN 1 and 3) THEN 1 ELSE 0 END)
: FROM t1 ORDER BY b
: 17. Since you already change this line, add spaces before and after all
: operations with two operands.
Hmm, ok
:
: > ]], {
: > -- <in-1.7>
: > 101, 102, 103, 4, 5, 6, 7, 8, 9, 10
: > @@ -157,7 +157,7 @@ test:do_execsql_test(
: > test:do_execsql_test(
: > "in-2.5",
: > [[
: > - SELECT a+100*(CAST(b IN (8,16,24) AS INTEGER)) FROM t1 ORDER BY b
: > + SELECT a+100*(CASE WHEN b IN (8,16,24) THEN 1 ELSE 0 END) FROM t1
: ORDER BY b
: 18. Same.
Ok
:
: > ]], {
: > -- <in-2.5>
: > 1, 2, 103, 104, 5, 6, 7, 8, 9, 10
: > @@ -207,7 +207,7 @@ test:do_execsql_test(
: > test:do_execsql_test(
: > "in-2.10",
: > [[
: > - SELECT a FROM t1 WHERE LEAST(0, CAST(b IN (a,30) AS INT)) <> 0
: > + SELECT a FROM t1 WHERE LEAST(0, CASE WHEN (b IN (a,30)) THEN 1
: ELSE 0 END) <> 0
: 19. I think a space should be after ',' in '(a,30)'.
Ok
: > diff --git a/test/sql-tap/in4.test.lua b/test/sql-tap/in4.test.lua
: > index 8442944b9..588daefec 100755
: > --- a/test/sql-tap/in4.test.lua
: > +++ b/test/sql-tap/in4.test.lua
: > @@ -133,7 +133,7 @@ test:do_execsql_test(
: > SELECT b FROM t2 WHERE a IN (1.0, 2.1)
: > ]], {
: > -- <in4-2.6>
: > - "one"
: > + "one", "two"
: 21. Why the result changed?
Good question! You see I've added case when implicit cast from double to
integer worked expectedly and brought us 2 rows, but here, by some reason
implicit cast from 2.1 didn't match integer 2. I need to investigate
reasons, at the moment I do not recollect why it was ok for me.
:
: > -- </in4-2.6>
: > })
: >
: > 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')
: 22. What does this test checks now and what it checked before?
That was actually a regression test for SQLite bug #668
```
-- Ticket #668: VDBE stack overflow occurs when the left-hand side
-- of an IN expression is NULL and the result is used as an integer, not
-- as a jump.
```
It makes no sense today for our conditions, and I believe it should
be kept here just for not reducing coverage. [Though I'd delete
subtests for #668 if we would care about logical system in our tests. :)]
: > 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'
: 23. Since all these CAST are now fails, I think it is better to divide this
: query into three. Otherwise only the first cast is checked.
Into 3? Have you actually meant 2 in this case?
:
: > | ...
: > 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'
: 24. Same.
The same confusion - we will receive 2 separate test, why 3?
: > @@ -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'
: > | ...
: 25. Same.
Division of this select into 3 separate selects makes no sense at all -
they will receive the same result for the same condition, the only
reasonable case now would be:
```
- SELECT cast(100 AS BOOLEAN), cast(1 AS BOOLEAN), cast(0 AS BOOLEAN);
+ SELECT cast(100 AS 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'
: > | ...
: 26. Same.
Similarly - only 1 expression is necessary
: > diff --git a/test/sql/types.result b/test/sql/types.result
: > index 687ca3b15..90a8bc5ec 100644
: > --- a/test/sql/types.result
: > +++ b/test/sql/types.result
: > @@ -1084,8 +1081,11 @@ box.execute("SELECT CAST(123 AS UNSIGNED);")
: > ...
: > box.execute("SELECT CAST(-123 AS UNSIGNED);")
: > ---
: > -- null
: > -- 'Type mismatch: can not convert -123 to unsigned'
: > +- metadata:
: > + - name: COLUMN_1
: > + type: unsigned
: > + rows:
: > + - [18446744073709551493]
: > ...
: 27. Where does such behaviour is described?
Ok, as a said above such reinterpret_cast<> was rather an extension
of RFC, and will return back conditional failure.
Thanks,
Timur
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/3] sql: updated implicit conversion table
2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches
@ 2021-06-28 0:06 ` Timur Safin via Tarantool-patches
0 siblings, 0 replies; 18+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-28 0:06 UTC (permalink / raw)
To: 'Mergen Imeev'; +Cc: tarantool-patches
: From: Mergen Imeev <imeevma@tarantool.org>
: Subject: Re: [PATCH v2 3/3] sql: updated implicit conversion table
:
: Thank you for the patch. See 33 comments below.
:
: On Fri, Jun 11, 2021 at 10:48:13AM +0300, Timur Safin wrote:
: > * Changed implicit casts table according to consistent types RFC
: >
: > * fixed following directions for implicit conversions
: > - string to double
: > - double to integer
: > - varbinary to string
: >
: > * got rid of mem_cast_implicit_old() as unnecessary, use always
: > the updated mem_cast_implicit()
: >
: > * in addition to update of all relevant tests there is extended
: > e_casts.test.lua used for checking of implicit conversion rules:
: >
: > * there is table information sanity check - e_casts.test.lua checks
: > that implicit table content is sane, i.e. it's sort of symmetric
: > for all defined combinations [from, to]
: >
: > * for the test we use inserts in specially crafted table as checks
: > availability of implicit conversions between type values
: > * Temporary TCASTS table used with sequence primary key
: > * All fields of a form `{name = "unsigned", type = "unsigned",
: is_nullable = true}`
: > used for all enabled types
: 1. Since you said that you fix 'implicit cast', not just 'implicit cast for
: insert', please fix all implicit casts. Also, your changes added
: implicit cast
: for comparison in some cases, even though it should be removed according
: to RFC.
You slightly misdirect your objections here - I simply explained the way
test was operating while checking of implicit conversion table, but I've got
your point.
:
: >
: > Relates to #5910, #6009
: > Closes #4470
: >
: > * Additionally we have fixed unexpected results for sum of implicitly
: > converted numbers
: 2. I believe this change should be moved to its own patch.
It was still a fix for implicit conversion (which is a goal of this patch).
But probably it would be easy to do.
:
: >
: > Fixing unexpected results after implicit conversion to
: > the signed or unsigned integer from well-formed string.
: >
: > ```
: > tarantool> box.execute([[select '1' + '2';]])
: > ---
: > - metadata:
: > - name: COLUMN_1
: > type: scalar
: > rows:
: > - [3]
: > ...
: >
: > tarantool> box.execute([[select '1' + '-2';]])
: > ---
: > - metadata:
: > - name: COLUMN_1
: > type: scalar
: > rows:
: > - [18446744073709551615]
: > ...
: >
: > tarantool> box.execute([[select '-1' + '-2';]])
: > ---
: > - null
: > - 'Failed to execute SQL statement: integer is overflowed'
: > ...
: > ```
: 3. So, this is how it works now? I do not see any description.
Nope. That was part of original bug description.
Will clarify the correct way.
[Iff dust will settle down in current discussions about scalar
types destiny]
: > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
: > index 8bb6b672c..d6b114a81 100644
: > --- a/src/box/sql/mem.c
: > +++ b/src/box/sql/mem.c
: > @@ -1033,95 +1033,24 @@ mem_cast_implicit(struct Mem *mem, enum field_type
: type)
: > }
: > switch (type) {
: > case FIELD_TYPE_UNSIGNED:
: > - if (mem->type == MEM_TYPE_UINT)
: > - return 0;
: > - if (mem->type == MEM_TYPE_DOUBLE)
: > - return double_to_uint(mem);
: > - return -1;
: > - case FIELD_TYPE_STRING:
: > - if (mem->type == MEM_TYPE_STR)
: > - return 0;
: > - if (mem->type == MEM_TYPE_UUID)
: > - return uuid_to_str0(mem);
: > - return -1;
: > - case FIELD_TYPE_DOUBLE:
: > - if (mem->type == MEM_TYPE_DOUBLE)
: > - return 0;
: > - if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
: > - return int_to_double(mem);
: > - return -1;
: > - case FIELD_TYPE_INTEGER:
: > if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
: > return 0;
: 4. I tried to insert negative integer and I got this:
: tarantool> box.execute('create table t (u unsigned primary key);')
: ---
: - row_count: 1
: ...
:
: tarantool> box.execute('insert into t values (-1);')
: ---
: - null
: - 'Tuple field 1 (U) type does not match one required by operation:
: expected unsigned,
: got integer'
: ...
:
: Is it right?
Yes, that's correct and as expected. We conditionally implicitly convert
integers to unsigned, only if they are in supported values range.
: > -int
: > -mem_cast_implicit_old(struct Mem *mem, enum field_type type)
: > -{
: > - if (mem->type == MEM_TYPE_NULL)
: > - return 0;
: > - switch (type) {
: > - case FIELD_TYPE_UNSIGNED:
: > - if (mem->type == MEM_TYPE_UINT)
: > - return 0;
: > if (mem->type == MEM_TYPE_DOUBLE)
: > return double_to_uint_precise(mem);
: > if (mem->type == MEM_TYPE_STR)
: > return bytes_to_uint(mem);
: > return -1;
: > case FIELD_TYPE_STRING:
: > - if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0)
: > + if (mem->type == MEM_TYPE_STR)
: > return 0;
: > + if (mem->type == MEM_TYPE_UUID)
: > + return uuid_to_str0(mem);
: > if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
: > return int_to_str0(mem);
: > if (mem->type == MEM_TYPE_DOUBLE)
: > return double_to_str0(mem);
: > - if (mem->type == MEM_TYPE_UUID)
: > - return uuid_to_str0(mem);
: 5. Why you moved these lines?
[I may be misunderstood your complain, but just in case]
Have you recognized the fact that those lines are from mem_cast_implicit()
and I've entirely removed mem_cast_implicit_old()?
And the order of cases in mem_cast_implicit() is now the same as
in mem_cast_explicit() after this reorder.
: > @@ -1129,25 +1058,28 @@ mem_cast_implicit_old(struct Mem *mem, enum
: field_type type)
: > if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
: > return int_to_double(mem);
: > if (mem->type == MEM_TYPE_STR)
: > - return bin_to_str(mem);
: > + return bytes_to_double(mem);
: > return -1;
: > case FIELD_TYPE_INTEGER:
: > if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
: > return 0;
: > - if (mem->type == MEM_TYPE_STR)
: > - return bytes_to_int(mem);
: 6. Same.
What is the same? Why I've reordered ifs here? To make it look
more consistent with cases around.
:
: > if (mem->type == MEM_TYPE_DOUBLE)
: > return double_to_int_precise(mem);
: > + if (mem->type == MEM_TYPE_STR)
: > + return bytes_to_int(mem);
: > return -1;
: > case FIELD_TYPE_BOOLEAN:
: > if (mem->type == MEM_TYPE_BOOL)
: > return 0;
: > return -1;
: > case FIELD_TYPE_VARBINARY:
: > - if (mem->type == MEM_TYPE_BIN)
: > + if ((mem->type & (MEM_TYPE_BIN | MEM_TYPE_MAP |
: > + MEM_TYPE_ARRAY)) != 0)
: 7. What about this? Where it is described?
This is half-cooked approach to handle anyhow map and array.
Will remove. Thanks for the catch. Maps and arrays will be handled
each separately and in future patches.
: > @@ -1156,11 +1088,11 @@ mem_cast_implicit_old(struct Mem *mem, enum
: field_type type)
: > return mem_to_number(mem);
: > return -1;
: > case FIELD_TYPE_MAP:
: > - if (mem->type == MEM_TYPE_MAP)
: > + if (mem_is_map(mem))
: 8. What is the point of this change? Please revert it.
Hehe! Then we should remove mem_is_map() and mem_is_array() which are
equivalent and compiler will generate exactly the same code here but
which we are unused at the moment and we afraid to use.
Though taking into account that around this code we always prefer
to directly manipulate with flags, not use helpers, it is look stranger
and inconsistent.
In any case, could you please elaborate your preferences here?
[Not that I promise to follow them - I'm just curious]
:
: > return 0;
: > return -1;
: > case FIELD_TYPE_ARRAY:
: > - if (mem->type == MEM_TYPE_ARRAY)
: > + if (mem_is_array(mem))
: 9. Same.
The same ^
: > @@ -1460,7 +1394,7 @@ get_number(const struct Mem *mem, struct sql_num
: *number)
: > if (mem->type == MEM_TYPE_INT) {
: > number->i = mem->u.i;
: > number->type = MEM_TYPE_INT;
: > - number->is_neg = true;
: > + number->is_neg = mem->u.i < 0;
: 10. Why? All values of type MEM_TYPE_INT are negative by default.
Nope. Not in this case. I'll provide example when I'll extract code
necessary for #5756.
: > @@ -1473,13 +1407,6 @@ get_number(const struct Mem *mem, struct sql_num
: *number)
: > return -1;
: > if (sql_atoi64(mem->z, &number->i, &number->is_neg, mem->n) == 0) {
: > number->type = number->is_neg ? MEM_TYPE_INT : MEM_TYPE_UINT;
: > - /*
: > - * The next line should be removed along with the is_neg field
: > - * of struct sql_num. The integer type tells us about the sign.
: > - * However, if it is removed, the behavior of arithmetic
: > - * operations will change.
: > - */
: > - number->is_neg = false;
: 11. Why you did not removed 'is_neg' field from 'struct sql_number'?
: What is the
: point of having it there?
Good point. Will consider removing this flag for the next version of
patch for #5756.
: > diff --git a/src/box/sql/util.c b/src/box/sql/util.c
: > index c556b9815..69b1a3937 100644
: > --- a/src/box/sql/util.c
: > +++ b/src/box/sql/util.c
: > @@ -958,9 +958,8 @@ sql_add_int(int64_t lhs, bool is_lhs_neg, int64_t rhs,
: bool is_rhs_neg,
: > *res = lhs + rhs;
: > return 0;
: > }
: > - *is_res_neg = is_rhs_neg ? (uint64_t)(-rhs) > (uint64_t) lhs :
: > - (uint64_t)(-lhs) > (uint64_t) rhs;
: > *res = lhs + rhs;
: > + *is_res_neg = *res < 0;
: 12. Is this right?
: tarantool> box.execute('select 18446744073709551606+-1;')
: ---
: - metadata:
: - name: COLUMN_1
: type: integer
: rows:
: - [-11]
: ...
:
: tarantool> box.execute('select -1+18446744073709551606;')
: ---
: - metadata:
: - name: COLUMN_1
: type: integer
: rows:
: - [-11]
: ...
:
: Before this change:
: tarantool> box.execute('select -1+18446744073709551606;')
: ---
: - metadata:
: - name: COLUMN_1
: type: integer
: rows:
: - [18446744073709551605]
: ...
:
: tarantool> box.execute('select 18446744073709551606+-1;')
: ---
: - metadata:
: - name: COLUMN_1
: type: integer
: rows:
: - [18446744073709551605]
: ...
:
Hmm, hmm... Good catch, thanks!
: > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
: > index 32d02d96e..0dc28e2d6 100644
: > --- a/src/box/sql/vdbe.c
: > +++ b/src/box/sql/vdbe.c
: > @@ -1660,7 +1660,7 @@ case OP_Ge: { /* same as TK_GE, jump,
: in1, in3 */
: > } else if (type == FIELD_TYPE_STRING) {
: > if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) {
: > const char *str =
: > - mem_cast_implicit_old(pIn3, type) != 0 ?
: > + mem_cast_implicit(pIn3, type) != 0 ?
: 13. Is this right?
: tarantool> box.execute('create table t2(s string primary key);')
: ---
: - row_count: 1
: ...
:
: tarantool> box.execute([[insert into t2 values('0');]])
: ---
: - row_count: 1
: ...
:
: tarantool> box.execute('select * from t2 where s < 1;')
: ---
: - metadata:
: - name: S
: type: string
: rows:
: - ['0']
: ...
Good point. This is comparison of different types, thus we have to use
box scalar comparison rules where `number` < `string` always. Thus
s < 1 predicate should never match, and should return empty resultset.
Indeed, in this particular case we should not call mem_cast_implicit() this
way, but instead rather call something similar to mp_compare_scalar_with_type()
Will refactor this part of vdbe.c (function is longish and unreadable mess),
and then will implement comparators.
[If this week we would not entirely rework decisions we have made about how
scalar type operates]
: > @@ -1671,7 +1671,7 @@ case OP_Ge: { /* same as TK_GE, jump,
: in1, in3 */
: > type = FIELD_TYPE_NUMBER;
: > if (mem_cmp_num(pIn3, pIn1, &res) != 0) {
: > const char *str =
: > - mem_cast_implicit_old(pIn3, type) != 0 ?
: > + mem_cast_implicit(pIn3, type) != 0 ?
: 14. Is this right?
: tarantool> box.execute('create table t3(n number primary key);')
: ---
: - row_count: 1
: ...
:
: tarantool> box.execute([[insert into t3 values(0);]])
: ---
: - row_count: 1
: ...
:
: tarantool> box.execute([[select * from t3 where n < '1';]])
: ---
: - metadata:
: - name: N
: type: number
: rows:
: - [0]
: ...
Well, number always less than string, thus n < '1' will
always be TRUE per our box scalar comparisons rules. So
(accidentally) we have returned correct resultset.
But correct SQL comparators need to be implemented yet.
:
: > @@ -1682,7 +1682,7 @@ case OP_Ge: { /* same as TK_GE, jump,
: in1, in3 */
: > assert(mem_is_str(pIn3) && mem_is_same_type(pIn3, pIn1));
: > if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) {
: > const char *str =
: > - mem_cast_implicit_old(pIn3, type) != 0 ?
: > + mem_cast_implicit(pIn3, type) != 0 ?
: 15. What changes after this?
Answer is as prior - type comparators to come here. But in this step
we have got rid of mem_cast_implicit_old().
: > @@ -2218,7 +2218,7 @@ case OP_MakeRecord: {
: > if (types != NULL) {
: > pRec = pData0;
: > do {
: > - mem_cast_implicit_old(pRec++, *(types++));
: > + mem_cast_implicit(pRec++, *(types++));
: 16. Same, what changes?
Answer same as above.
: > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
: > index 4a1fdb637..c8e242c2f 100644
: > --- a/src/box/sql/vdbeaux.c
: > +++ b/src/box/sql/vdbeaux.c
: > @@ -2332,7 +2332,7 @@ sqlVdbeGetBoundValue(Vdbe * v, int iVar, u8 aff)
: > sql_value *pRet = sqlValueNew(v->db);
: > if (pRet) {
: > mem_copy(pRet, pMem);
: > - mem_cast_implicit_old(pRet, aff);
: > + mem_cast_implicit(pRet, aff);
: 17. Same, what changes?
Glad you asked! And the answer is the same as above.
: > diff --git a/test/sql-tap/e_casts.test.lua b/test/sql-tap/e_casts.test.lua
: > index 32d7e8e0c..952572921 100755
: > --- a/test/sql-tap/e_casts.test.lua
: > +++ b/test/sql-tap/e_casts.test.lua
: > +test:test("e_casts - check consistency of implicit conversion table",
: test_check_table_consistency)
: > test:test("e_casts - check explicit casts", test_check_explicit_casts)
: > +test:test("e_casts - check implicit casts", test_check_implicit_casts)
: 18. Same comments as in previous letter.
I've answered it then alsewhere. Hopefully.
: > diff --git a/test/sql-tap/in4.test.lua b/test/sql-tap/in4.test.lua
: > index 588daefec..6aeaff5d5 100755
: > --- a/test/sql-tap/in4.test.lua
: > +++ b/test/sql-tap/in4.test.lua
: > @@ -128,15 +128,26 @@ test:do_execsql_test(
: > })
: >
: > test:do_execsql_test(
: > - "in4-2.6",
: > + "in4-2.6.0",
: > [[
: > - SELECT b FROM t2 WHERE a IN (1.0, 2.1)
: > + SELECT b FROM t2 WHERE a IN (1.0, 2.0)
: > ]], {
: > -- <in4-2.6>
: > "one", "two"
: > -- </in4-2.6>
: > })
: >
: > +-- FIXME - IN [2.1] should convert to expected type of a
: > +test:do_execsql_test(
: > + "in4-2.6.1",
: > + [[
: > + SELECT b FROM t2 WHERE a IN (1.0, 2.1)
: > + ]], {
: > + -- <in4-2.6.1>
: > + "one"
: > + -- </in4-2.6.1>
: > + })
: > +
: 19. In the previous patch you changed the result and now you changed the
: test.
: Why?
Because in this patch we have modified implicit casts behavior thus once
again have to update results, or introduce cases which will bring original
results.
: > diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-
: 9a8b09f8e6.test.lua
: > index 67d6a1ccd..8ecf9f914 100755
: > --- a/test/sql-tap/tkt-9a8b09f8e6.test.lua
: > +++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua
: > @@ -239,23 +239,23 @@ test:do_execsql_test(
: > -- </4.2>
: > })
: >
: > -test:do_catchsql_test(
: > +test:do_execsql_test(
: > 4.3,
: > [[
: > SELECT x FROM t3 WHERE x IN ('1');
: > ]], {
: > -- <4.3>
: > - 1, "Type mismatch: can not convert 1 to number"
: > + 1.0
: 20. Why this does not throw an error now? Where this behaviour is described?
Because this is the place where implicit cast (from `string` to `number`)
will be activated. No?
: > -test:do_catchsql_test(
: > +test:do_execsql_test(
: > 4.4,
: > [[
: > SELECT x FROM t3 WHERE x IN ('1.0');
: > ]], {
: > -- <4.4>
: > - 1, "Type mismatch: can not convert 1.0 to number"
: > + 1.0
: 21. Same.
Ditto.
: > @@ -279,23 +279,23 @@ test:do_execsql_test(
: > -- </4.6>
: > })
: >
: > -test:do_catchsql_test(
: > +test:do_execsql_test(
: > 4.7,
: > [[
: > SELECT x FROM t3 WHERE '1' IN (x);
: > ]], {
: > -- <4.7>
: > - 1, "Type mismatch: can not convert 1 to number"
: > + 1.0
: 22. Same.
Ditto.
:
: > -- </4.7>
: > })
: >
: > -test:do_catchsql_test(
: > +test:do_execsql_test(
: > 4.8,
: > [[
: > SELECT x FROM t3 WHERE '1.0' IN (x);
: > ]], {
: > -- <4.8>
: > - 1, "Type mismatch: can not convert 1.0 to number"
: > + 1.0
: 23. Same.
Ditto
:
: > -- </4.8>
: > })
: >
: > @@ -319,23 +319,23 @@ test:do_execsql_test(
: > -- </5.2>
: > })
: >
: > -test:do_catchsql_test(
: > +test:do_execsql_test(
: > 5.3,
: > [[
: > SELECT x FROM t4 WHERE x IN ('1');
: > ]], {
: > -- <5.3>
: > - 1, "Type mismatch: can not convert 1 to number"
: > +
: 24. Same.
Ditto.
:
: > -- </5.3>
: > })
: >
: > -test:do_catchsql_test(
: > +test:do_execsql_test(
: > 5.4,
: > [[
: > SELECT x FROM t4 WHERE x IN ('1.0');
: > ]], {
: > -- <5.4>
: > - 1, "Type mismatch: can not convert 1.0 to number"
: > +
: 25. Same.
Ditto.
:
: > -- </5.4>
: > })
: >
: > @@ -349,13 +349,13 @@ test:do_execsql_test(
: > -- </5.5>
: > })
: >
: > -test:do_catchsql_test(
: > +test:do_execsql_test(
: > 5.6,
: > [[
: > SELECT x FROM t4 WHERE x IN ('1.11');
: > ]], {
: > -- <5.6>
: > - 1, "Type mismatch: can not convert 1.11 to number"
: > + 1.11
: 26. Same.
Ditto.
:
: > -- </5.6>
: > })
: >
: > @@ -379,23 +379,23 @@ test:do_execsql_test(
: > -- </5.8>
: > })
: >
: > -test:do_catchsql_test(
: > +test:do_execsql_test(
: > 5.9,
: > [[
: > SELECT x FROM t4 WHERE '1' IN (x);
: > ]], {
: > -- <5.9>
: > - 1, "Type mismatch: can not convert 1 to number"
: > +
: 27. Same.
Ditto.
:
: > -- </5.9>
: > })
: >
: > -test:do_catchsql_test(
: > +test:do_execsql_test(
: > 5.10,
: > [[
: > SELECT x FROM t4 WHERE '1.0' IN (x);
: > ]], {
: > -- <5.10>
: > - 1, "Type mismatch: can not convert 1.0 to number"
: > +
: 28. Same.
Ditto.
:
: > -- </5.10>
: > })
: >
: > @@ -409,13 +409,13 @@ test:do_execsql_test(
: > -- </5.11>
: > })
: >
: > -test:do_catchsql_test(
: > +test:do_execsql_test(
: > 5.12,
: > [[
: > SELECT x FROM t4 WHERE '1.11' IN (x);
: > ]], {
: > -- <5.12>
: > - 1, "Type mismatch: can not convert 1.11 to number"
: > + 1.11
: 29. Same.
Glad you asked! Ditto.
: > diff --git a/test/sql/types.result b/test/sql/types.result
: > index 90a8bc5ec..20dbd2073 100644
: > --- a/test/sql/types.result
: > +++ b/test/sql/types.result
: > @@ -1059,7 +1059,8 @@ box.execute("INSERT INTO t1 VALUES (0), (1), (2);")
: > box.execute("INSERT INTO t1 VALUES (-3);")
: > ---
: > - null
: > -- 'Type mismatch: can not convert -3 to unsigned'
: > +- 'Tuple field 1 (ID) type does not match one required by operation:
: expected unsigned,
: > + got integer'
: 30. Why the error changed?
Good question - need to look around. Return back here before sending the
next version.
: > @@ -1224,12 +1225,13 @@ box.execute("INSERT INTO t VALUES(1, true);")
: > ...
: > box.execute("INSERT INTO t VALUES(1, 'asd');")
: > ---
: > -- null
: > -- 'Type mismatch: can not convert asd to varbinary'
: > +- row_count: 1
: > ...
: > box.execute("INSERT INTO t VALUES(1, x'616263');")
: > ---
: > -- row_count: 1
: > +- null
: > +- Duplicate key exists in unique index "pk_unnamed_T_1" in space "T" with
: old tuple
: > + - [1, "asd"] and new tuple - [1, "abc"]
: 31. Can you avoid this error? For example by changing 1 to 2. This would
: allow to avoid some changes below.
Yep, we may do so.
: > @@ -2385,13 +2395,14 @@ box.execute([[INSERT INTO tv(v) VALUES (true);]])
: > ...
: > box.execute([[INSERT INTO tv(v) VALUES ('33');]])
: > ---
: > -- null
: > -- 'Type mismatch: can not convert 33 to varbinary'
: > +- autoincrement_ids:
: > + - 2
: > + row_count: 1
: 32. Why this does not fails? I think RFC says this this cast is not allowed.
`string` is always convertible to `varbinary`.
: > @@ -2715,8 +2722,7 @@ box.execute([[UPDATE tv SET v = true WHERE a = 1;]])
: > ...
: > box.execute([[UPDATE tv SET v = '33' WHERE a = 1;]])
: > ---
: > -- null
: > -- 'Type mismatch: can not convert 33 to varbinary'
: > +- row_count: 1
: 33. Same.
`string` is convertible to `varbinary`.
Regards,
Timur
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines
2021-06-27 23:16 ` Timur Safin via Tarantool-patches
@ 2021-06-29 16:21 ` Igor Munkin via Tarantool-patches
2021-06-30 6:49 ` Timur Safin via Tarantool-patches
0 siblings, 1 reply; 18+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-06-29 16:21 UTC (permalink / raw)
To: Timur Safin; +Cc: alexander.turenko, tarantool-patches
Timur,
On 28.06.21, Timur Safin wrote:
>
> : From: Igor Munkin <imun@tarantool.org>
> : Subject: Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported
> : error lines
> :
> : Timur,
> :
> : Thanks for the patch! Though the change is quite trivial, the patch is
> : not OK right from the top. Please adjust the commit subject according to
> : our contribution guidelines[1].
>
> Much thanks Sasha for his useful proposal, not thanks to Igor for useless
> referring to guidelines without suggestion.
Sorry, my bad. If you need a suggestion, here you are: re-read our
contribution guidelines, especially the following bullet:
| Use the imperative mood in the subject line. A properly formed Git
| commit subject line should always be able to complete the following
| sentence: "If applied, this commit will /your subject line here/".
>
<snipped>
> :
> : Minor: Strictly saying :123 part is also broken.
>
> O_o, why? Could you please clarify? We report it as line number in the
> source file, in this case tkt-9a8b09f8e6.test.lua:123 was exactly the
> caller location?
Do you need the location of definition (:123) or location where the
function execution is stopped at the moment (:142)? If I got your point,
you prefer the latter, right?
>
<snipped>
> : > diff --git a/src/lua/tap.lua b/src/lua/tap.lua
> : > index 346724d84..77fd8d096 100644
> : > --- a/src/lua/tap.lua
> : > +++ b/src/lua/tap.lua
> : > @@ -23,7 +23,7 @@ local function traceback(level)
> : > local frame = {
> : > source = info.source;
> : > src = info.short_src;
> : > - line = info.linedefined or 0;
> : > + line = info.currentline or info.linedefined or 0;
> :
> : Why did you leave such a complex code here? I believe you can use just
> : info.currentline here.
>
> Just in case :)
Nice reasoning :)
> if currentline may be missing in the traceback object for any reason.
It may not, considering the flags used for obtaining debug info.
> If it's impossible for the set of requested flags, then I'll get rid
> of linedefined here.
Yes, <currentline> field is provided by 'l' flag[1]. Please, get rid of
<linedefined> and the default 0 and drop a few sentences regarding this
change in commit message.
>
> : : [1]: :
> https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/#how-
> : to-write-a-commit-message : : -- : Best regards, : IM
>
> Thanks, Timur
>
[1]: https://www.lua.org/manual/5.1/manual.html#lua_getinfo
--
Best regards,
IM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines
2021-06-29 16:21 ` Igor Munkin via Tarantool-patches
@ 2021-06-30 6:49 ` Timur Safin via Tarantool-patches
2021-07-21 7:24 ` Igor Munkin via Tarantool-patches
0 siblings, 1 reply; 18+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-06-30 6:49 UTC (permalink / raw)
To: 'Igor Munkin'; +Cc: alexander.turenko, tarantool-patches
: From: Igor Munkin <imun@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported
: error lines
:
: Timur,
:
: > :
: > : Minor: Strictly saying :123 part is also broken.
: >
: > O_o, why? Could you please clarify? We report it as line number in the
: > source file, in this case tkt-9a8b09f8e6.test.lua:123 was exactly the
: > caller location?
:
: Do you need the location of definition (:123) or location where the
: function execution is stopped at the moment (:142)? If I got your point,
: you prefer the latter, right?
FWIW, currentline is pointing _exactly_ to the line number in the file
Which was calling this error, not to the start of function definition.
At least in our cases, when we not introduce any extra do scopes.
[Now I start to worry with the question when .linedefined is not 0
and whether we would need to sum .currentline and .linedefined to
get actual source line in the file?]
: > if currentline may be missing in the traceback object for any reason.
:
: It may not, considering the flags used for obtaining debug info.
:
: > If it's impossible for the set of requested flags, then I'll get rid
: > of linedefined here.
:
: Yes, <currentline> field is provided by 'l' flag[1]. Please, get rid of
: <linedefined> and the default 0 and drop a few sentences regarding this
: change in commit message.
Please see my worries about .linedefined above. Currently it's in main scope,
when it will be not main scope, do any extra do create separate scope?
Could .currentline be .linedefined related or it's guaranteed to be
Counted from start of file?
:
: [1]: https://www.lua.org/manual/5.1/manual.html#lua_getinfo
:
: --
: Best regards,
: IM
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines
2021-06-30 6:49 ` Timur Safin via Tarantool-patches
@ 2021-07-21 7:24 ` Igor Munkin via Tarantool-patches
0 siblings, 0 replies; 18+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-07-21 7:24 UTC (permalink / raw)
To: Timur Safin; +Cc: alexander.turenko, tarantool-patches
Timur,
On 30.06.21, Timur Safin wrote:
> : From: Igor Munkin <imun@tarantool.org>
> : Subject: Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported
> : error lines
> :
> : Timur,
> :
>
> : > :
> : > : Minor: Strictly saying :123 part is also broken.
> : >
> : > O_o, why? Could you please clarify? We report it as line number in the
> : > source file, in this case tkt-9a8b09f8e6.test.lua:123 was exactly the
> : > caller location?
> :
> : Do you need the location of definition (:123) or location where the
> : function execution is stopped at the moment (:142)? If I got your point,
> : you prefer the latter, right?
>
> FWIW, currentline is pointing _exactly_ to the line number in the file
> Which was calling this error, not to the start of function definition.
Yes, this is written in Lua Reference Manual[1].
> At least in our cases, when we not introduce any extra do scopes.
What is wrong with extra do scopes?
>
> [Now I start to worry with the question when .linedefined is not 0
> and whether we would need to sum .currentline and .linedefined to
> get actual source line in the file?]
>
> : > if currentline may be missing in the traceback object for any reason.
> :
> : It may not, considering the flags used for obtaining debug info.
> :
> : > If it's impossible for the set of requested flags, then I'll get rid
> : > of linedefined here.
> :
> : Yes, <currentline> field is provided by 'l' flag[1]. Please, get rid of
> : <linedefined> and the default 0 and drop a few sentences regarding this
> : change in commit message.
>
> Please see my worries about .linedefined above. Currently it's in main scope,
> when it will be not main scope, do any extra do create separate scope?
Lexical blocks are described here[2].
> Could .currentline be .linedefined related or it's guaranteed to be
> Counted from start of file?
I see nothing about such odd behaviour in Reference Manual[1], but you
can check manually whether LuaJIT works fine in this case.
>
> :
> : [1]: https://www.lua.org/manual/5.1/manual.html#lua_getinfo
> :
> : --
> : Best regards,
> : IM
>
[1]: https://www.lua.org/manual/5.1/manual.html#3.8
[2]: https://www.lua.org/manual/5.1/manual.html#2.4.2
--
Best regards,
IM
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-07-21 7:48 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-11 7:48 [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Timur Safin via Tarantool-patches
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines Timur Safin via Tarantool-patches
2021-06-20 18:57 ` Igor Munkin via Tarantool-patches
2021-06-23 21:01 ` Alexander Turenko via Tarantool-patches
2021-06-27 23:16 ` Timur Safin via Tarantool-patches
2021-06-29 16:21 ` Igor Munkin via Tarantool-patches
2021-06-30 6:49 ` Timur Safin via Tarantool-patches
2021-07-21 7:24 ` Igor Munkin via Tarantool-patches
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 2/3] sql: updated explicit conversion table Timur Safin via Tarantool-patches
2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches
2021-06-25 21:26 ` Timur Safin via Tarantool-patches
2021-06-25 21:26 ` [Tarantool-patches] Отзыв: " Timur Safin via Tarantool-patches
2021-06-27 23:46 ` [Tarantool-patches] " Timur Safin via Tarantool-patches
2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 3/3] sql: updated implicit " Timur Safin via Tarantool-patches
2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches
2021-06-28 0:06 ` Timur Safin via Tarantool-patches
2021-06-20 18:52 ` [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Mergen Imeev via Tarantool-patches
2021-06-27 23:29 ` Timur Safin via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox