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

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

Hi! Thanks for the patch!

> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3962-column
> Issue: https://github.com/tarantool/tarantool/issues/3962

Yeah, you added the links afterwards, but still you missed the
changelog again. I once again ask you to create a document with
pre-check actions to do before you send anything.

>> 03/03/2020, 23:13> Also you seem to be very consistent with forgetting to put issue
>> and branch links to your patchsets. Please, create a text document
>> with a list of things to check before sending a patch (issue and
>> branch links, changelog, check that the branch is pushed, etc),
>> save it somewhere, and look at it before a send. I have such
>> document, and it helps sometimes.

See 8 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. I would use 'COLUMN_N', without leading '_', which usually
means something internal, not intended for being used by users.
But even better option - ask Nikita.

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

2. Starting from what? 0, 1?

>     generated column is a column in a query result generated by an
>     expression or a column from <VALUES> construction.

3. Better provide examples. Peter may be in the context, and
would probably extend the description by himself, but the issue
may be taken by a different technical writer. Be more
descriptive.

> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 1579cc92e..8c9708cf6 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4513,4 +4513,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.
> + *
> + * @param number Number of column.
> + *
> + * @retval string success.

4. Yeah, the comment is correct, but too much water, I would
say. These @param and @retval are not obligatory, as I said
sometime earlier. So many details is clearly an overkill for
such a simple function. Better keep it a one-line comment, or
if you feel inspiration - write about why do we change column
names to that template. About the standard being silent on
the topic, about postgre and mariadb behaviour.

> + */
> +static inline char *
> +sql_generate_column_name(uint32_t number)
> +{
> +	return (char *) tt_sprintf("_COLUMN_%d", number);

5. Better keep it const to prevent any potential attempts
to change or even free the name memory.

====================
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 54b6f83d8..b50f512e7 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1946,9 +1946,8 @@ sqlColumnsFromExprList(Parse * parse, ExprList * expr_list,
 				zName = pColExpr->u.zToken;
 			}
 		}
-		if (zName == NULL)
-			zName = sql_generate_column_name(i + 1);
-		zName = sqlMPrintf(db, "%s", zName);
+		zName = sqlDbStrDup(db, zName != NULL ?
+				    zName : sql_generate_column_name(i + 1));
 
 		/* 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 8c9708cf6..3360ab67e 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4520,10 +4520,10 @@ sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
  *
  * @retval string success.
  */
-static inline char *
+static inline const char *
 sql_generate_column_name(uint32_t number)
 {
-	return (char *) tt_sprintf("_COLUMN_%d", number);
+	return tt_sprintf("_COLUMN_%d", number);
 }
 
 #endif				/* sqlINT_H */
====================

> diff --git a/test/sql-tap/select6.test.lua b/test/sql-tap/select6.test.lua
> index c9960dc29..e99260505 100755
> --- a/test/sql-tap/select6.test.lua
> +++ b/test/sql-tap/select6.test.lua
> @@ -128,9 +128,9 @@ test:do_execsql_test(
>  test:do_execsql_test(
>      "select6-1.7",
>      [=[
> -        SELECT a.y, a."count(*)", "max(x)", "count(*)"
> -        FROM (SELECT count(*),y FROM t1 GROUP BY y) AS a,
> -             (SELECT max(x),y FROM t1 GROUP BY y) as b
> +        SELECT a.y, a.count, max, count
> +        FROM (SELECT count(*) AS count, y FROM t1 GROUP BY y) AS a,
> +             (SELECT max(x) AS max, y FROM t1 GROUP BY y) as b

6. Don't auto-names work here? Why didn't you use _COLUMN_N
names?

>          WHERE a.y=b.y ORDER BY a.y
>      ]=], {
>          -- <select6-1.7>
> 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.

> -...
> + | ---
> + | ...
> +
8. There should be a test specifically for the issue. Labeled
with its number, with a comment, and with the tests from the
initial Peter's comment. Add them, please.