From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D44992EE1E for ; Tue, 13 Nov 2018 07:02:44 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id gD9fOg-cqkS3 for ; Tue, 13 Nov 2018 07:02:44 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 937672EE13 for ; Tue, 13 Nov 2018 07:02:44 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 3/3] sql: change collation compatibility rules References: From: Vladislav Shpilevoy Message-ID: <105845b9-019b-9b37-4812-898c0fa4d4be@tarantool.org> Date: Tue, 13 Nov 2018 15:02:41 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Nikita Pettik , tarantool-patches@freelists.org Thanks for the fixes! See 2 comments below, my fixes at the end of the email and on the branch. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index cea453f08..ab0376a1d 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -2150,47 +2151,51 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak) > } > > #ifndef SQLITE_OMIT_COMPOUND_SELECT > -static struct coll * > -multi_select_coll_seq_r(Parse *parser, Select *p, int n, bool *is_found, > - uint32_t *coll_id) > +/** > + * This function determines resulting collation sequence for > + * @n-th column of the result set for the compound SELECT > + * statement. Since compound SELECT performs implicit comparisons > + * between values, all parts of compound queries must use > + * the same collation. Otherwise, an error is raised. > + * > + * @param parser Parse context. > + * @param p Select meta-information. > + * @param n Column number of the result set. > + * @param is_forced_coll Used if we fall into recursion. > + * For most-outer call it is unused. Used to indicate that > + * explicit COLLATE clause is used. > + * @retval Id of collation to be used during string comparison. > + */ > +static uint32_t > +multi_select_coll_seq(struct Parse *parser, struct Select *p, int n, > + bool *is_forced_coll) 1. I renamed this function into multi_select_coll_seq_r and created a wrapper without is_forced_coll to remove some of 'bool unused' things. Apply or drop the fix, up to you. > diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c > index 9fa6ce15d..e05967554 100644 > --- a/src/box/sql/whereexpr.c > +++ b/src/box/sql/whereexpr.c > @@ -168,7 +168,7 @@ exprCommute(Parse * pParse, Expr * pExpr) > bool is_found; > uint32_t id; > sql_expr_coll(pParse, pExpr->pLeft, &is_found, &id); > - if (is_found) { > + if (id != 0) { 2. If you fix a comment about COLL_NONE constant return, do not forget to pick this hunk up as well, please. Maybe this patch contains more, I am not sure. > /* > * Neither X nor Y have COLLATE > * operators, but X has a ==================================================================== commit b138616a1851dbe905a2e86b981f36793a7eb117 Author: Vladislav Shpilevoy Date: Tue Nov 13 14:39:45 2018 +0300 Speculative review fixes diff --git a/src/box/sql/select.c b/src/box/sql/select.c index ffbe83aa5..2db8e1c7f 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -2167,16 +2167,16 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak) * @retval Id of collation to be used during string comparison. */ static uint32_t -multi_select_coll_seq(struct Parse *parser, struct Select *p, int n, - bool *is_forced_coll) +multi_select_coll_seq_r(struct Parse *parser, struct Select *p, int n, + bool *is_forced_coll) { bool is_prior_forced = false; bool is_current_forced; uint32_t prior_coll_id = 0; uint32_t current_coll_id; if (p->pPrior != NULL) { - prior_coll_id = multi_select_coll_seq(parser, p->pPrior, n, - &is_prior_forced); + prior_coll_id = multi_select_coll_seq_r(parser, p->pPrior, n, + &is_prior_forced); } /* * Column number must be less than p->pEList->nExpr. @@ -2198,6 +2198,13 @@ multi_select_coll_seq(struct Parse *parser, struct Select *p, int n, return res_coll_id; } +static inline uint32_t +multi_select_coll_seq(struct Parse *parser, struct Select *p, int n) +{ + bool unused; + return multi_select_coll_seq_r(parser, p, n, &unused); +} + /** * The select statement passed as the second parameter is a * compound SELECT with an ORDER BY clause. This function @@ -2237,8 +2244,7 @@ sql_multiselect_orderby_to_key_info(struct Parse *parse, struct Select *s, sql_expr_coll(parse, term, &unused, &id); } else { id = multi_select_coll_seq(parse, s, - item->u.x.iOrderByCol - 1, - &unused); + item->u.x.iOrderByCol - 1); if (id != COLL_NONE) { const char *name = coll_by_id(id)->name; order_by->a[i].pExpr = @@ -2900,10 +2906,9 @@ multiSelect(Parse * pParse, /* Parsing context */ struct sql_key_info *key_info = sql_key_info_new(db, nCol); if (key_info == NULL) goto multi_select_end; - bool unused; for (int i = 0; i < nCol; i++) { key_info->parts[i].coll_id = - multi_select_coll_seq(pParse, p, i, &unused); + multi_select_coll_seq(pParse, p, i); } for (struct Select *pLoop = p; pLoop; pLoop = pLoop->pPrior) { @@ -3328,12 +3333,10 @@ multiSelectOrderBy(Parse * pParse, /* Parsing context */ pParse->nMem += expr_count + 1; sqlite3VdbeAddOp2(v, OP_Integer, 0, regPrev); key_info_dup = sql_key_info_new(db, expr_count); - bool unused; if (key_info_dup != NULL) { for (int i = 0; i < expr_count; i++) { key_info_dup->parts[i].coll_id = - multi_select_coll_seq(pParse, p, i, - &unused); + multi_select_coll_seq(pParse, p, i); } } } diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index cfe87c62a..470320e8d 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -4330,7 +4330,8 @@ const char *sqlite3ErrStr(int); * * @param parse Parsing context. * @param expr Expression to scan. - * @param[out] is_found Flag set if explicit COLLATE clause is used. + * @param[out] is_explicit_coll Flag set if explicit COLLATE + * clause is used. * @param[out] coll_id Collation identifier. * * @retval Pointer to collation.