From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 65BA34696C3 for ; Tue, 28 Apr 2020 01:53:15 +0300 (MSK) References: <20200422100303.GA29050@tarantool.org> From: Vladislav Shpilevoy Message-ID: <89021f4c-beb3-e23b-038d-ad41f017e94e@tarantool.org> Date: Tue, 28 Apr 2020 00:53:11 +0200 MIME-Version: 1.0 In-Reply-To: <20200422100303.GA29050@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Mergen Imeev Cc: tarantool-patches@dev.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 > 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; > +}