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 C39D52D925 for ; Wed, 31 Oct 2018 08:34:56 -0400 (EDT) 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 uWvyjWKbHrEC for ; Wed, 31 Oct 2018 08:34:56 -0400 (EDT) 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 22F332D91B for ; Wed, 31 Oct 2018 08:34:56 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] sql: change collation compatibility rules References: <3fdf7287dc5fc2636a3cbc0803c8ba90a4c8c623.1540460716.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 31 Oct 2018 15:34:53 +0300 MIME-Version: 1.0 In-Reply-To: <3fdf7287dc5fc2636a3cbc0803c8ba90a4c8c623.1540460716.git.korablev@tarantool.org> 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 patch! See 5 comments below. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index a13de4f0e..bd25ed656 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -316,25 +315,62 @@ binaryCompareP5(Expr * pExpr1, Expr * pExpr2, int jumpIfNull) > return aff; > } > > +int > +collations_are_compatible(uint32_t lhs_id, bool is_lhs_forced, > + uint32_t rhs_id, bool is_rhs_forced, > + uint32_t *res_id) 1. Why not to rename to _check_compatibility() and set diag right here? > +{ > + assert(res_id != NULL); > + if (is_lhs_forced && is_rhs_forced) { > + if (lhs_id != rhs_id) > + return -1; > + } > + if (is_lhs_forced) { > + *res_id = lhs_id; > + return 0; > + } > + if (is_rhs_forced) { > + *res_id = rhs_id; > + return 0; > + } > + if (lhs_id != rhs_id) { > + if (lhs_id == COLL_NONE) { > + *res_id = rhs_id; > + return 0; > + } > + if (rhs_id == COLL_NONE) { > + *res_id = lhs_id; > + return 0; > + } > + return -1; > + } > + *res_id = lhs_id; > + return 0; > +} > + > struct coll * > sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right, > uint32_t *coll_id) > { > - struct coll *coll; > - bool is_found; > assert(left != NULL); > - if ((left->flags & EP_Collate) != 0) { > - coll = sql_expr_coll(parser, left, &is_found, coll_id); > - } else if (right != NULL && (right->flags & EP_Collate) != 0) { > - coll = sql_expr_coll(parser, right, &is_found, coll_id); > - } else { > - coll = sql_expr_coll(parser, left, &is_found, coll_id); > - if (! is_found) { > - coll = sql_expr_coll(parser, right, &is_found, > - coll_id); > - } > + bool is_lhs_forced; > + bool is_rhs_forced; > + uint32_t lhs_coll_id; > + uint32_t rhs_coll_id; > + struct coll *lhs_coll = sql_expr_coll(parser, left, &is_lhs_forced, > + &lhs_coll_id); > + struct coll *rhs_coll = sql_expr_coll(parser, right, &is_rhs_forced, > + &rhs_coll_id); > + if (collations_are_compatible(lhs_coll_id, is_lhs_forced, > + rhs_coll_id, is_rhs_forced, > + coll_id) != 0) { > + diag_set(ClientError, ER_ILLEGAL_COLLATION_MIX); > + parser->rc = SQL_TARANTOOL_ERROR; > + parser->nErr++; > + *coll_id = COLL_NONE; 3. Why do you need to set coll_id in a case of an error? As I understand, after an error the compilation is terminated. > + return NULL; > } > - return coll; > + return *coll_id == rhs_coll_id ? rhs_coll : lhs_coll;; > } > > /* > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 1ec92aac7..c3403670b 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -2035,8 +2035,9 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, /* Parsing contexts */ > bool is_found; > uint32_t coll_id; > if (pTab->def->fields[i].coll_id == COLL_NONE && > - sql_expr_coll(pParse, p, &is_found, &coll_id) && is_found) > + sql_expr_coll(pParse, p, &is_found, &coll_id) && coll_id != COLL_NONE) > pTab->def->fields[i].coll_id = coll_id; > + assert(pTab->def->fields[i].coll_id != COLL_BINARY); 4. Why? I can create a table with binary collation after the previous commit. > } > } > > @@ -2223,47 +2224,53 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak) > +static uint32_t > +multi_select_coll_seq(struct Parse *parser, struct Select *p, int n, > + bool *is_forced_coll) > { > - struct coll *coll; > + bool is_prior_forced = false; > + bool is_current_forced; > + uint32_t prior_coll_id = COLL_NONE; > + uint32_t current_coll_id; > if (p->pPrior != NULL) { > - coll = multi_select_coll_seq_r(parser, p->pPrior, n, is_found, > - coll_id); > - } else { > - coll = NULL; > - *coll_id = COLL_NONE; > + prior_coll_id = multi_select_coll_seq(parser, p->pPrior, n, > + &is_prior_forced); > } > - assert(n >= 0); > - /* iCol must be less than p->pEList->nExpr. Otherwise an error would > - * have been thrown during name resolution and we would not have gotten > - * this far > + /* > + * Column number must be less than p->pEList->nExpr. > + * Otherwise an error would have been thrown during name > + * resolution and we would not have gotten this far. 5. 'have gotten'? Maybe 'have got'? > */ > - if (!*is_found && ALWAYS(n < p->pEList->nExpr)) { > - coll = sql_expr_coll(parser, p->pEList->a[n].pExpr, is_found, > - coll_id); > - } > - return coll; > -}