[tarantool-patches] Re: [PATCH 1/2] sql: add better collation determination in functions

["i.koptelov" <ivan.koptelov@tarantool.org>]

> On 28 Mar 2019, at 15:50, n.pettik <korablev@tarantool.org> wrote:
> 
>> 
>> 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 == 1) {
>> -				bool unused;
>> -				uint32_t id;
>> -				if (sql_expr_coll(pParse,
>> -						  pFarg->a[0].pExpr, &unused,
>> -						  &id, &coll) != 0)
>> -					return 0;
>> -			}
>> 			if ((pDef->funcFlags & SQL_FUNC_NEEDCOLL) != 0 &&
>> -			     coll == NULL && nFarg > 1) {
> 
> coll == 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) != 0 &&
-			     coll == NULL) {
+			if ((pDef->funcFlags & SQL_FUNC_NEEDCOLL) != 0) {
 				struct coll *unused = NULL;
 				uint32_t curr_id = COLL_NONE;
 				bool is_curr_forced = false;

> 
>> +			     coll == NULL) {
>> 				struct coll *unused = NULL;
>> 				uint32_t curr_id = COLL_NONE;
>> 				bool is_curr_forced = false;
>> 
>> -				uint32_t temp_id = COLL_NONE;
>> -				bool is_temp_forced = false;
>> -
>> -				uint32_t lhs_id = COLL_NONE;
>> -				bool is_lhs_forced = false;
>> +				uint32_t next_id = COLL_NONE;
>> +				bool is_next_forced = false;
>> 
>> -				uint32_t rhs_id = COLL_NONE;
>> -				bool is_rhs_forced = false;
>> +				if (sql_expr_coll(pParse, pFarg->a[0].pExpr,
>> +						  &is_curr_forced, &curr_id,
>> +						  &unused) != 0)
>> +					return 0;
>> 
>> -				for (int i = 0; i < nFarg; i++) {
>> +				for (int j = 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) != 0)
>> 						return 0;
>> 
>> -					for (int j = i + 1; j < nFarg; j++) {
>> -						if (sql_expr_coll(pParse,
>> -								  pFarg->a[j].pExpr,
>> -								  &is_rhs_forced,
>> -								  &rhs_id,
>> -								  &unused) != 0)
>> -							return 0;
>> -
>> -						if (collations_check_compatibility(
>> -							lhs_id, is_lhs_forced,
>> -							rhs_id, is_rhs_forced,
>> -							&temp_id) != 0) {
>> -							pParse->is_aborted = true;
>> -							return 0;
>> -						}
>> -
>> -						is_temp_forced = (temp_id ==
>> -								  lhs_id) ?
>> -								 is_lhs_forced :
>> -								 is_rhs_forced;
>> -
>> -						if (collations_check_compatibility(
>> -							curr_id, is_curr_forced,
>> -							temp_id, is_temp_forced,
>> -							&curr_id) != 0) {
>> -							pParse->is_aborted = true;
>> -							return 0;
>> -						}
>> +					if (collations_check_compatibility(
>> +						curr_id, is_curr_forced,
>> +						next_id, is_next_forced,
>> +						&curr_id) != 0) {
>> +						pParse->is_aborted = true;
>> +						return 0;
>> 					}
>> 				}
>> 				coll = coll_by_id(curr_id)->coll;
>>> 
>>>> +
>>>> +						if (collations_check_compatibility(
>>>> +							lhs_id, is_lhs_forced,
>>>> +							rhs_id, is_rhs_forced,
>>>> +							&temp_id) != 0) {
>>>> +							pParse->rc =
>>>> +								SQL_TARANTOOL_ERROR;
>>>> +							pParse->nErr++;
>>>> +							return 0;
>>>> +						}
>>>> +
>>>> +						is_temp_forced = (temp_id ==
>>>> +								  lhs_id) ?
>>>> +								 is_lhs_forced :
>>>> +								 is_rhs_forced;
>>>> +
>>>> +						if (collations_check_compatibility(
>>>> +							curr_id, is_curr_forced,
>>>> +							temp_id, is_temp_forced,
>>>> +							&curr_id) != 0) {
>>>> +							pParse->rc =
>>>> +								SQL_TARANTOOL_ERROR;
>>>> +							pParse->nErr++;
>>>> +							return 0;
>>>> +						}
> 
> You don’t update is_curr_forced, and it allows to use invalid
> collation mix:
> 
> tarantool> select min ('abc', 'asd' collate "binary", 'abc' COLLATE "unicode")
> ---
> - - ['abc']
> ...
> 
Oh, sorry, I was too quick to send fixes.

@@ -4107,6 +4106,9 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 						pParse->is_aborted = true;
 						return 0;
 					}
+					is_curr_forced = curr_id == next_id ?
+							 is_next_forced :
+							 is_curr_forced;
 				}
 				coll = coll_by_id(curr_id)->coll;
 			}

