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 192122CF28 for ; Mon, 12 Nov 2018 18:46:45 -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 WSpM3qof-kFW for ; Mon, 12 Nov 2018 18:46:44 -0500 (EST) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 4EF972CEBC for ; Mon, 12 Nov 2018 18:46:44 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH 3/3] sql: change collation compatibility rules From: "n.pettik" In-Reply-To: Date: Tue, 13 Nov 2018 02:46:34 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <9E3A4492-F426-402B-808D-A31CEF743A47@tarantool.org> References: <3fdf7287dc5fc2636a3cbc0803c8ba90a4c8c623.1540460716.git.korablev@tarantool.org> 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: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy After discussion with Konstantin in @dev mailing list, we have decided to introduce real entities in _collation space to represent binary and =E2=80=9Cnone=E2=80=9D collations. Hence, I am going to send = updated patch-set as second iteration (v2). Also, I=E2=80=99ve took your comments into consideration. >> 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) >=20 > 1. Why not to rename to _check_compatibility() and set > diag right here? Just another way to implement this. In updated patch I did it as you suggested. >> +{ >> + assert(res_id !=3D NULL); >> + if (is_lhs_forced && is_rhs_forced) { >> + if (lhs_id !=3D rhs_id) >> + return -1; >> + } >> + if (is_lhs_forced) { >> + *res_id =3D lhs_id; >> + return 0; >> + } >> + if (is_rhs_forced) { >> + *res_id =3D rhs_id; >> + return 0; >> + } >> + if (lhs_id !=3D rhs_id) { >> + if (lhs_id =3D=3D COLL_NONE) { >> + *res_id =3D rhs_id; >> + return 0; >> + } >> + if (rhs_id =3D=3D COLL_NONE) { >> + *res_id =3D lhs_id; >> + return 0; >> + } >> + return -1; >> + } >> + *res_id =3D 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 !=3D NULL); >> - if ((left->flags & EP_Collate) !=3D 0) { >> - coll =3D sql_expr_coll(parser, left, &is_found, = coll_id); >> - } else if (right !=3D NULL && (right->flags & EP_Collate) !=3D = 0) { >> - coll =3D sql_expr_coll(parser, right, &is_found, = coll_id); >> - } else { >> - coll =3D sql_expr_coll(parser, left, &is_found, = coll_id); >> - if (! is_found) { >> - coll =3D 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 =3D sql_expr_coll(parser, left, = &is_lhs_forced, >> + &lhs_coll_id); >> + struct coll *rhs_coll =3D 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) !=3D 0) { >> + diag_set(ClientError, ER_ILLEGAL_COLLATION_MIX); >> + parser->rc =3D SQL_TARANTOOL_ERROR; >> + parser->nErr++; >> + *coll_id =3D COLL_NONE; >=20 > 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. Just in case, it shouldn=E2=80=99t be really used. I removed it. >=20 >> + return NULL; >> } >> - return coll; >> + return *coll_id =3D=3D 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 =3D=3D COLL_NONE && >> - sql_expr_coll(pParse, p, &is_found, &coll_id) && = is_found) >> + sql_expr_coll(pParse, p, &is_found, &coll_id) && = coll_id !=3D COLL_NONE) >> pTab->def->fields[i].coll_id =3D coll_id; >> + assert(pTab->def->fields[i].coll_id !=3D COLL_BINARY); >=20 > 4. Why? I can create a table with binary collation after the > previous commit. I guess it is debug remains. Removed. >=20 >> } >> } >> @@ -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 =3D false; >> + bool is_current_forced; >> + uint32_t prior_coll_id =3D COLL_NONE; >> + uint32_t current_coll_id; >> if (p->pPrior !=3D NULL) { >> - coll =3D multi_select_coll_seq_r(parser, p->pPrior, n, = is_found, >> - coll_id); >> - } else { >> - coll =3D NULL; >> - *coll_id =3D COLL_NONE; >> + prior_coll_id =3D multi_select_coll_seq(parser, = p->pPrior, n, >> + &is_prior_forced); >> } >> - assert(n >=3D 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. >=20 > 5. 'have gotten'? Maybe 'have got=E2=80=99? Yep, fixed.