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 F0AD11F9B2 for ; Thu, 28 Mar 2019 08:50:22 -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 dJFkmK1jHDqP for ; Thu, 28 Mar 2019 08:50:22 -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 42E2E1F95F for ; Thu, 28 Mar 2019 08:50:21 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH 1/2] sql: add better collation determination in functions From: "n.pettik" In-Reply-To: <73268E75-2978-465E-8E82-1EEC0DCE92CA@tarantool.org> Date: Thu, 28 Mar 2019 15:50:19 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <6472870C-8952-43AC-9B86-8BB2E006502A@tarantool.org> <73268E75-2978-465E-8E82-1EEC0DCE92CA@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: Ivan Koptelov > Thank you for noticing, fixed now: > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 8c1889d8a..34abb9665 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -4102,65 +4102,34 @@ sqlExprCodeTarget(Parse * pParse, Expr * = pExpr, int target) > * is done using ANSI rules from > * collations_check_compatibility(). > */ > - if (nFarg =3D=3D 1) { > - bool unused; > - uint32_t id; > - if (sql_expr_coll(pParse, > - pFarg->a[0].pExpr, = &unused, > - &id, &coll) !=3D 0) > - return 0; > - } > if ((pDef->funcFlags & SQL_FUNC_NEEDCOLL) !=3D 0 = && > - coll =3D=3D NULL && nFarg > 1) { coll =3D=3D NULL is redundant check: before this branch this variable is assigned only once with NULL value. > + coll =3D=3D NULL) { > struct coll *unused =3D NULL; > uint32_t curr_id =3D COLL_NONE; > bool is_curr_forced =3D false; >=20 > - uint32_t temp_id =3D COLL_NONE; > - bool is_temp_forced =3D false; > - > - uint32_t lhs_id =3D COLL_NONE; > - bool is_lhs_forced =3D false; > + uint32_t next_id =3D COLL_NONE; > + bool is_next_forced =3D false; >=20 > - uint32_t rhs_id =3D COLL_NONE; > - bool is_rhs_forced =3D false; > + if (sql_expr_coll(pParse, = pFarg->a[0].pExpr, > + &is_curr_forced, = &curr_id, > + &unused) !=3D 0) > + return 0; >=20 > - for (int i =3D 0; i < nFarg; i++) { > + for (int j =3D 1; j < nFarg; j++) { > if (sql_expr_coll(pParse, > - = pFarg->a[i].pExpr, > - = &is_lhs_forced, > - &lhs_id, > + = pFarg->a[j].pExpr, > + = &is_next_forced, > + &next_id, > &unused) !=3D = 0) > return 0; >=20 > - for (int j =3D i + 1; j < nFarg; = j++) { > - if = (sql_expr_coll(pParse, > - = pFarg->a[j].pExpr, > - = &is_rhs_forced, > - = &rhs_id, > - = &unused) !=3D 0) > - return 0; > - > - if = (collations_check_compatibility( > - lhs_id, = is_lhs_forced, > - rhs_id, = is_rhs_forced, > - &temp_id) !=3D = 0) { > - = pParse->is_aborted =3D true; > - return 0; > - } > - > - is_temp_forced =3D = (temp_id =3D=3D > - = lhs_id) ? > - = is_lhs_forced : > - = is_rhs_forced; > - > - if = (collations_check_compatibility( > - curr_id, = is_curr_forced, > - temp_id, = is_temp_forced, > - &curr_id) !=3D = 0) { > - = pParse->is_aborted =3D true; > - return 0; > - } > + if = (collations_check_compatibility( > + curr_id, is_curr_forced, > + next_id, is_next_forced, > + &curr_id) !=3D 0) { > + pParse->is_aborted =3D = true; > + return 0; > } > } > coll =3D coll_by_id(curr_id)->coll; >>=20 >>> + >>> + if = (collations_check_compatibility( >>> + lhs_id, = is_lhs_forced, >>> + rhs_id, = is_rhs_forced, >>> + &temp_id) !=3D = 0) { >>> + pParse->rc =3D >>> + = SQL_TARANTOOL_ERROR; >>> + pParse->nErr++; >>> + return 0; >>> + } >>> + >>> + is_temp_forced =3D = (temp_id =3D=3D >>> + = lhs_id) ? >>> + = is_lhs_forced : >>> + = is_rhs_forced; >>> + >>> + if = (collations_check_compatibility( >>> + curr_id, = is_curr_forced, >>> + temp_id, = is_temp_forced, >>> + &curr_id) !=3D = 0) { >>> + pParse->rc =3D >>> + = SQL_TARANTOOL_ERROR; >>> + pParse->nErr++; >>> + return 0; >>> + } You don=E2=80=99t update is_curr_forced, and it allows to use invalid collation mix: tarantool> select min ('abc', 'asd' collate "binary", 'abc' COLLATE = "unicode") --- - - ['abc'] ... >>> + } >>> } >>> + coll =3D coll_by_id(curr_id)->coll; >>> } >>> if (pFarg) { >>> if (constMask) { >>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h >>> index 8967ea3e0..47ee474bb 100644 >>> --- a/src/box/sql/sqlInt.h >>> +++ b/src/box/sql/sqlInt.h >>> @@ -1660,7 +1660,13 @@ struct FuncDestructor { >>> #define SQL_FUNC_LIKE 0x0004 /* Candidate for the LIKE = optimization */ >>> #define SQL_FUNC_CASE 0x0008 /* Case-sensitive LIKE-type = function */ >>> #define SQL_FUNC_EPHEM 0x0010 /* Ephemeral. Delete with VDBE = */ >>> -#define SQL_FUNC_NEEDCOLL 0x0020 /* sqlGetFuncCollSeq() might be = called */ >>> +#define SQL_FUNC_NEEDCOLL 0x0020 /* sqlGetFuncCollSeq() might be = called. >>> + * The flag is set when the = collation >>> + * of function arguments should = be >>> + * determined, using rules in >>> + * = collations_check_compatibility() >>> + * function. >>> + */ >>> #define SQL_FUNC_LENGTH 0x0040 /* Built-in length() function */ >>> #define SQL_FUNC_TYPEOF 0x0080 /* Built-in typeof() function */ >>> #define SQL_FUNC_COUNT 0x0100 /* Built-in count(*) aggregate = */ >>> --=20 >>=20 >> Please, provide basic test cases involving one or more built-in >> functions and incompatible arguments (at least min-max funcs use it). >> Moreover, this flag can=E2=80=99t be set for user-defined functions, = which is pretty sad. > Add a few tests: > diff --git a/test/sql-tap/func5.test.lua b/test/sql-tap/func5.test.lua > index 6605a2ba1..4282fdac8 100755 > --- a/test/sql-tap/func5.test.lua > +++ b/test/sql-tap/func5.test.lua > @@ -1,6 +1,6 @@ > #!/usr/bin/env tarantool > test =3D require("sqltester") > -test:plan(5) > +test:plan(9) >=20 > --!./tcltestrunner.lua > -- 2010 August 27 > @@ -98,5 +98,59 @@ test:do_execsql_test( > -- > }) >=20 > +-- The following tests ensures that max() and min() functions > +-- raise error if argument's collations are incompatible. > + > +test:do_catchsql_test( > + "func-5-3.1", > + [[ > + SELECT max('a' COLLATE "unicode", 'A' COLLATE "unicode_ci"); > + ]], > + { > + -- > + 1, "Illegal mix of collations" > + -- > + } > +) > + > +test:do_catchsql_test( > + "func-5-3.2", > + [[ > + CREATE TABLE test1 (s1 VARCHAR(5) PRIMARY KEY COLLATE = "unicode"); > + CREATE TABLE test2 (s2 VARCHAR(5) PRIMARY KEY COLLATE = "unicode_ci"); > + INSERT INTO test1 VALUES ('a'); > + INSERT INTO test2 VALUES ('a'); > + SELECT max(s1, s2) FROM test1 JOIN test2; > + ]], > + { > + -- > + 1, "Illegal mix of collations" > + -- > + } > +) > + > +test:do_catchsql_test( > + "func-5-3.3", > + [[ > + SELECT min('a' COLLATE "unicode", 'A' COLLATE = "unicode_ci=E2=80=9D); Please add tests involving more than two params. Example of illegal collation mix passing your tests I pointed out above.