Re: [Tarantool-patches] [PATCH v1 2/7] sql: remove implicit cast from comparison opcodes

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

Thanks for the patch!

See 5 comments below.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index e804dba67..b04303be2 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -1976,25 +2000,21 @@ mem_bit_not(const struct Mem *mem, struct Mem *result)
>  	return 0;
>  }
>  
> -int
> -mem_cmp_bool(const struct Mem *a, const struct Mem *b, int *result)
> +static int
> +mem_cmp_bool(const struct Mem *a, const struct Mem *b)
>  {
> -	if ((a->type & b->type & MEM_TYPE_BOOL) == 0)
> -		return -1;
> +	assert((a->type & b->type & MEM_TYPE_BOOL) != 0);
>  	if (a->u.b == b->u.b)
> -		*result = 0;
> -	else if (a->u.b)
> -		*result = 1;
> -	else
> -		*result = -1;
> -	return 0;
> +		return 0;
> +	if (a->u.b)
> +		return 1;
> +	return -1;

1. Could be simpler:

====================
@@ -2004,11 +2004,7 @@ static int
 mem_cmp_bool(const struct Mem *a, const struct Mem *b)
 {
 	assert((a->type & b->type & MEM_TYPE_BOOL) != 0);
-	if (a->u.b == b->u.b)
-		return 0;
-	if (a->u.b)
-		return 1;
-	return -1;
+	return a->u.b - b->u.b;
 }
====================

> @@ -2245,8 +2189,11 @@ mem_cmp_msgpack(const struct Mem *a, const char **b, int *result,
>  		if (type == MP_UUID) {
>  			assert(len == UUID_LEN);
>  			mem.type = MEM_TYPE_UUID;
> -			if (uuid_unpack(b, len, &mem.u.uuid) == NULL)
> +			if (uuid_unpack(b, len, &mem.u.uuid) == NULL) {
> +				diag_set(ClientError, ER_SQL_EXECUTE,
> +					 "cannot parse UUID");

2. Shouldn't this be a separate commit as a bugfix?

>  				return -1;
> +			}
>  			break;
>  		}
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index d143ce364..62f58def9 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
<...>

> -	switch( pOp->opcode) {
> -	case OP_Eq:    res2 = res==0;     break;
> -	case OP_Ne:    res2 = res;        break;
> -	case OP_Lt:    res2 = res<0;      break;
> -	case OP_Le:    res2 = res<=0;     break;
> -	case OP_Gt:    res2 = res>0;      break;
> -	default:       res2 = res>=0;     break;
> +	bool result;
> +	switch(pOp->opcode) {

3. It does not look good to have a second switch-case here. But I
can't find a better solution right away. We can't wrap this all
into a function, because need to make goto abort_due_to_error and
goto jump_to_p2 in some places. Up to you if you want to find a
way to fix it.

As a side note, the code now is incomparably simpler than it was.
It is getting actually understable, nice.

> +	case OP_Eq:
> +		result = cmp_res == 0;
> +		break;
> +	case OP_Ne:
> +		result = cmp_res != 0;
> +		break;
> +	case OP_Lt:
> +		result = cmp_res < 0;
> +		break;
> +	case OP_Le:
> +		result = cmp_res <= 0;
> +		break;
> +	case OP_Gt:
> +		result = cmp_res > 0;
> +		break;
> +	case OP_Ge:
> +		result = cmp_res >= 0;
> +		break;
> +	default:
> +		unreachable();
>  	}
> diff --git a/test/sql-tap/transitive1.test.lua b/test/sql-tap/transitive1.test.lua
> index dbc2559fa..2d502f5e8 100755
> --- a/test/sql-tap/transitive1.test.lua
> +++ b/test/sql-tap/transitive1.test.lua
> @@ -73,7 +73,7 @@ test:do_execsql_test(
>  test:do_execsql_test(
>      "transitive1-210",
>      [[
> -        SELECT a,b,c FROM t2 WHERE a=b AND c=b AND c>='20' ORDER BY +a;
> +        SELECT a,b,c FROM t2 WHERE a = b AND c >= '20' ORDER BY +a;

4. Might worth adding whitespaces after ',' in the select list.

>      ]], {
>          -- <transitive1-210>
>          3, 3, "3", 20, 20, "20"
> diff --git a/test/sql/boolean.result b/test/sql/boolean.result
> index d54de8fe7..81d79ee78 100644
> --- a/test/sql/boolean.result
> +++ b/test/sql/boolean.result
> @@ -4898,12 +4898,12 @@ SELECT a1, a1 != 2.3 FROM t6
>  SELECT a1, 2.3 == a1 FROM t6
>   | ---
>   | - null
> - | - 'Type mismatch: can not convert double(2.3) to boolean'
> + | - 'Type mismatch: can not convert boolean(FALSE) to number'

5. Is it possible to keep the old error messages? For the sake of
smaller diff.