>>>> +					}
>>>> 				}
>>>> +				coll = 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 */
>>>> -- 
>>> 
>>> 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’t 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 = require("sqltester")
>> -test:plan(5)
>> +test:plan(9)
>> 
>> --!./tcltestrunner.lua
>> -- 2010 August 27
>> @@ -98,5 +98,59 @@ test:do_execsql_test(
>>        -- </func5-2.2>
>>    })
>> 
>> +-- 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");
>> +    ]],
>> +    {
>> +        -- <func5-3.1>
>> +        1, "Illegal mix of collations"
>> +        -- </func5-3.1>
>> +    }
>> +)
>> +
>> +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;
>> +    ]],
>> +    {
>> +        -- <func5-3.2>
>> +        1, "Illegal mix of collations"
>> +        -- </func5-3.2>
>> +    }
>> +)
>> +
>> +test:do_catchsql_test(
>> +    "func-5-3.3",
>> +    [[
>> +        SELECT min('a' COLLATE "unicode", 'A' COLLATE "unicode_ci”);
> 
> Please add tests involving more than two params.
> Example of illegal collation mix passing your tests
> I pointed out above.

> Oh, I meant ‘implicitly set’. Still can’t 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 = require("sqltester")
-test:plan(9)
+test:plan(15)
 
 --!./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")
     ]],
     {
         -- <func5-3.3>
@@ -141,15 +141,91 @@ test:do_catchsql_test(
     }
 )
 
-test:do_catchsql_test(
+test:do_execsql_test(
     "func-5-3.4",
+    [[
+        SELECT max (s1, 'asd' COLLATE "binary", s2) FROM test1 JOIN test2;
+    ]], {
+        -- <func5-3.4>
+        "asd"
+        -- </func5-3.4>
+    }
+)
+
+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;
+    ]],
+    {
+        -- <func5-3.5>
+        1, "Illegal mix of collations"
+        -- </func5-3.5>
+    }
+)
+
+test:do_catchsql_test(
+    "func-5-3.6",
+    [[
+        SELECT min('a' COLLATE "unicode", 'A' COLLATE "unicode_ci");
+    ]],
+    {
+        -- <func5-3.6>
+        1, "Illegal mix of collations"
+        -- </func5-3.6>
+    }
+)
+
+test:do_catchsql_test(
+    "func-5-3.7",
     [[
         SELECT min(s1, s2) FROM test1 JOIN test2;
     ]],
     {
-        -- <func5-3.4>
+        -- <func5-3.7>
         1, "Illegal mix of collations"
-        -- </func5-3.4>
+        -- </func5-3.7>
+    }
+)
+
+test:do_catchsql_test(
+    "func-5-3.8",
+    [[
+        SELECT min ('abc', 'asd' COLLATE "binary", 'abc' COLLATE "unicode")
+    ]],
+    {
+        -- <func5-3.8>
+        1, "Illegal mix of collations"
+        -- </func5-3.8>
+    }
+)
+
+test:do_execsql_test(
+    "func-5-3.9",
+    [[
+        SELECT min (s1, 'asd' COLLATE "binary", s2) FROM test1 JOIN test2;
+    ]], {
+        -- <func5-3.9>
+        "a"
+        -- </func5-3.9>
+    }
+)
+
+test:do_catchsql_test(
+    "func-5.3.10",
+    [[
+        SELECT min(s3, s4, s5) FROM test3 JOIN test4 JOIN test5;
+    ]],
+    {
+        -- <func5-3.10>
+        1, "Illegal mix of collations"
+        -- <func5-3.10>
     }
 )