[tarantool-patches] Re: [PATCH 2/2 v2] sql: allow <COLLATE> only for string-like args

[n.pettik]

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 898d16cf3..b850250e3 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 @a expr with the collation in the root
> + * can be used with <COLLATE>. If it is not, leave an error
> + * message in pParse.
> + *
> + * @param parse Parser context.
> + * @param expr Expression for checking.
> + *
> + * @retval 0 on success.
> + * @retval -1 on error.
> + */
> +static int
> +check_collate_arg(struct Parse *parse, struct Expr *expr)
> +{
> +	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;
> +	}
> +	if (type != FIELD_TYPE_STRING && type != FIELD_TYPE_SCALAR) {
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +			 "COLLATE can't be used with "
> +			 "non-string arguments”);

-> COLLATE clause ...

> +		parse->is_aborted = true;
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> int
> sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
> 	      struct coll **coll)
> @@ -4173,6 +4207,9 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
> 		}
> 	case TK_SPAN:
> 	case TK_COLLATE:{
> +			pExpr->pLeft->type = sql_expr_type(pExpr->pLeft);
> +			if (check_collate_arg(pParse, pExpr) != 0)
> +				break;
> 			return sqlExprCodeTarget(pParse, pExpr->pLeft,
> 						     target);
> 		}
> @@ -4396,6 +4433,8 @@ sqlExprCodeAtInit(Parse * pParse,	/* Parsing context */
> int
> sqlExprCodeTemp(Parse * pParse, Expr * pExpr, int *pReg)
> {
> +	if (pExpr->op == TK_COLLATE && check_collate_arg(pParse, pExpr) != 0)
> +		return 0;

Why this check is required (in addition to the same check in ExprCodeTarget)?

> 	int r2;
> 	pExpr = sqlExprSkipCollate(pExpr);
> 	if (ConstFactorOk(pParse)
> diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
> index f36540fc2..f614d0e86 100755
> --- a/test/sql-tap/collation.test.lua
> +++ b/test/sql-tap/collation.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
> -test:plan(177)
> +test:plan(191)
> 
> local prefix = "collation-"
> 
> @@ -546,4 +546,77 @@ test:do_execsql_test(
>     [[ SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE "unicode" ]],
>     {"b","B"})
> 
> +-- gh-3805 Check COLLATE passing with string-like args only.
> +
> +test:do_execsql_test(
> +    "collation-2.7.0",
> +    [[ CREATE TABLE test1 (one INT PRIMARY KEY, two INT) ]],
> +    {})
> +
> +test:do_catchsql_test(
> +    "collation-2.7.1",
> +    'SELECT one COLLATE BINARY FROM test1’,

BINARY is non-existent collation. Correct usage is “binary”.

> +test:do_catchsql_test(
> +    "collation-2.7.14",
> +    'SELECT +\'str\' COLLATE \"unicode\"',
> +    {0,{"str"}})

What about concatenation operation?

> diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
> index 1ca6a6446..1fdee16b7 100755
> --- a/test/sql-tap/in3.test.lua
> +++ b/test/sql-tap/in3.test.lua
> @@ -186,33 +186,6 @@ test:do_test(
>         -- </in3-1.13>
>     })
> 
> -
> -
> --- The first of these queries has to use the temp-table, because the 
> --- collation sequence used for the index on "t1.a" does not match the
> --- collation sequence used by the "IN" comparison. The second does not
> --- require a temp-table, because the collation sequences match.
> ---
> -test:do_test(
> -    "in3-1.14",
> -    function()
> -        return exec_neph(" SELECT a FROM t1 WHERE a COLLATE \"unicode_ci\" IN (SELECT a FROM t1) ")
> -    end, {
> -        -- <in3-1.14>
> -        1, 1, 3, 5
> -        -- </in3-1.14>
> -    })
> -
> -test:do_test(
> -    "in3-1.15",
> -    function()
> -        return exec_neph(" SELECT a FROM t1 WHERE a COLLATE \"binary\" IN (SELECT a FROM t1) ")
> -    end, {
> -        -- <in3-1.15>
> -        1, 1, 3, 5
> -        -- </in3-1.15>
> -    })

I’d better fix these tests rather than delete.