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 BA19E2ADDE for ; Thu, 28 Mar 2019 10:08:06 -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 EpuxNsFrzQci for ; Thu, 28 Mar 2019 10:08:06 -0400 (EDT) Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 48E452ADC8 for ; Thu, 28 Mar 2019 10:08:06 -0400 (EDT) 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 1/2] sql: add better collation determination in functions From: "i.koptelov" In-Reply-To: Date: Thu, 28 Mar 2019 17:08:03 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <0E6D4501-E63C-4A8F-B686-C947E07B6B7F@tarantool.org> 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: "n.pettik" > On 28 Mar 2019, at 15:50, n.pettik wrote: >=20 >>=20 >> 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) { >=20 > coll =3D=3D NULL is redundant check: before this branch this variable > is assigned only once with NULL value. Ok, removed. diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 4eaec8e42..444b262a3 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -4078,8 +4078,7 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, = int target) * is done using ANSI rules from * collations_check_compatibility(). */ - if ((pDef->funcFlags & SQL_FUNC_NEEDCOLL) !=3D 0 = && - coll =3D=3D NULL) { + if ((pDef->funcFlags & SQL_FUNC_NEEDCOLL) !=3D = 0) { struct coll *unused =3D NULL; uint32_t curr_id =3D COLL_NONE; bool is_curr_forced =3D false; >=20 >> + 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; >>>> + } >=20 > You don=E2=80=99t update is_curr_forced, and it allows to use invalid > collation mix: >=20 > tarantool> select min ('abc', 'asd' collate "binary", 'abc' COLLATE = "unicode") > --- > - - ['abc'] > ... >=20 Oh, sorry, I was too quick to send fixes. @@ -4107,6 +4106,9 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, = int target) pParse->is_aborted =3D = true; return 0; } + is_curr_forced =3D curr_id =3D=3D = next_id ? + is_next_forced = : + is_curr_forced; } coll =3D coll_by_id(curr_id)->coll; } >>>> + } >>>> } >>>> + 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); >=20 > Please add tests involving more than two params. > Example of illegal collation mix passing your tests > I pointed out above. > Oh, I meant =E2=80=98implicitly set=E2=80=99. Still can=E2=80=99t find = such tests. Added additional tests: iff --git a/test/sql-tap/func5.test.lua b/test/sql-tap/func5.test.lua index 4282fdac8..4120d5fb5 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(9) +test:plan(15) =20 --!./tcltestrunner.lua -- 2010 August 27 @@ -132,7 +132,7 @@ test:do_catchsql_test( test:do_catchsql_test( "func-5-3.3", [[ - SELECT min('a' COLLATE "unicode", 'A' COLLATE "unicode_ci"); + SELECT max ('abc', 'asd' COLLATE "binary", 'abc' COLLATE = "unicode") ]], { -- @@ -141,15 +141,91 @@ test:do_catchsql_test( } ) =20 -test:do_catchsql_test( +test:do_execsql_test( "func-5-3.4", + [[ + SELECT max (s1, 'asd' COLLATE "binary", s2) FROM test1 JOIN = test2; + ]], { + -- + "asd" + -- + } +) + +test:do_catchsql_test( + "func-5.3.5", + [[ + CREATE TABLE test3 (s3 VARCHAR(5) PRIMARY KEY COLLATE = "unicode"); + CREATE TABLE test4 (s4 VARCHAR(5) PRIMARY KEY COLLATE = "unicode"); + CREATE TABLE test5 (s5 VARCHAR(5) PRIMARY KEY COLLATE = "binary"); + INSERT INTO test3 VALUES ('a'); + INSERT INTO test4 VALUES ('a'); + INSERT INTO test5 VALUES ('a'); + SELECT max(s3, s4, s5) FROM test3 JOIN test4 JOIN test5; + ]], + { + -- + 1, "Illegal mix of collations" + -- + } +) + +test:do_catchsql_test( + "func-5-3.6", + [[ + SELECT min('a' COLLATE "unicode", 'A' COLLATE "unicode_ci"); + ]], + { + -- + 1, "Illegal mix of collations" + -- + } +) + +test:do_catchsql_test( + "func-5-3.7", [[ SELECT min(s1, s2) FROM test1 JOIN test2; ]], { - -- + -- 1, "Illegal mix of collations" - -- + -- + } +) + +test:do_catchsql_test( + "func-5-3.8", + [[ + SELECT min ('abc', 'asd' COLLATE "binary", 'abc' COLLATE = "unicode") + ]], + { + -- + 1, "Illegal mix of collations" + -- + } +) + +test:do_execsql_test( + "func-5-3.9", + [[ + SELECT min (s1, 'asd' COLLATE "binary", s2) FROM test1 JOIN = test2; + ]], { + -- + "a" + -- + } +) + +test:do_catchsql_test( + "func-5.3.10", + [[ + SELECT min(s3, s4, s5) FROM test3 JOIN test4 JOIN test5; + ]], + { + -- + 1, "Illegal mix of collations" + -- } )