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

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

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index f39484013..d65266586 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1815,6 +1815,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>  		vdbe_metadata_set_col_nullability(v, i, -1);
>  		const char *colname = pEList->a[i].zName;
>  		const char *span = pEList->a[i].zSpan;
> +		p = sqlExprSkipCollate(p);

1. Why? Firstly, would be cool to have a comment here.

Secondly, I commented the line out and got this diff hunk
(among others):

[016] --- sql/full_metadata.result	Fri May  1 20:51:21 2020
[016] +++ sql/full_metadata.reject	Fri May  1 20:58:53 2020
[016] @@ -76,10 +76,9 @@
[016]  execute("SELECT c COLLATE \"unicode\" FROM t;")
[016]   | ---
[016]   | - metadata:
[016] - |   - span: c COLLATE "unicode"
[016] - |     type: string
[016] - |     is_nullable: true
[016] - |     name: C
[016] + |   - type: string
[016] + |     span: c COLLATE "unicode"
[016] + |     name: COLUMN_1
[016]   |     collation: unicode
[016]   |   rows:
[016]   |   - ['aSd']

So you basically return {c COLLATE \"unicode\"} as normal column {C}.
Looks like a bad idea. Peter confirmed it in his comment:

    Peter:
      CREATE TABLE j (s1 SCALAR PRIMARY KEY);
      SELECT s1 COLLATE "unicode_ci" FROM j;
      I see that the name of the column is COLUMN_1.
      Okay.
      For a while I thought that S1 would be okay too, but
      now I realize that would be confusing.

When you say "column COLLATE", this is not very different for a string,
than saying 'column + 1' for an integer column. This becomes a new
value, not the original column.

>  		if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
>  			char *zCol;
>  			int iCol = p->iColumn;
> @@ -1847,19 +1848,19 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>  				if (space->sequence != NULL &&
>  				    space->sequence_fieldno == (uint32_t) iCol)
>  					vdbe_metadata_set_col_autoincrement(v, i);
> -				if (colname != NULL)
> +				if (span != NULL)
>  					vdbe_metadata_set_col_span(v, i, span);

2. Is it related to the issue? Looks like it is not. I reverted it
and got some tests failing, but definitely not related to this issue.

Please, extract it into a separate commit.

Just in case I will say - it is not bad to make small commits. On the
contrary, the smaller the commit the better. The most important condition -
keep them atomic. So as it wouldn't be possible to remove anything
and still solve the declared task. Even if a commit becomes just one
line of code changes + a few tests.

Moreover, this commit would need to be backported. It is a bug.