Re: [Tarantool-patches] [PATCH] sql: use unify pattern for column names

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

Hi! Thanks for the fixes!

>>> diff --git a/test/sql/gh-3888-values-blob-assert.result b/test/sql/gh-3888-values-blob-assert.result
>>> index 0a1af28f2..d2e27efd9 100644
>>> --- a/test/sql/gh-3888-values-blob-assert.result
>>> +++ b/test/sql/gh-3888-values-blob-assert.result
>>> @@ -1,3 +1,4 @@
>>> +-- test-run result file version 2
>>> -- sql: assertion fault on VALUES #3888
>>> --
>>> -- Make sure that tokens representing values of integer, float,
>>> @@ -5,79 +6,87 @@
>>> -- keywords of the same names.
>>> --
>>> test_run = require('test_run').new()
>>> ----
>>> -...
>>> + | ---
>>> + | ...
>>> engine = test_run:get_cfg('engine')
>>> ----
>>> -...
>>> + | ---
>>> + | ...
>>> _ = box.space._session_settings:update('sql_default_engine', {{'=', 2, engine}})
>>> ----
>>
>> 7. These and similar changes look unrelated to the
>> issue. In some other files too.
> What I should do? Tests will fall, if I don’t add these changes.

Well, they won't. Test-run interprets .result files using their
version in the first line. You added the version 2, and it
expects output of the version 2. Don't add version 2, and you
will be able to keep it without these changes.

I suspect you updated the file by removing the old .result, and
creating a new by running the test. But it is not really correct.
Better move .reject file into .result file. Then the version won't
change.

See 6 comments below.

>     sql: use unify pattern for column names
>     
>     Name resulting columns generated by an expression or <VALUES>
>     construction by the "_COLUMN_N" pattern.

1. The pattern here and below is outdated. There is no '_' in the
first character anymore.

>     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]
>     ...
> 
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 65e41f219..f39983761 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1946,14 +1946,13 @@ sqlColumnsFromExprList(Parse * parse, ExprList * expr_list,
>  			} else if (pColExpr->op == TK_ID) {
>  				assert(!ExprHasProperty(pColExpr, EP_IntValue));
>  				zName = pColExpr->u.zToken;
> -			} else {
> -				/* Use the original text of the column expression as its name */
> -				zName = expr_list->a[i].zSpan;
>  			}
>  		}
> -		if (zName == NULL)
> -			zName = "_auto_field_";
> -		zName = sqlMPrintf(db, "%s", zName);
> +		if (zName == NULL) {
> +			uint32_t idx = ++parse->autoname_i;
> +			zName = (char *) sql_generate_column_name(idx);

2. Wait, I remember I asked to keep it const. I even provided a mini patch
how to keep it without casts. Why did you leave non-const char here?

> +		}
> +		zName = sqlDbStrDup(db, zName);
>  
>  		/* Make sure the column name is unique.  If the name is not unique,
>  		 * append an integer to the name so that it becomes unique.
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 1579cc92e..c355d1b17 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -2223,6 +2223,8 @@ struct Parse {
>  	TriggerPrg *pTriggerPrg;	/* Linked list of coded triggers */
>  	With *pWith;		/* Current WITH clause, or NULL */
>  	With *pWithToFree;	/* Free this WITH object at the end of the parse */
> +	/** Index of previous auto generated name */

3. Please, add a dot in the end.

> +	uint32_t autoname_i;
>  	/** Space triggers are being coded for. */
>  	struct space *triggered_space;
>  	/**
> @@ -4513,4 +4515,17 @@ int
>  sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
>  		    uint32_t *fieldno);
>  
> +/**
> + * Return a string of the form "_COLUMN_N", where N is @a number.

4. The template it outdated.

> + *
> + * We decided to name every auto generated column in output by
> + * this pattern (like PostgreSQL), because it is convenient than

5. 'convenient' -> 'more convenient'.

> + * "_auto_name_" and naming with span like MariaDB do.
> + */
> +static inline const char *
> +sql_generate_column_name(uint32_t number)
> +{
> +	return tt_sprintf("COLUMN_%d", number);
> +}
> +
>  #endif				/* sqlINT_H */
> diff --git a/test/sql/full_metadata.test.lua b/test/sql/full_metadata.test.lua
> index a9ba320c9..90972c48c 100644
> --- a/test/sql/full_metadata.test.lua
> +++ b/test/sql/full_metadata.test.lua
> @@ -47,7 +47,15 @@ execute("SELECT x, y FROM v;")
>  
>  execute([[UPDATE "_session_settings" SET "value" = false WHERE "name" = 'sql_full_metadata';]])
>  
> +-- gh-3962: Check auto generated names in different selects.

6. This does not look like the right file for that. Because nothing
here concerns full metadata is sent or not. colname.test.lua may be
the right place.