[tarantool-patches] Re: [PATCH 1/2] sql: fix collation node duplication in AST

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes! See 2 comments below.

On 28/05/2019 17:10, Roman Khabibov wrote:
> Hi! Thanks for the review.
> 
>> I vote for the second as the simplest, and add a test provided by me
>> above to ensure we will never break it accidentally.
>>
>> This commit should consist of the test and a comment in resolveAlias.
>> And keep this nice ASCII schema.
> 
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 504096e6d..f952d8c04 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -109,6 +109,11 @@ 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.
> +	 */

1. Please, add a comment, that 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 0bf54576d..3c5d3053a 100755
> --- a/test/sql-tap/collation.test.lua
> +++ b/test/sql-tap/collation.test.lua
> @@ -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. Must pass.

2. Of course it must pass. It is the purpose of test. Please,
drop 'Must pass'.

> +
> +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()
>