[Tarantool-patches] [PATCH v1 2/2] sql: fix comparison in IN with subselect
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Apr 28 01:53:11 MSK 2020
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 at 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;
> +}
More information about the Tarantool-patches
mailing list