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

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

Hi! Thanks for the fixes!

>>> 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?
>>
> Fixed. I gave it a new name, 'is_comparable'. It is not a
> lot better, still since it is just a static inline function,
> will this be enough?

I am afraid it is too common for a huge file like expr.c. Just
extend the old name "is_in_type_cmp()" -> "are_in_args_comparable()".

> From 946cd381dd0af8ba1b534e9721417423f315a542 Mon Sep 17 00:00:00 2001
> From: Mergen Imeev <imeevma@gmail.com>
> Date: Wed, 15 Apr 2020 02:17:43 +0300
> Subject: [PATCH] sql: fix comparison in IN with subselect
> 
> After this patch, the IN statement checks the compatibility of the
> values from subselect on the right with the value on the left. If
> values from subselect contains a value that is not comparable with
> the left value, it throws an error.
> 
> Closes #4692
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 4fe4f83..9cd3a2c 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -2711,6 +2711,49 @@ expr_in_type(Parse *pParse, Expr *pExpr)
>  	return zRet;
>  }
>  
> +/**
> + * Check that arguments on both sides of IN are comparable.
> + *
> + * @param parse Parsing context.
> + * @param pExpr Expr that contains all operands.
> + *
> + * @retval true if comparable, false otherwise.
> + */
> +static inline bool
> +is_comparable(struct Parse *parse, struct Expr *pExpr)
> +{
> +	uint32_t size = sqlExprVectorSize(pExpr->pLeft);
> +	ExprList *lhs_list = NULL;
> +	ExprList *rhs_list = pExpr->x.pSelect->pEList;
> +	u8 op = pExpr->pLeft->op;
> +	if (op == TK_REGISTER)
> +		op = pExpr->pLeft->op2;
> +	if (op == TK_VECTOR)
> +		lhs_list = pExpr->pLeft->x.pList;
> +	else if (op == TK_SELECT)
> +		lhs_list = pExpr->pLeft->x.pSelect->pEList;
> +	assert(size == 1 || lhs_list != NULL);

What is happening on the lines above?

Also, I see similar code below is_comparable() invocation:

	for (i = 0; i < nVal; i++) {
		Expr *p =
		    sqlVectorFieldSubexpr
		    (pLeft, i);
		if (sql_binary_compare_coll_seq(pParse, p, pEList->a[i].pExpr,
						&key_info->parts[i].coll_id) != 0)
			return 0;
	}

It is ugly, but the point is they use sqlVectorFieldSubexpr(pLeft, i)
without any 'TK_VECTOR', 'TK_SELECT', 'TK_REGISTER' checks. How
does it work? And shouldn't we move this cycle into is_comparable(),
because seems like it also checks if types are comparable when they
are strings. However I am not sure.

> +
> +	for (uint32_t i = 0; i < size; ++i) {
> +		struct Expr *lhs = lhs_list == NULL ? pExpr->pLeft :
> +				   lhs_list->a[i].pExpr;
> +		struct Expr *rhs = rhs_list->a[i].pExpr;
> +		enum field_type lhs_type = sql_expr_type(lhs);
> +		enum field_type rhs_type = sql_expr_type(rhs);
> +		if (field_type1_contains_type2(lhs_type, rhs_type) ||
> +		    field_type1_contains_type2(rhs_type, lhs_type))
> +			continue;
> +		if (sql_type_is_numeric(rhs_type) &&
> +		    sql_type_is_numeric(lhs_type))
> +			continue;
> +		parse->is_aborted = true;
> +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +			 field_type_strs[rhs_type], field_type_strs[lhs_type]);
> +		return false;
> +	}
> +	return true;
> +}