[tarantool-patches] Re: [PATCH 1/2 v2] test: check that collations isn't ignored in SELECTs

[tarantool-patches] Re: [PATCH 1/2 v2] test: check that collations isn't ignored in SELECTs

[n.pettik]

> On 24 Jun 2019, at 13:54, Roman Khabibov <roman.habibov@tarantool.org> wrote:
> 
> Add test to check that a new collation isn't ignored regardless
> of a name of a previous one in the following patterns of quries:

Nit: quries -> queries

> 
> SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE “unicode_ci"

Why do you consider this kind of query? What can be wrong with it?
Specifying collation for result set members doesn’t make much sense btw.

> 
> Also note: It is disallowed to compare strings with different
> collations: ISO/IEC JTC 1/SC 32, Part 2: Foundation, page 531
> ---
> src/box/sql/resolve.c           |  7 +++++++
> test/sql-tap/collation.test.lua | 19 ++++++++++++++++++-
> 2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index fdf3703da..348b3ea9a 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -109,6 +109,13 @@ resolveAlias(Parse * pParse,	/* Parsing context */
> 		return;
> 	if (zType[0] != 'G')
> 		incrAggFunctionDepth(pDup, nSubquery);
> +	/*
> +	 * If there was typed more than one explicit collations in
> +	 * query, it will be a sequence of left nodes with the
> +	 * collations in a tree. There is nothing special about
> +	 * keeping the sequence. Only one collation could be
> +	 * stored, but the present solution is simpler.
> +	 */

Do not understand how mentioned example is related to this code.
I suppose you might mean example like:

SELECT s COLLATE “unicode” COLLATE “binary” COLLATE “unicode_ci” …

Is this syntax allowed by ANSI? If so, which one (first or last) collation must be used?

> 	if (pExpr->op == TK_COLLATE) {
> 		pDup =
> 		    sqlExprAddCollateString(pParse, pDup, pExpr->u.zToken);
> diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
> index 79547361c..f36540fc2 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(174)
> +test:plan(177)
> 
> local prefix = "collation-"
> 
> @@ -529,4 +529,21 @@ test:do_catchsql_test(
>         'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))',
>         {1, "Collation 'FOO' does not exist"})
> 
> +-- gh-3805 Check that collation is not ignored.

Why these test cases are related to the mentioned ticket?
Which collation might be ignored? I see two collations in
two different parts of query. First one is used during sorting
routines, second one is simply ignored (since values of
resulting set are not supposed to be compared with anything else).

> +
> +test:do_execsql_test(
> +    "collation-2.6.0",
> +    [[ CREATE TABLE a (id INT PRIMARY KEY, s TEXT) ]],
> +    {})
> +
> +test:do_execsql_test(
> +    "collation-2.6.1",
> +    [[ INSERT INTO a VALUES (1, 'B'), (2, 'b') ]],
> +    {})
> +
> +test:do_execsql_test(
> +    "collation-2.6.2",
> +    [[ SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE "unicode" ]],
> +    {"b","B”})
> +
> test:finish_test()
> -- 
> 2.20.1 (Apple Git-117)
> 

[tarantool-patches] Re: [PATCH 1/2 v2] test: check that collations isn't ignored in SELECTs

[Vladislav Shpilevoy]

On 25/06/2019 19:30, n.pettik wrote:
> 
> 
>> On 24 Jun 2019, at 13:54, Roman Khabibov <roman.habibov@tarantool.org> wrote:
>>
>> Add test to check that a new collation isn't ignored regardless
>> of a name of a previous one in the following patterns of quries:
> 
> Nit: quries -> queries
> 
>>
>> SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE “unicode_ci"
> 
> Why do you consider this kind of query? What can be wrong with it?

That example works, but when the collations are different,
according to the standard the last one should be applied, as
I remember when I last time checked. Before the patch a wrong
collation was used when there are several of them.

If under `this kind of query` you mean why 'order by'? - it is
just the only request where we can reproduce that. I've tried
to find a simpler test, but did not manage.

> Specifying collation for result set members doesn’t make much sense btw.
> 
>>
>> Also note: It is disallowed to compare strings with different
>> collations: ISO/IEC JTC 1/SC 32, Part 2: Foundation, page 531
>> ---
>> src/box/sql/resolve.c           |  7 +++++++
>> test/sql-tap/collation.test.lua | 19 ++++++++++++++++++-
>> 2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>> index fdf3703da..348b3ea9a 100644
>> --- a/src/box/sql/resolve.c
>> +++ b/src/box/sql/resolve.c
>> @@ -109,6 +109,13 @@ resolveAlias(Parse * pParse,	/* Parsing context */
>> 		return;
>> 	if (zType[0] != 'G')
>> 		incrAggFunctionDepth(pDup, nSubquery);
>> +	/*
>> +	 * If there was typed more than one explicit collations in
>> +	 * query, it will be a sequence of left nodes with the
>> +	 * collations in a tree. There is nothing special about
>> +	 * keeping the sequence. Only one collation could be
>> +	 * stored, but the present solution is simpler.
>> +	 */
> 
> Do not understand how mentioned example is related to this code.
> I suppose you might mean example like:
> 
> SELECT s COLLATE “unicode” COLLATE “binary” COLLATE “unicode_ci” …
> 
> Is this syntax allowed by ANSI? If so, which one (first or last) collation must be used?

Yes, allowed, the last collation should be used. Probably, Roman,
you should include in the commit message a reference to the
standard (probably you did, I don't know - the first message in
that thread is lost in my copy of our mailing list).

> 
>> 	if (pExpr->op == TK_COLLATE) {
>> 		pDup =
>> 		    sqlExprAddCollateString(pParse, pDup, pExpr->u.zToken);

[tarantool-patches] Re: [PATCH 1/2 v2] test: check that collations isn't ignored in SELECTs

[Roman Khabibov]

Hi! Thanks for the review.

> On Jun 25, 2019, at 8:30 PM, n.pettik <korablev@tarantool.org> wrote:
> 
> 
> 
>> On 24 Jun 2019, at 13:54, Roman Khabibov <roman.habibov@tarantool.org> wrote:
>> 
>> Add test to check that a new collation isn't ignored regardless
>> of a name of a previous one in the following patterns of quries:
> 
> Nit: quries -> queries
Fixed.

>> 
>> SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE “unicode_ci"
> 
> Why do you consider this kind of query? What can be wrong with it?
> Specifying collation for result set members doesn’t make much sense btw.
I consider it, because it didn’t work after my addition of checking from the
second patch:
+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 = sql_expr_type(left);
+	if (type != FIELD_TYPE_STRING && type != FIELD_TYPE_SCALAR) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+			 "COLLATE clause can't be used with non-string "
+			 "arguments");
+		parse->is_aborted = true;
+		return -1;
+	}
+	return 0;
+}
The tree of this query included sequence of collations on left nodes. This is
the result of optimizations in multiSelectOrderBy. Now I can not exactly say how
It occurs. At first time, I didn’t use loop to descend throughout left nodes.
I added this loop, because of that reason.

>> Also note: It is disallowed to compare strings with different
>> collations: ISO/IEC JTC 1/SC 32, Part 2: Foundation, page 531
>> ---
>> src/box/sql/resolve.c           |  7 +++++++
>> test/sql-tap/collation.test.lua | 19 ++++++++++++++++++-
>> 2 files changed, 25 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>> index fdf3703da..348b3ea9a 100644
>> --- a/src/box/sql/resolve.c
>> +++ b/src/box/sql/resolve.c
>> @@ -109,6 +109,13 @@ resolveAlias(Parse * pParse,	/* Parsing context */
>> 		return;
>> 	if (zType[0] != 'G')
>> 		incrAggFunctionDepth(pDup, nSubquery);
>> +	/*
>> +	 * If there was typed more than one explicit collations in
>> +	 * query, it will be a sequence of left nodes with the
>> +	 * collations in a tree. There is nothing special about
>> +	 * keeping the sequence. Only one collation could be
>> +	 * stored, but the present solution is simpler.
>> +	 */
> 
> Do not understand how mentioned example is related to this code.
> I suppose you might mean example like:
> 
> SELECT s COLLATE “unicode” COLLATE “binary” COLLATE “unicode_ci” …
> 
> Is this syntax allowed by ANSI? If so, which one (first or last) collation must be used?
> 
>> 	if (pExpr->op == TK_COLLATE) {
>> 		pDup =
>> 		    sqlExprAddCollateString(pParse, pDup, pExpr->u.zToken);
>> diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
>> index 79547361c..f36540fc2 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(174)
>> +test:plan(177)
>> 
>> local prefix = "collation-"
>> 
>> @@ -529,4 +529,21 @@ test:do_catchsql_test(
>>        'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))',
>>        {1, "Collation 'FOO' does not exist"})
>> 
>> +-- gh-3805 Check that collation is not ignored.
> 
> Why these test cases are related to the mentioned ticket?
> Which collation might be ignored? I see two collations in
> two different parts of query. First one is used during sorting
> routines, second one is simply ignored (since values of
> resulting set are not supposed to be compared with anything else).
See above.

>> +
>> +test:do_execsql_test(
>> +    "collation-2.6.0",
>> +    [[ CREATE TABLE a (id INT PRIMARY KEY, s TEXT) ]],
>> +    {})
>> +
>> +test:do_execsql_test(
>> +    "collation-2.6.1",
>> +    [[ INSERT INTO a VALUES (1, 'B'), (2, 'b') ]],
>> +    {})
>> +
>> +test:do_execsql_test(
>> +    "collation-2.6.2",
>> +    [[ SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE "unicode" ]],
>> +    {"b","B”})
>> +
>> test:finish_test()
>> -- 
>> 2.20.1 (Apple Git-117)
>> 
> 

[tarantool-patches] Re: [PATCH 1/2 v2] test: check that collations isn't ignored in SELECTs

[Roman Khabibov]

Forgot.

commit 40690ab014e80d954c0139506ed41b0aa18b95d5
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Mon May 6 14:45:51 2019 +0300

    test: check that collations isn't ignored in SELECTs
    
    Add test to check that a new collation isn't ignored regardless
    of a name of a previous one in the following patterns of queries:
    
    SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE "unicode_ci"
    
    Also note: It is disallowed to compare strings with different
    collations: ISO/IEC JTC 1/SC 32, Part 2: Foundation, page 531

diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index fdf3703da..348b3ea9a 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -109,6 +109,13 @@ resolveAlias(Parse * pParse,	/* Parsing context */
 		return;
 	if (zType[0] != 'G')
 		incrAggFunctionDepth(pDup, nSubquery);
+	/*
+	 * If there was typed more than one explicit collations in
+	 * query, it will be a sequence of left nodes with the
+	 * collations in a tree. There is nothing special about
+	 * keeping the sequence. Only one collation could be
+	 * stored, but the present solution is simpler.
+	 */
 	if (pExpr->op == TK_COLLATE) {
 		pDup =
 		    sqlExprAddCollateString(pParse, pDup, pExpr->u.zToken);
diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
index 79547361c..f36540fc2 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(174)
+test:plan(177)
 
 local prefix = "collation-"
 
@@ -529,4 +529,21 @@ test:do_catchsql_test(
         'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))',
         {1, "Collation 'FOO' does not exist"})
 
+-- gh-3805 Check that collation is not ignored.
+
+test:do_execsql_test(
+    "collation-2.6.0",
+    [[ CREATE TABLE a (id INT PRIMARY KEY, s TEXT) ]],
+    {})
+
+test:do_execsql_test(
+    "collation-2.6.1",
+    [[ INSERT INTO a VALUES (1, 'B'), (2, 'b') ]],
+    {})
+
+test:do_execsql_test(
+    "collation-2.6.2",
+    [[ SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE "unicode" ]],
+    {"b","B"})
+
 test:finish_test()

[tarantool-patches] Re: [PATCH 1/2 v2] test: check that collations isn't ignored in SELECTs

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 4730 bytes --]



> On 28 Jun 2019, at 16:11, Roman Khabibov <roman.habibov@tarantool.org> wrote:
> 
> Hi! Thanks for the review.
> 
>> On Jun 25, 2019, at 8:30 PM, n.pettik <korablev@tarantool.org <mailto:korablev@tarantool.org>> wrote:
>>> On 24 Jun 2019, at 13:54, Roman Khabibov <roman.habibov@tarantool.org <mailto:roman.habibov@tarantool.org>> wrote:
>>> 
>>> Add test to check that a new collation isn't ignored regardless
>>> of a name of a previous one in the following patterns of quries:
>> 
>> Nit: quries -> queries
> Fixed.
> 
>>> 
>>> SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE “unicode_ci"
>> 
>> Why do you consider this kind of query? What can be wrong with it?
>> Specifying collation for result set members doesn’t make much sense btw.
> I consider it, because it didn’t work after my addition of checking from the
> second patch:
> +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 = sql_expr_type(left);
> +	if (type != FIELD_TYPE_STRING && type != FIELD_TYPE_SCALAR) {
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +			 "COLLATE clause can't be used with non-string "
> +			 "arguments");
> +		parse->is_aborted = true;
> +		return -1;
> +	}
> +	return 0;
> +}
> The tree of this query included sequence of collations on left nodes. This is
> the result of optimizations in multiSelectOrderBy. Now I can not exactly say how
> It occurs. At first time, I didn’t use loop to descend throughout left nodes.
> I added this loop, because of that reason.

Wait, in the example like this:

 SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE “unicode_ci"

collate clauses refer to independent parts of query and
can’t contradict to each other. As I already said:

"First one is used during sorting
routines, second one is simply ignored (since values of
resulting set are not supposed to be compared with anything else)."

Why then are they organised into list? Please, investigate reasons
of this “optimisation”.

>>> Also note: It is disallowed to compare strings with different
>>> collations: ISO/IEC JTC 1/SC 32, Part 2: Foundation, page 531
>>> ---
>>> src/box/sql/resolve.c           |  7 +++++++
>>> test/sql-tap/collation.test.lua | 19 ++++++++++++++++++-
>>> 2 files changed, 25 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>>> index fdf3703da..348b3ea9a 100644
>>> --- a/src/box/sql/resolve.c
>>> +++ b/src/box/sql/resolve.c
>>> @@ -109,6 +109,13 @@ resolveAlias(Parse * pParse,	/* Parsing context */
>>> 		return;
>>> 	if (zType[0] != 'G')
>>> 		incrAggFunctionDepth(pDup, nSubquery);
>>> +	/*
>>> +	 * If there was typed more than one explicit collations in
>>> +	 * query, it will be a sequence of left nodes with the
>>> +	 * collations in a tree. There is nothing special about
>>> +	 * keeping the sequence. Only one collation could be
>>> +	 * stored, but the present solution is simpler.
>>> +	 */
>> 
>> Do not understand how mentioned example is related to this code.
>> I suppose you might mean example like:
>> 
>> SELECT s COLLATE “unicode” COLLATE “binary” COLLATE “unicode_ci” …
>> 
>> Is this syntax allowed by ANSI? If so, which one (first or last) collation must be used?
>> 
>>> 	if (pExpr->op == TK_COLLATE) {
>>> 		pDup =
>>> 		    sqlExprAddCollateString(pParse, pDup, pExpr->u.zToken);
>>> diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
>>> index 79547361c..f36540fc2 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(174)
>>> +test:plan(177)
>>> 
>>> local prefix = "collation-"
>>> 
>>> @@ -529,4 +529,21 @@ test:do_catchsql_test(
>>>       'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))',
>>>       {1, "Collation 'FOO' does not exist"})
>>> 
>>> +-- gh-3805 Check that collation is not ignored.
>> 
>> Why these test cases are related to the mentioned ticket?
>> Which collation might be ignored? I see two collations in
>> two different parts of query. First one is used during sorting
>> routines, second one is simply ignored (since values of
>> resulting set are not supposed to be compared with anything else).
> See above.

See above what?:) Please drop this commit or provide
decent explanation. Now I can’t understand how provided
test is related to the ticket and what’s wrong with this case
at all.


[-- Attachment #2: Type: text/html, Size: 39478 bytes --]

[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.

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

[Roman Khabibov]

Hi! Thanks for the review.

> On Jun 25, 2019, at 8:30 PM, n.pettik <korablev@tarantool.org> wrote:
> 
> 
>> 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 …
+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 = sql_expr_type(left);
+	if (type != FIELD_TYPE_STRING && type != FIELD_TYPE_SCALAR) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+			 "COLLATE clause can't be used with non-string "
+			 "arguments");
+		parse->is_aborted = true;
+		return -1;
+	}
+	return 0;
+}
+

>> +		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)?
It is needed e.g. for the following queries: “SELECT NOT TRUE COLLATE “unicode””.
“NOT” will be on the root. The sequence of calls: sqlExprCodeTagret -> (case TK_NOT)
-> sqlExprCodeTemp.
			NOT
		       /   \
		   unicode 0x0
		  /      \
		TRUE	 0x0
>> 	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”.
@@ -546,4 +546,87 @@ 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',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.2",
+    'SELECT one COLLATE "unicode_ci" FROM test1',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.3",
+    'SELECT two COLLATE "binary" FROM test1',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+
+test:do_catchsql_test(
+    "collation-2.7.4",
+    'SELECT (one + two) COLLATE "binary" FROM test1',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.5",
+    'SELECT (SELECT one FROM test1) COLLATE "binary"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_execsql_test(
+    "collation-2.7.6",
+    'SELECT TRIM(\'A\') COLLATE "binary"',
+    {"A"})
+
+test:do_catchsql_test(
+    "collation-2.7.7",
+    'SELECT RANDOM() COLLATE "binary"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.8",
+    'SELECT LENGTH(\'A\') COLLATE "binary"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+

>> +test:do_catchsql_test(
>> +    "collation-2.7.14",
>> +    'SELECT +\'str\' COLLATE \"unicode\"',
>> +    {0,{"str"}})
> 
> What about concatenation operation?
+
+test:do_execsql_test(
+    "collation-2.7.15",
+    'SELECT (\'con\'||\'cat\') COLLATE "unicode"',
+    {"concat"})
+

>> 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.

Previous test in this file will be same if I remove COLLATE clauses.
test:do_test(
    "in3-1.3",
    function()
        return exec_neph(" SELECT a FROM t1 WHERE a IN (SELECT a FROM t1); ")
    end, {
        -- <in3-1.3>
        0, 1, 3, 5
        -- </in3-1.3>
    })

commit b35caf9a05e5a4a01566756e8dda6b9621b9348b
Author: Roman Khabibov <roman.habibov@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 898d16cf3..e1aae18fc 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -197,6 +197,34 @@ 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 = sql_expr_type(left);
+	if (type != FIELD_TYPE_STRING && type != FIELD_TYPE_SCALAR) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+			 "COLLATE clause can't be used with non-string "
+			 "arguments");
+		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 +4201,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 +4427,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;
 	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..99314ec04 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(193)
 
 local prefix = "collation-"
 
@@ -546,4 +546,87 @@ 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',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.2",
+    'SELECT one COLLATE "unicode_ci" FROM test1',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.3",
+    'SELECT two COLLATE "binary" FROM test1',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+
+test:do_catchsql_test(
+    "collation-2.7.4",
+    'SELECT (one + two) COLLATE "binary" FROM test1',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.5",
+    'SELECT (SELECT one FROM test1) COLLATE "binary"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_execsql_test(
+    "collation-2.7.6",
+    'SELECT TRIM(\'A\') COLLATE "binary"',
+    {"A"})
+
+test:do_catchsql_test(
+    "collation-2.7.7",
+    'SELECT RANDOM() COLLATE "binary"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.8",
+    'SELECT LENGTH(\'A\') COLLATE "binary"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.9",
+    'SELECT TRUE COLLATE "unicode"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.10",
+    'SELECT NOT TRUE COLLATE "unicode"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.11",
+    'SELECT TRUE AND TRUE COLLATE "unicode"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.12",
+    'SELECT 1 | 1 COLLATE "unicode"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_execsql_test(
+    "collation-2.7.14",
+    'SELECT +\'str\' COLLATE "unicode"',
+    {"str"})
+
+test:do_execsql_test(
+    "collation-2.7.15",
+    'SELECT (\'con\'||\'cat\') COLLATE "unicode"',
+    {"concat"})
+
+test:do_execsql_test(
+    "collation-2.7.16",
+    'SELECT (SELECT \'str\') COLLATE "binary" COLLATE "binary";',
+    {"str"})
+
 test:finish_test()
diff --git a/test/sql-tap/distinct.test.lua b/test/sql-tap/distinct.test.lua
index e6970084e..ae35a0db5 100755
--- a/test/sql-tap/distinct.test.lua
+++ b/test/sql-tap/distinct.test.lua
@@ -121,12 +121,12 @@ local data = {
     {"12.1", 0, "SELECT DISTINCT a, d FROM t1"},
     {"12.2", 0, "SELECT DISTINCT a, d FROM t4"},
     {"13.1", 0, "SELECT DISTINCT a, b, c COLLATE \"unicode_ci\" FROM t1"},
-    {"13.2", 0, "SELECT DISTINCT a, b, c COLLATE \"unicode_ci\" FROM t4"},
+    {"13.2", 1, "SELECT DISTINCT a, b, c FROM t4"},
     {"14.1", 0, "SELECT DISTINCT a, d COLLATE \"unicode_ci\" FROM t1"},
     {"14.2", 1, "SELECT DISTINCT a, d COLLATE \"unicode_ci\" FROM t4"},
     {"15 ", 0, "SELECT DISTINCT a, d COLLATE \"binary\" FROM t1"},
     {"16.1", 0, "SELECT DISTINCT a, b, c COLLATE \"binary\" FROM t1"},
-    {"16.2", 0, "SELECT DISTINCT a, b, c COLLATE \"binary\" FROM t4"},
+    {"16.2", 1, "SELECT DISTINCT a, b, c FROM t4"},
     {"17",  0,   --{ \/* Technically, it would be possible to detect that DISTINCT\n            ** is a no-op in cases like the following. But sql does not\n            ** do so. *\/\n
     "SELECT DISTINCT t1.id FROM t1, t2 WHERE t1.id=t2.x" },
     {"18 ", 1, "SELECT DISTINCT c1, c2 FROM t3"},
@@ -173,7 +173,7 @@ data = {
     {"a, b, c FROM t1", {"btree"}, {"A", "B", "C", "a", "b", "c"}},
     {"a, b, c FROM t1 ORDER BY a, b, c", {"btree"}, {"A", "B", "C", "a", "b", "c"}},
     {"b FROM t1 WHERE a = 'a'", {}, {"b"}},
-    {"b FROM t1 ORDER BY +b COLLATE \"binary\"", {"btree", "btree"}, {"B", "b"}},
+    {"b FROM t1 ORDER BY b COLLATE \"binary\"", {"btree", "btree"}, {"B", "b"}},
     {"a FROM t1", {}, {"A", "a"}},
     {"b COLLATE \"unicode_ci\" FROM t1", {}, {"b"}},
     {"b COLLATE \"unicode_ci\" FROM t1 ORDER BY b COLLATE \"unicode_ci\"", {}, {"b"}},
diff --git a/test/sql-tap/identifier_case.test.lua b/test/sql-tap/identifier_case.test.lua
index 4db729f11..65ed9aea1 100755
--- a/test/sql-tap/identifier_case.test.lua
+++ b/test/sql-tap/identifier_case.test.lua
@@ -166,7 +166,7 @@ test:do_execsql_test(
 
 test:do_execsql_test(
     test_prefix.."4.1",
-    string.format([[select * from table1 order by a collate "unicode_ci"]]),
+    string.format([[select * from table1 order by a]]),
     {}
 )
 
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
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(27)
+test:plan(25)
 
 --!./tcltestrunner.lua
 -- 2007 November 29
@@ -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>
-    })
-
 -- Neither of these queries require a temp-table. The collation sequence
 -- makes no difference when using a rowid.
 --
diff --git a/test/sql-tap/resolver01.test.lua b/test/sql-tap/resolver01.test.lua
index 9fcf0c7c0..315c892ac 100755
--- a/test/sql-tap/resolver01.test.lua
+++ b/test/sql-tap/resolver01.test.lua
@@ -106,7 +106,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.1",
     [[
-        SELECT 2 AS y FROM t1, t2 ORDER BY y COLLATE "unicode_ci";
+        SELECT 2 AS y FROM t1, t2 ORDER BY y;
     ]], {
         -- <resolver01-2.1>
         0, {2}
@@ -116,7 +116,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.2",
     [[
-        SELECT 2 AS yy FROM t1, t2 ORDER BY y COLLATE "unicode_ci";
+        SELECT 2 AS yy FROM t1, t2 ORDER BY y;
     ]], {
         -- <resolver01-2.2>
         1, "ambiguous column name: Y"
@@ -126,7 +126,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.3",
     [[
-        SELECT x AS y FROM t3 ORDER BY y COLLATE "unicode_ci";
+        SELECT x AS y FROM t3 ORDER BY y;
     ]], {
         -- <resolver01-2.3>
         0, {11, 33}
@@ -136,7 +136,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.4",
     [[
-        SELECT x AS yy FROM t3 ORDER BY y COLLATE "unicode_ci";
+        SELECT x AS yy FROM t3 ORDER BY y;
     ]], {
         -- <resolver01-2.4>
         0, {33, 11}
@@ -146,7 +146,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.5",
     [[
-        SELECT x AS yy FROM t3 ORDER BY yy COLLATE "unicode_ci";
+        SELECT x AS yy FROM t3 ORDER BY yy;
     ]], {
         -- <resolver01-2.5>
         0, {11, 33}
@@ -156,7 +156,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.6",
     [[
-        SELECT x AS yy FROM t3 ORDER BY 1 COLLATE "unicode_ci";
+        SELECT x AS yy FROM t3 ORDER BY 1;
     ]], {
         -- <resolver01-2.6>
         0, {11, 33}
diff --git a/test/sql-tap/select1.test.lua b/test/sql-tap/select1.test.lua
index 6beeb34cb..6811f7dcb 100755
--- a/test/sql-tap/select1.test.lua
+++ b/test/sql-tap/select1.test.lua
@@ -1672,7 +1672,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "select1-10.7",
     [[
-        SELECT f1 COLLATE "unicode_ci" AS x FROM test1 ORDER BY x
+        SELECT f1 AS x FROM test1 ORDER BY x
     ]], {
         -- <select1-10.7>
         11, 33
diff --git a/test/sql-tap/tkt-b75a9ca6b0.test.lua b/test/sql-tap/tkt-b75a9ca6b0.test.lua
index 89817d2af..dde6ab502 100755
--- a/test/sql-tap/tkt-b75a9ca6b0.test.lua
+++ b/test/sql-tap/tkt-b75a9ca6b0.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(22)
+test:plan(20)
 
 --!./tcltestrunner.lua
 -- 2014-04-21
@@ -57,7 +57,6 @@ local eqps = {
     {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x, y DESC", {1, 3, 2, 2, 3, 1}, {idxscan, sort}},
     {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x DESC, y DESC", {3, 1, 2, 2, 1, 3}, {idxscan, sort}},
     {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x ASC, y ASC", {1, 3, 2, 2, 3, 1}, {idxscan}},
-    {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x COLLATE \"unicode_ci\", y", {1, 3, 2, 2, 3, 1}, {idxscan, sort}},
 }
 for tn, val in ipairs(eqps) do
     local q = val[1]
diff --git a/test/sql-tap/with1.test.lua b/test/sql-tap/with1.test.lua
index ec45e5e76..6985c589e 100755
--- a/test/sql-tap/with1.test.lua
+++ b/test/sql-tap/with1.test.lua
@@ -1043,7 +1043,7 @@ test:do_catchsql_test(13.3, [[
 -- 2015-04-12
 --
 test:do_execsql_test(14.1, [[
-  WITH x AS (SELECT * FROM t) SELECT 0 EXCEPT SELECT 0 ORDER BY 1 COLLATE "binary";
+  WITH x AS (SELECT * FROM t) SELECT 0 EXCEPT SELECT 0 ORDER BY 1;
 ]], {
   -- <14.1>
   

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

[n.pettik]

>>> @@ -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)?
> It is needed e.g. for the following queries: “SELECT NOT TRUE COLLATE “unicode””.
> “NOT” will be on the root. The sequence of calls: sqlExprCodeTagret -> (case TK_NOT)
> -> sqlExprCodeTemp.
> 			NOT
> 		       /   \
> 		   unicode 0x0
> 		  /      \
> 		TRUE	 0x0
>>> 	int r2;
>>> 	pExpr = sqlExprSkipCollate(pExpr);

In fact, problem is here: this function skips collate token.
So, diff is following:

@@ -4427,10 +4426,7 @@ 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;
        int r2;
-       pExpr = sqlExprSkipCollate(pExpr);
        if (ConstFactorOk(pParse)
            && pExpr->op != TK_REGISTER && sqlExprIsConstantNotJoin(pExpr)
            ) {

What is more, you don’t need to set type of expression
in sqlExprCodeTarget():

@@ -4201,7 +4201,6 @@ 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,

>>> 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.
> 
> Previous test in this file will be same if I remove COLLATE clauses.

You don’t have to remove collate clause, you should create new
table with string field type and run this test using that table.
Comment above the test clearly indicates its purpose.

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

[Roman Khabibov]

Hi! Thanks for the review. I decided to drop first commit.

> On Jul 2, 2019, at 6:55 PM, n.pettik <korablev@tarantool.org> wrote:
> 
> 
>>>> @@ -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)?
>> It is needed e.g. for the following queries: “SELECT NOT TRUE COLLATE “unicode””.
>> “NOT” will be on the root. The sequence of calls: sqlExprCodeTagret -> (case TK_NOT)
>> -> sqlExprCodeTemp.
>> 			NOT
>> 		       /   \
>> 		   unicode 0x0
>> 		  /      \
>> 		TRUE	 0x0
>>>> 	int r2;
>>>> 	pExpr = sqlExprSkipCollate(pExpr);
> 
> In fact, problem is here: this function skips collate token.
> So, diff is following:
> 
> @@ -4427,10 +4426,7 @@ 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;
>        int r2;
> -       pExpr = sqlExprSkipCollate(pExpr);
>        if (ConstFactorOk(pParse)
>            && pExpr->op != TK_REGISTER && sqlExprIsConstantNotJoin(pExpr)
>            ) {
@@ -4397,7 +4427,6 @@ int
 sqlExprCodeTemp(Parse * pParse, Expr * pExpr, int *pReg)
 {
 	int r2;
-	pExpr = sqlExprSkipCollate(pExpr);
 	if (ConstFactorOk(pParse)
 	    && pExpr->op != TK_REGISTER && sqlExprIsConstantNotJoin(pExpr)
 	    ) {

> What is more, you don’t need to set type of expression
> in sqlExprCodeTarget():
> 
> @@ -4201,7 +4201,6 @@ 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,
@@ -4173,6 +4201,8 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 		}
 	case TK_SPAN:
 	case TK_COLLATE:{
+			if (check_collate_arg(pParse, pExpr) != 0)
+				break;
 			return sqlExprCodeTarget(pParse, pExpr->pLeft,
 						     target);
 		}

>>>> 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.
>> 
>> Previous test in this file will be same if I remove COLLATE clauses.
> 
> You don’t have to remove collate clause, you should create new
> table with string field type and run this test using that table.
> Comment above the test clearly indicates its purpose.
diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
index 1ca6a6446..67883d2a0 100755
--- a/test/sql-tap/in3.test.lua
+++ b/test/sql-tap/in3.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(27)
+test:plan(29)
 
 --!./tcltestrunner.lua
 -- 2007 November 29
@@ -186,7 +186,17 @@ test:do_test(
         -- </in3-1.13>
     })
 
-
+test:do_execsql_test(
+    "in3-1.14",
+    [[
+        CREATE TABLE t2(a TEXT PRIMARY KEY);
+        INSERT INTO t2 VALUES('A');
+        INSERT INTO t2 VALUES('B');
+        INSERT INTO t2 VALUES('C');
+    ]], {
+        -- <in3-1.14>
+        -- </in3-1.14>
+    })
 
 -- 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
@@ -194,23 +204,32 @@ test:do_test(
 -- require a temp-table, because the collation sequences match.
 --
 test:do_test(
-    "in3-1.14",
+    "in3-1.15",
     function()
-        return exec_neph(" SELECT a FROM t1 WHERE a COLLATE \"unicode_ci\" IN (SELECT a FROM t1) ")
+        return exec_neph(" SELECT a FROM t2 WHERE a COLLATE \"unicode_ci\" IN (SELECT a FROM t2) ")
     end, {
-        -- <in3-1.14>
-        1, 1, 3, 5
-        -- </in3-1.14>
+        -- <in3-1.15>
+        1, "A", "B", "C"
+        -- </in3-1.15>
     })
 
 test:do_test(
-    "in3-1.15",
+    "in3-1.16",
     function()
-        return exec_neph(" SELECT a FROM t1 WHERE a COLLATE \"binary\" IN (SELECT a FROM t1) ")
+        return exec_neph(" SELECT a FROM t2 WHERE a COLLATE \"binary\" IN (SELECT a FROM t2) ")
     end, {
-        -- <in3-1.15>
-        1, 1, 3, 5
-        -- </in3-1.15>
+        -- <in3-1.16>
+        1, "A", "B", "C"
+        -- </in3-1.16>
+    })
+
+test:do_execsql_test(
+    "in3-1.17",
+    [[
+        DROP TABLE t2
+    ]], {
+        -- <in3-1.17>
+        -- </in3-1.17>
     })


commit 7a000757faeab3e2fddefb02b3dc65ef6398c17f
Author: Roman Khabibov <roman.habibov@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 898d16cf3..96824e8e3 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -197,6 +197,34 @@ 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 = sql_expr_type(left);
+	if (type != FIELD_TYPE_STRING && type != FIELD_TYPE_SCALAR) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+			 "COLLATE clause can't be used with non-string "
+			 "arguments");
+		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 +4201,8 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 		}
 	case TK_SPAN:
 	case TK_COLLATE:{
+			if (check_collate_arg(pParse, pExpr) != 0)
+				break;
 			return sqlExprCodeTarget(pParse, pExpr->pLeft,
 						     target);
 		}
@@ -4397,7 +4427,6 @@ int
 sqlExprCodeTemp(Parse * pParse, Expr * pExpr, int *pReg)
 {
 	int r2;
-	pExpr = sqlExprSkipCollate(pExpr);
 	if (ConstFactorOk(pParse)
 	    && pExpr->op != TK_REGISTER && sqlExprIsConstantNotJoin(pExpr)
 	    ) {
diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
index 79547361c..edf4a28b9 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(174)
+test:plan(190)
 
 local prefix = "collation-"
 
@@ -529,4 +529,87 @@ test:do_catchsql_test(
         'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))',
         {1, "Collation 'FOO' does not exist"})
 
+-- 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',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.2",
+    'SELECT one COLLATE "unicode_ci" FROM test1',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.3",
+    'SELECT two COLLATE "binary" FROM test1',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+
+test:do_catchsql_test(
+    "collation-2.7.4",
+    'SELECT (one + two) COLLATE "binary" FROM test1',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.5",
+    'SELECT (SELECT one FROM test1) COLLATE "binary"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_execsql_test(
+    "collation-2.7.6",
+    'SELECT TRIM(\'A\') COLLATE "binary"',
+    {"A"})
+
+test:do_catchsql_test(
+    "collation-2.7.7",
+    'SELECT RANDOM() COLLATE "binary"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.8",
+    'SELECT LENGTH(\'A\') COLLATE "binary"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.9",
+    'SELECT TRUE COLLATE "unicode"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.10",
+    'SELECT NOT TRUE COLLATE "unicode"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.11",
+    'SELECT TRUE AND TRUE COLLATE "unicode"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_catchsql_test(
+    "collation-2.7.12",
+    'SELECT 1 | 1 COLLATE "unicode"',
+    {1, "COLLATE clause can't be used with non-string arguments"})
+
+test:do_execsql_test(
+    "collation-2.7.14",
+    'SELECT +\'str\' COLLATE "unicode"',
+    {"str"})
+
+test:do_execsql_test(
+    "collation-2.7.15",
+    'SELECT (\'con\'||\'cat\') COLLATE "unicode"',
+    {"concat"})
+
+test:do_execsql_test(
+    "collation-2.7.16",
+    'SELECT (SELECT \'str\') COLLATE "binary" COLLATE "binary";',
+    {"str"})
+
 test:finish_test()
diff --git a/test/sql-tap/distinct.test.lua b/test/sql-tap/distinct.test.lua
index e6970084e..ae35a0db5 100755
--- a/test/sql-tap/distinct.test.lua
+++ b/test/sql-tap/distinct.test.lua
@@ -121,12 +121,12 @@ local data = {
     {"12.1", 0, "SELECT DISTINCT a, d FROM t1"},
     {"12.2", 0, "SELECT DISTINCT a, d FROM t4"},
     {"13.1", 0, "SELECT DISTINCT a, b, c COLLATE \"unicode_ci\" FROM t1"},
-    {"13.2", 0, "SELECT DISTINCT a, b, c COLLATE \"unicode_ci\" FROM t4"},
+    {"13.2", 1, "SELECT DISTINCT a, b, c FROM t4"},
     {"14.1", 0, "SELECT DISTINCT a, d COLLATE \"unicode_ci\" FROM t1"},
     {"14.2", 1, "SELECT DISTINCT a, d COLLATE \"unicode_ci\" FROM t4"},
     {"15 ", 0, "SELECT DISTINCT a, d COLLATE \"binary\" FROM t1"},
     {"16.1", 0, "SELECT DISTINCT a, b, c COLLATE \"binary\" FROM t1"},
-    {"16.2", 0, "SELECT DISTINCT a, b, c COLLATE \"binary\" FROM t4"},
+    {"16.2", 1, "SELECT DISTINCT a, b, c FROM t4"},
     {"17",  0,   --{ \/* Technically, it would be possible to detect that DISTINCT\n            ** is a no-op in cases like the following. But sql does not\n            ** do so. *\/\n
     "SELECT DISTINCT t1.id FROM t1, t2 WHERE t1.id=t2.x" },
     {"18 ", 1, "SELECT DISTINCT c1, c2 FROM t3"},
@@ -173,7 +173,7 @@ data = {
     {"a, b, c FROM t1", {"btree"}, {"A", "B", "C", "a", "b", "c"}},
     {"a, b, c FROM t1 ORDER BY a, b, c", {"btree"}, {"A", "B", "C", "a", "b", "c"}},
     {"b FROM t1 WHERE a = 'a'", {}, {"b"}},
-    {"b FROM t1 ORDER BY +b COLLATE \"binary\"", {"btree", "btree"}, {"B", "b"}},
+    {"b FROM t1 ORDER BY b COLLATE \"binary\"", {"btree", "btree"}, {"B", "b"}},
     {"a FROM t1", {}, {"A", "a"}},
     {"b COLLATE \"unicode_ci\" FROM t1", {}, {"b"}},
     {"b COLLATE \"unicode_ci\" FROM t1 ORDER BY b COLLATE \"unicode_ci\"", {}, {"b"}},
diff --git a/test/sql-tap/identifier_case.test.lua b/test/sql-tap/identifier_case.test.lua
index 4db729f11..65ed9aea1 100755
--- a/test/sql-tap/identifier_case.test.lua
+++ b/test/sql-tap/identifier_case.test.lua
@@ -166,7 +166,7 @@ test:do_execsql_test(
 
 test:do_execsql_test(
     test_prefix.."4.1",
-    string.format([[select * from table1 order by a collate "unicode_ci"]]),
+    string.format([[select * from table1 order by a]]),
     {}
 )
 
diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
index 1ca6a6446..67883d2a0 100755
--- a/test/sql-tap/in3.test.lua
+++ b/test/sql-tap/in3.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(27)
+test:plan(29)
 
 --!./tcltestrunner.lua
 -- 2007 November 29
@@ -186,7 +186,17 @@ test:do_test(
         -- </in3-1.13>
     })
 
-
+test:do_execsql_test(
+    "in3-1.14",
+    [[
+        CREATE TABLE t2(a TEXT PRIMARY KEY);
+        INSERT INTO t2 VALUES('A');
+        INSERT INTO t2 VALUES('B');
+        INSERT INTO t2 VALUES('C');
+    ]], {
+        -- <in3-1.14>
+        -- </in3-1.14>
+    })
 
 -- 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
@@ -194,23 +204,32 @@ test:do_test(
 -- require a temp-table, because the collation sequences match.
 --
 test:do_test(
-    "in3-1.14",
+    "in3-1.15",
     function()
-        return exec_neph(" SELECT a FROM t1 WHERE a COLLATE \"unicode_ci\" IN (SELECT a FROM t1) ")
+        return exec_neph(" SELECT a FROM t2 WHERE a COLLATE \"unicode_ci\" IN (SELECT a FROM t2) ")
     end, {
-        -- <in3-1.14>
-        1, 1, 3, 5
-        -- </in3-1.14>
+        -- <in3-1.15>
+        1, "A", "B", "C"
+        -- </in3-1.15>
     })
 
 test:do_test(
-    "in3-1.15",
+    "in3-1.16",
     function()
-        return exec_neph(" SELECT a FROM t1 WHERE a COLLATE \"binary\" IN (SELECT a FROM t1) ")
+        return exec_neph(" SELECT a FROM t2 WHERE a COLLATE \"binary\" IN (SELECT a FROM t2) ")
     end, {
-        -- <in3-1.15>
-        1, 1, 3, 5
-        -- </in3-1.15>
+        -- <in3-1.16>
+        1, "A", "B", "C"
+        -- </in3-1.16>
+    })
+
+test:do_execsql_test(
+    "in3-1.17",
+    [[
+        DROP TABLE t2
+    ]], {
+        -- <in3-1.17>
+        -- </in3-1.17>
     })
 
 -- Neither of these queries require a temp-table. The collation sequence
diff --git a/test/sql-tap/resolver01.test.lua b/test/sql-tap/resolver01.test.lua
index 9fcf0c7c0..315c892ac 100755
--- a/test/sql-tap/resolver01.test.lua
+++ b/test/sql-tap/resolver01.test.lua
@@ -106,7 +106,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.1",
     [[
-        SELECT 2 AS y FROM t1, t2 ORDER BY y COLLATE "unicode_ci";
+        SELECT 2 AS y FROM t1, t2 ORDER BY y;
     ]], {
         -- <resolver01-2.1>
         0, {2}
@@ -116,7 +116,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.2",
     [[
-        SELECT 2 AS yy FROM t1, t2 ORDER BY y COLLATE "unicode_ci";
+        SELECT 2 AS yy FROM t1, t2 ORDER BY y;
     ]], {
         -- <resolver01-2.2>
         1, "ambiguous column name: Y"
@@ -126,7 +126,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.3",
     [[
-        SELECT x AS y FROM t3 ORDER BY y COLLATE "unicode_ci";
+        SELECT x AS y FROM t3 ORDER BY y;
     ]], {
         -- <resolver01-2.3>
         0, {11, 33}
@@ -136,7 +136,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.4",
     [[
-        SELECT x AS yy FROM t3 ORDER BY y COLLATE "unicode_ci";
+        SELECT x AS yy FROM t3 ORDER BY y;
     ]], {
         -- <resolver01-2.4>
         0, {33, 11}
@@ -146,7 +146,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.5",
     [[
-        SELECT x AS yy FROM t3 ORDER BY yy COLLATE "unicode_ci";
+        SELECT x AS yy FROM t3 ORDER BY yy;
     ]], {
         -- <resolver01-2.5>
         0, {11, 33}
@@ -156,7 +156,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "resolver01-2.6",
     [[
-        SELECT x AS yy FROM t3 ORDER BY 1 COLLATE "unicode_ci";
+        SELECT x AS yy FROM t3 ORDER BY 1;
     ]], {
         -- <resolver01-2.6>
         0, {11, 33}
diff --git a/test/sql-tap/select1.test.lua b/test/sql-tap/select1.test.lua
index 6beeb34cb..6811f7dcb 100755
--- a/test/sql-tap/select1.test.lua
+++ b/test/sql-tap/select1.test.lua
@@ -1672,7 +1672,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "select1-10.7",
     [[
-        SELECT f1 COLLATE "unicode_ci" AS x FROM test1 ORDER BY x
+        SELECT f1 AS x FROM test1 ORDER BY x
     ]], {
         -- <select1-10.7>
         11, 33
diff --git a/test/sql-tap/tkt-b75a9ca6b0.test.lua b/test/sql-tap/tkt-b75a9ca6b0.test.lua
index 89817d2af..dde6ab502 100755
--- a/test/sql-tap/tkt-b75a9ca6b0.test.lua
+++ b/test/sql-tap/tkt-b75a9ca6b0.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(22)
+test:plan(20)
 
 --!./tcltestrunner.lua
 -- 2014-04-21
@@ -57,7 +57,6 @@ local eqps = {
     {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x, y DESC", {1, 3, 2, 2, 3, 1}, {idxscan, sort}},
     {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x DESC, y DESC", {3, 1, 2, 2, 1, 3}, {idxscan, sort}},
     {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x ASC, y ASC", {1, 3, 2, 2, 3, 1}, {idxscan}},
-    {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x COLLATE \"unicode_ci\", y", {1, 3, 2, 2, 3, 1}, {idxscan, sort}},
 }
 for tn, val in ipairs(eqps) do
     local q = val[1]
diff --git a/test/sql-tap/with1.test.lua b/test/sql-tap/with1.test.lua
index ec45e5e76..6985c589e 100755
--- a/test/sql-tap/with1.test.lua
+++ b/test/sql-tap/with1.test.lua
@@ -1043,7 +1043,7 @@ test:do_catchsql_test(13.3, [[
 -- 2015-04-12
 --
 test:do_execsql_test(14.1, [[
-  WITH x AS (SELECT * FROM t) SELECT 0 EXCEPT SELECT 0 ORDER BY 1 COLLATE "binary";
+  WITH x AS (SELECT * FROM t) SELECT 0 EXCEPT SELECT 0 ORDER BY 1;
 ]], {
   -- <14.1>
   

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

[n.pettik]

Lgtm.

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

[Kirill Yukhin]

Hello,

I've checked your patch into master.

--
Regards, Kirill Yukhin