Re: [Tarantool-patches] [PATCH v1 1/7] sql: rework implicit cast fo assignment

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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?