From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D732C4696C3 for ; Sun, 19 Apr 2020 20:47:25 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Sun, 19 Apr 2020 19:47:23 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v1 2/2] sql: fix comparison in IN with subselect List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: imeevma@tarantool.org, tsafin@tarantool.org, tarantool-patches@dev.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.