Re: [Tarantool-patches] [PATCH v1 2/2] sql: fix comparison in IN with subselect

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 6 comments below.

On 15/04/2020 13:47, imeevma@tarantool.org wrote:
> After this patch, the IN statement checks the compatibility of the
> values from subselect ​​on the right with the value on the left. If

1. I see broken unicode after 'subselect' on this line, when I do
'git log'.

> values from subselect contains a value that is not comparable with
> the left value, it throws an error.
> 
> Closes #4692
> ---
>  src/box/sql/expr.c                                 | 27 ++++++++++++++++++++++
>  .../gh-4692-comparison-in-IN-operator.test.lua     | 24 ++++++++++++++++++-
>  test/sql-tap/in3.test.lua                          |  2 +-
>  test/sql-tap/subquery.test.lua                     |  8 +++----
>  test/sql-tap/tkt-80e031a00f.test.lua               |  8 +++----
>  test/sql/boolean.result                            | 12 +++++-----
>  6 files changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 4fe4f83..c20c04b 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -2711,6 +2711,28 @@ expr_in_type(Parse *pParse, Expr *pExpr)
>  	return zRet;
>  }
>  
> +static inline bool
> +is_in_type_cmp(struct Parse *parse, enum field_type lhs_type, struct Expr *expr)

2. Sorry, can't parse the name. Can't understand what is 'is cmp'.
Could you please rename and add a comment maybe?

> +{
> +	uint32_t fieldno = expr->iColumn;
> +	enum field_type rhs_type = expr->space_def == NULL ?
> +				   rhs_type = expr->type :

3. You basically wrote 'rhs_type = (rhs_type = expr->type)'. You
may need to remove the second 'rhs_type = '.

> +				   expr->space_def->fields[fieldno].type;
> +	if (rhs_type == lhs_type)
> +		return true;
> +	if (rhs_type == FIELD_TYPE_ANY || lhs_type == FIELD_TYPE_ANY)
> +		return true;
> +	if (rhs_type == FIELD_TYPE_SCALAR || lhs_type == FIELD_TYPE_SCALAR)
> +		return true;
> +	if (sql_type_is_numeric(rhs_type) && sql_type_is_numeric(lhs_type))
> +		return true;
> +	parse->is_aborted = true;
> +	diag_set(ClientError, ER_SQL_TYPE_MISMATCH, field_type_strs[rhs_type],
> +		 field_type_strs[lhs_type]);

4. Wouldn't it be the same to check

    field_type1_contains_type2(lhs_type, rhs_type) ||
    field_type1_contains_type2(rhs_type, lhs_type)

?

> +	return false;
> +}
> +
> +
>  /*
>   * Generate code for scalar subqueries used as a subquery expression, EXISTS,
>   * or IN operators.  Examples:
> @@ -2821,6 +2843,11 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
>  							      pExpr->iTable, reg_eph);
>  					dest.dest_type =
>  						expr_in_type(pParse, pExpr);
> +					if (nVal == 1 &&

5. What if it is not 1? How types are checked then?

> +					    !is_in_type_cmp(pParse,
> +							    sql_expr_type(pLeft),
> +							    pEList->a[0].pExpr))
> +						return 0;
>  					assert((pExpr->iTable & 0x0000FFFF) ==
>  					       pExpr->iTable);
>  					pSelect->iLimit = 0;
> diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua
> index 6bedf58..e03a6a0 100755
> --- a/test/sql-tap/subquery.test.lua
> +++ b/test/sql-tap/subquery.test.lua
> @@ -358,12 +358,12 @@ test:do_test(
>          -- comparision. Hence, the integer value 10 in t3 will compare equal
>          -- to the string value '10.0' in t4 because the t4 value will be
>          -- converted into an integer.
> -        return test:execsql [[
> +        return test:catchsql [[

6. The comment above is outdated. Probably better to remove this test,
since it no longer serves its purpose. Or change the comment. Please,
check other changed tests too. Maybe they also became irrelevant or their
comments are misleading.