[tarantool-patches] Re: [PATCH 1/2] sql: add better collation determination in functions
n.pettik
korablev at tarantool.org
Thu Mar 28 15:50:19 MSK 2019
> 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.
> + 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']
...
>>> + }
>>> }
>>> + 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.
More information about the Tarantool-patches
mailing list