[Tarantool-patches] [PATCH v3 1/2] sql: use unify pattern for column names

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 4 comments below.

On 11/06/2020 17:18, Roman Khabibov wrote:

> sql: use unify pattern for column names

1. 'Unify' is a verb. You don't need 'use' before it.

> Name resulting columns generated by an expression or <VALUES>
> construction by the "COLUMN_N" pattern.
> 
> Closes #3962
> 
> @TarantoolBot document
> Title: Column naming in SQL
> 
> Now, every auto generated column is named by the "COLUMN_N"
> pattern, where N is the number of column in a query (starting)
> from 1). Auto generated column is a column in a query result
> generated by an expression or a column from <VALUES>
> construction.
> 
> Examples:
> box.execute("VALUES(1, 2, 3);")
> ---
> - metadata:
>   - name: COLUMN_1
>     type: integer
>   - name: COLUMN_2
>     type: integer
>   - name: COLUMN_3
>     type: integer
>   rows:
>   - [1, 2, 3]
> ...
> box.execute("SELECT * FROM (VALUES (1+1, 1+1));")
> ---
> - metadata:
>   - name: COLUMN_1
>     type: integer
>   - name: COLUMN_2
>     type: integer
>   rows:
>   - [2, 2]
> ...
> box.execute("SELECT 1+1, 1+1;")
> ---
> - metadata:
>   - name: COLUMN_1
>     type: integer
>   - name: COLUMN_2
>     type: integer
>   rows:
>   - [2, 2]
> ...

2. Please, wrap code into ``` to make the github markdown
more readable.

3. What if I specify first column by myself, and the second
is an expression? Will it be _1 or _2?

> @@ -4951,6 +4951,18 @@ selectExpander(Walker * pWalker, Select * p)
>  		       || (pE->pLeft != 0 && pE->pLeft->op == TK_ID));
>  		if (pE->op == TK_DOT && pE->pRight->op == TK_ASTERISK)
>  			break;
> +		/*
> +		 * It is necessary to generate auto names for
> +		 * expressions before clauses such as ORDER BY,
> +		 * GROUP BY will be resolved. These names can be
> +		 * found inside these clauses during resolving.
> +		 */
> +		if (pEList->a[k].zName == NULL && pE->op != TK_DOT &&
> +		    ((pE->flags & EP_Leaf) == 0 || pE->op != TK_ID)) {
> +			uint32_t idx = ++pParse->autoname_i;
> +			pEList->a[k].zName =
> +				sqlDbStrDup(db, sql_generate_column_name(idx));

4. I don't think I understand what is happening here. Looks too
complicated. Why do you need so many checks, what do these checks
mean, how are they related to ORDER BY/GROUP BY? And how is it
related to select expansion (the function is called selectExpander())?
Why does it work fine when I use normal column names in ORDER BY/GROUP BY,
and does not work here without this crutch?

Also, this looks broken:

tarantool> box.execute('SELECT *, s1 COLLATE "unicode_ci" FROM j ORDER BY column_1;')
---
- null
- Can’t resolve field 'COLUMN_1'
...


tarantool> box.execute('SELECT s1 COLLATE "unicode_ci" FROM j ORDER BY column_1;')
---
- metadata:
  - name: COLUMN_1
    type: scalar
  rows:
  - [1]
...

When I add '*' before the column_1, it stops working.