[tarantool-patches] Re: [PATCH 2/2] sql: allow <COLLATE> only for string-like args
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Jun 2 20:09:39 MSK 2019
Thanks for the fixes! See 8 comments below.
>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>> index ba7eea59d..29e3386fa 100644
>>> --- a/src/box/sql/expr.c
>>> +++ b/src/box/sql/expr.c
>>> @@ -4215,6 +4215,22 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>>> }
>>> case TK_SPAN:
>>> case TK_COLLATE:{
>>> + enum field_type type;
>>> + struct Expr *left = pExpr->pLeft;
>>> + if (left->op == TK_COLUMN) {
>>> + int col_num = left->iColumn;
>>> + type = left->space_def->fields[col_num].type;
>>> + } else
>>> + type = left->type;
>>> + if (left->op != TK_CONCAT &&
>>
>> Why do you check for TK_CONCAT? Its type should be FIELD_TYPE_STRING.
> Concatenations are rejected without this check.
1. It is not a good answer, when you can't explain, why you need
a line of code, but a program does not work without it. Please, investigate
and explain, why do you check for both 'type == STRING' and 'op == CONCAT'.
If an op is CONCAT, then type is already STRING, so the additional check
for 'op' should not be necessary.
>
> commit 5feaa9330f2c642fc16b479383b1c5cac96c28cc
> Author: Roman Khabibov <roman.habibov at tarantool.org>
> Date: Mon May 6 14:30:21 2019 +0300
>
> sql: allow <COLLATE> only for string-like args
>
> Before this patch, user could use COLLATE with non-string-like
> literals, columns or subquery results. Disallow such usage.
>
> Closes #3804
>
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index ba7eea59d..e558cbb18 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -197,6 +197,40 @@ sqlExprSkipCollate(Expr * pExpr)
> return pExpr;
> }
>
> +/*
> + * Check that left node of @expr with the collation in the root
2. '@expr' is an invalid Doxygen command. If you want to reference
a parameter, use <@a parameter_name>. See Doxygen documentation.
> + * can be used with <COLLATE>. If it is not, leave an error
> + * message in pParse.
3. There is no argument 'pParse'.
> + *
> + * @param pParse Parser context.
> + * @param expr Expression for checking.
> + *
> + * @retval 0 on success.
> + * @retval -1 on error.
> + */
> +static int
> +check_collate_arg(Parse *parse, Expr *expr)
4. We use 'struct' prefix in all new SQL code.
> +{
> + struct Expr *left = expr->pLeft;
> + while (left->op == TK_COLLATE)
> + left = left->pLeft;
> + enum field_type type;
> + if (left->op == TK_COLUMN) {
> + int col_num = left->iColumn;
> + type = left->space_def->fields[col_num].type;
> + } else
> + type = left->type;
5. We always either use '{}' for both 'if-else' parts,
or do not use it at all.
> + if (left->op != TK_CONCAT && type != FIELD_TYPE_STRING &&
> + type != FIELD_TYPE_SCALAR) {
> + diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> + "COLLATE can't be used with "
> + "non-string arguments");
> + parse->is_aborted = true;
> + return -1;
> + }
> + return 0;
> @@ -3935,6 +3969,9 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
> testcase(op == TK_BITNOT);
> assert(TK_NOT == OP_Not);
> testcase(op == TK_NOT);
> + if (pExpr->pLeft->op == TK_COLLATE &&
> + check_collate_arg(pParse, pExpr->pLeft) != 0)
> + break;
> r1 = sqlExprCodeTemp(pParse, pExpr->pLeft,
> ®Free1);
6. Why do you check for COLLATE before CodeTemp()? This function
does exactly the same check.
> testcase(regFree1 == 0);
> diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
> index c4724e636..566f2e723 100755
> --- a/test/sql-tap/e_select1.test.lua
> +++ b/test/sql-tap/e_select1.test.lua
> @@ -1938,7 +1938,7 @@ test:do_execsql_test(
> test:do_execsql_test(
> "e_select-8.9.2",
> [[
> - SELECT x COLLATE "binary" FROM d4 ORDER BY 1 COLLATE "unicode_ci"
> + SELECT x COLLATE "unicode_ci" FROM d4 ORDER BY 1
7. Why? The old test should pass as well. '1' here is not a value, but an
index of a selected column.
> ]], {
> -- <e_select-8.9.2>
> "abc", "DEF", "ghi", "JKL"
> diff --git a/test/sql-tap/selectE.test.lua b/test/sql-tap/selectE.test.lua
> index 32c3d6a06..16075d38e 100755
> --- a/test/sql-tap/selectE.test.lua
> +++ b/test/sql-tap/selectE.test.lua
> @@ -57,8 +57,7 @@ test:do_test(
> CREATE TABLE t3(a TEXT primary key);
> INSERT INTO t3 VALUES('def'),('jkl');
>
> - SELECT a FROM t1 EXCEPT SELECT a FROM t2
> - ORDER BY a COLLATE "unicode_ci";
> + SELECT a FROM t1 EXCEPT SELECT a FROM t2 ORDER BY a COLLATE "unicode_ci";
> ]]
> end, {
> -- <selectE-1.0>
> @@ -70,8 +69,7 @@ test:do_test(
> "selectE-1.1",
> function()
> return test:execsql [[
> - SELECT a FROM t2 EXCEPT SELECT a FROM t3
> - ORDER BY a COLLATE "unicode_ci";
> + SELECT a FROM t2 EXCEPT SELECT a FROM t3 ORDER BY a COLLATE "unicode_ci";
> ]]
> end, {
> -- <selectE-1.1>
> @@ -83,8 +81,7 @@ test:do_test(
> "selectE-1.2",
> function()
> return test:execsql [[
> - SELECT a FROM t2 EXCEPT SELECT a FROM t3
> - ORDER BY a COLLATE "binary";
> + SELECT a FROM t2 EXCEPT SELECT a FROM t3 ORDER BY a COLLATE "binary";
> ]]
> end, {
> -- <selectE-1.2>
> @@ -96,8 +93,7 @@ test:do_test(
> "selectE-1.3",
> function()
> return test:execsql [[
> - SELECT a FROM t2 EXCEPT SELECT a FROM t3
> - ORDER BY a;
> + SELECT a FROM t2 EXCEPT SELECT a FROM t3 ORDER BY a;
> ]]
> end, {
> -- <selectE-1.3>
> @@ -113,8 +109,7 @@ test:do_test(
> DELETE FROM t3;
> INSERT INTO t2 VALUES('ABC'),('def'),('GHI'),('jkl');
> INSERT INTO t3 SELECT lower(a) FROM t2;
> - SELECT a COLLATE "unicode_ci" FROM t2 EXCEPT SELECT a FROM t3
> - ORDER BY 1
> + SELECT a COLLATE "unicode_ci" FROM t2 EXCEPT SELECT a FROM t3 ORDER BY 1
> ]]
> end, {
8. Why do you need changes in this file? You just made multiline requests
one line.
More information about the Tarantool-patches
mailing list