[Tarantool-patches] [PATCH v2 3/3] sql: updated implicit conversion table
Timur Safin
tsafin at tarantool.org
Mon Jun 28 03:06:41 MSK 2021
: From: Mergen Imeev <imeevma at 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
More information about the Tarantool-patches
mailing list