[Tarantool-patches] [PATCH v1 1/7] sql: rework implicit cast fo assignment
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Jul 31 00:55:05 MSK 2021
Thanks for the patch!
I will return later to continue the review of the next commits.
Sending first comments so as I could switch to other patches.
See 10 comments below.
> diff --git a/changelogs/unreleased/gh-4470-implicit-cast-for-assignment.md b/changelogs/unreleased/gh-4470-implicit-cast-for-assignment.md
> new file mode 100644
> index 000000000..c758494eb
> --- /dev/null
> +++ b/changelogs/unreleased/gh-4470-implicit-cast-for-assignment.md
> @@ -0,0 +1,3 @@
> +## feature/sql
> +
> +* Implicit cast for assignment now works according to defined rules (gh-4470).
1. Please, be more specific about what exactly has changed.
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 351d80b76..e804dba67 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -45,6 +45,8 @@
> #include "uuid/mp_uuid.h"
> #include "mp_decimal.h"
>
> +#define CMP_OLD_NEW(a, b, type) ((int)(a > (type)b) - (int)(a < (type)b))
2. Please, wrap 'a' and 'b' into parentheses. Otherwise the macro might
work in unexpected ways if these are expressions.
> @@ -900,6 +902,92 @@ uuid_to_bin(struct Mem *mem)
> return mem_copy_bin(mem, (char *)&mem->u.uuid, UUID_LEN);
> }
>
> +static inline int
> +forced_int_to_double(struct Mem *mem)
> +{
> + assert(mem->type == MEM_TYPE_INT || mem->type == MEM_TYPE_UINT);
> + double d;
> + int res;
> + if (mem->type == MEM_TYPE_INT) {
> + d = (double)mem->u.i;
> + res = CMP_OLD_NEW(mem->u.i, d, int64_t);
> + } else {
> + d = (double)mem->u.u;
> + res = CMP_OLD_NEW(mem->u.u, d, uint64_t);
> + }
> + mem->u.r = d;
> + mem->type = MEM_TYPE_DOUBLE;
> + mem->field_type = FIELD_TYPE_DOUBLE;
3. Why do you need not to fail in case of imprecise conversion? It don't
see -1 and 1 values used. Only != 0 in the end where the implicit casts
are applied.
> + return res;
> +}
> +
> +static inline int
> +forced_int_to_uint(struct Mem *mem)
> +{
> + assert(mem->type == MEM_TYPE_INT || mem->type == MEM_TYPE_UINT);
4. It is never used with MEM_TYPE_UINT from what I see.
> + if (mem->type == MEM_TYPE_UINT)
> + return 0;
> + mem->u.u = 0;
> + mem->type = MEM_TYPE_UINT;
> + mem->field_type = FIELD_TYPE_UNSIGNED;
> + return -1;
> +}
> @@ -1228,6 +1316,53 @@ mem_cast_implicit_old(struct Mem *mem, enum field_type type)
> return -1;
> }
>
> +int
> +mem_cast_implicit_number(struct Mem *mem, enum field_type type)
> +{
> + assert(mem_is_num(mem) && sql_type_is_numeric(type));
> + switch (type) {
> + case FIELD_TYPE_UNSIGNED:
> + switch (mem->type) {
> + case MEM_TYPE_UINT:
> + mem->field_type = FIELD_TYPE_UNSIGNED;
> + return 0;
> + case MEM_TYPE_INT:
> + return forced_int_to_uint(mem);
> + case MEM_TYPE_DOUBLE:
> + return forced_double_to_uint(mem);
> + default:
> + unreachable();
> + }
> + break;
> + case FIELD_TYPE_DOUBLE:
> + switch (mem->type) {
> + case MEM_TYPE_INT:
> + case MEM_TYPE_UINT:
> + return forced_int_to_double(mem);
5. You already have the switch here and yet you have a branch
by MEM_TYPE_UINT vs MEM_TYPE_INT inside forced_int_to_double.
Maybe split it 2 for INT and UINT separately and use them
right under each 'case'?
> diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
> index 645d0ee27..9766bb836 100644
> --- a/src/box/sql/mem.h
> +++ b/src/box/sql/mem.h
> @@ -779,6 +779,16 @@ mem_cast_implicit(struct Mem *mem, enum field_type type);
> int
> mem_cast_implicit_old(struct Mem *mem, enum field_type type);
>
> +/**
> + * Convert the given MEM that contains numeric value to given numeric type
> + * according to implicit cast rules. This function cannot fail. Returns:
> + * -1 if before conversion value was more that after conversion;
6. 'more' -> 'greater', 'that' -> 'than'.
> + * +1 if before conversion value was more that after conversion;
> + * 0 if conversion is precise.
> + */
> +int
> +mem_cast_implicit_number(struct Mem *mem, enum field_type type);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 9e763ed85..d143ce364 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2157,11 +2157,24 @@ case OP_ApplyType: {
> while((type = *(types++)) != field_type_MAX) {
> assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
> assert(memIsValid(pIn1));
> - if (mem_cast_implicit(pIn1, type) != 0) {
> + if (mem_is_field_compatible(pIn1, type)) {
> + pIn1++;
> + continue;
> + }
> + if (!mem_is_num(pIn1) || !sql_type_is_numeric(type)) {
> diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> mem_str(pIn1), field_type_strs[type]);
> goto abort_due_to_error;
> }
> + struct Mem mem;
> + mem.type = pIn1->type;
> + mem.u = pIn1->u;
> + mem.flags = 0;
7. The mem is only used for the error message. Please, move it
under the 'if' right before diag_set so as not to create it for
success path. And why can't you use pIn1 in mem_str() below?
> + if (mem_cast_implicit_number(pIn1, type) != 0) {
> + diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> + mem_str(&mem), field_type_strs[type]);
> + goto abort_due_to_error;
> + }
> pIn1++;
> }
> break;
> diff --git a/test/sql-tap/numcast.test.lua b/test/sql-tap/numcast.test.lua
> index 802fe712c..be1366260 100755
> --- a/test/sql-tap/numcast.test.lua
> +++ b/test/sql-tap/numcast.test.lua
> @@ -136,13 +136,13 @@ test:do_catchsql_test(
> 1,"Type mismatch: can not convert double(2.0e+19) to integer"
> })
>
> -test:do_execsql_test(
> +test:do_catchsql_test(
> "cast-2.9",
> [[
> INSERT INTO t VALUES(2.1);
> SELECT * FROM t;
8. The select is not needed now.
> ]], {
> - 2, 9223372036854775808ULL, 18000000000000000000ULL
> + 1, "Type mismatch: can not convert double(2.1) to integer"
> })
>
> --
> diff --git a/test/sql-tap/uuid.test.lua b/test/sql-tap/uuid.test.lua
> index 70683a4fd..613f4c865 100755
> --- a/test/sql-tap/uuid.test.lua
> +++ b/test/sql-tap/uuid.test.lua
> @@ -844,13 +837,13 @@ test:do_catchsql_test(
> 1, "Type mismatch: can not convert boolean(TRUE) to uuid"
> })
>
> -test:do_execsql_test(
> +test:do_catchsql_test(
> "uuid-8.2.7",
> [[
> INSERT INTO tsu SELECT '7_varbinary', x'11111111111111111111111111111111' FROM t2 LIMIT 1;
> SELECT * FROM tsu ORDER BY s DESC LIMIT 1;
9. The select is unreachable.
> ]], {
> - '7_varbinary', uuid1
> + 1, "Type mismatch: can not convert varbinary(x'11111111111111111111111111111111') to uuid"
> })
> @@ -882,59 +875,48 @@ test:execsql([[
<...>
> -test:do_execsql_test(
> +test:do_catchsql_test(
> "uuid-10.1.3",
> [[
> INSERT INTO t10(i) VALUES (3);
> - SELECT * FROM t10 WHERE i = 3;
> ]], {
> - 3, uuid1
> + 1, "Type mismatch: can not convert string('11111111-1111-1111-1111-111111111111') to uuid"
10. I suspect this test was about checking if UUID DEFAULT works.
Maybe better fix its DEFAULT in the table definition so as not to
use a string. Is it possible to use CAST in DEFAULT?
More information about the Tarantool-patches
mailing list