[tarantool-patches] Re: [PATCH] sql: set names for constant fields within VIEW

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

On 07/12/2018 18:02, n.pettik wrote:
> 
> 
>> On 4 Dec 2018, at 15:14, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Hi! Thanks for the patch!
>>
>> On 04/12/2018 01:03, Nikita Pettik wrote:
>>> If VIEW contains constant fields (e.g. CREATE VIEW v AS SELECT 1, 'k';)
>>> it uses string representation of literal as a field name. In the example
>>> above it would be '1' and 'k'. However, if VIEW is created using AS VALUES
>>> syntax, then expressions representing constant literals lack of names
>>> (since span-expression is not assigned in this case).
>>> Lets generate names for all fields which lack names for VIEW.
>>> Closes #3849
>>> ---
>>> Branch: https://github.com/tarantool/tarantool/commits/np/gh-3849-view-constant-value-v2
>>> Issue: https://github.com/tarantool/tarantool/issues/3849
>>
>> I prefer this variant to parser's one. Parser fix weakens perf adding Span
>> to each expr even not used for VALUES.
> 
> I prefer v2 for the same reason.
> 
>> The patch is ok except field name. Are you sure '__auto-fld:%d' is ok? I
>> think here we should follow our usual naming policy: single '_' prefix for
>> system names, use '_' instead of '-', ' ', ':'. And do not use contractions
>> (as I remember we do not contract system names, but I can be wrong).
>>
>> Also I propose to start fields enumeration from 1.
>>
>> So my favorite name template is '_auto_field_%d', from 1 to N_fields.
> 
> Ok, but I had to fix several tests to use this pattern:
> 
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 839224131..5e80225fe 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1872,7 +1872,7 @@ sqlite3ColumnsFromExprList(Parse * parse, ExprList * expr_list, Table *table)
>                          }
>                  }
>                  if (zName == NULL)
> -                       zName = "__auto-fld";
> +                       zName = "_auto_field_";
>                  zName = sqlite3MPrintf(db, "%s", zName);
>   
>                  /* Make sure the column name is unique.  If the name is not unique,
> @@ -1885,11 +1885,11 @@ sqlite3ColumnsFromExprList(Parse * parse, ExprList * expr_list, Table *table)
>                                  int j;
>                                  for (j = nName - 1;
>                                       j > 0 && sqlite3Isdigit(zName[j]); j--);
> -                               if (zName[j] == ':')
> +                               if (zName[j] == '_')
>                                          nName = j;
>                          }
>                          zName =
> -                           sqlite3MPrintf(db, "%.*z:%u", nName, zName, ++cnt);
> +                           sqlite3MPrintf(db, "%.*z_%u", nName, zName, ++cnt);
> 
> 
> diff --git a/test/sql-tap/colname.test.lua b/test/sql-tap/colname.test.lua
> index 207e7a979..4423e8ead 100755
> --- a/test/sql-tap/colname.test.lua
> +++ b/test/sql-tap/colname.test.lua
> @@ -158,7 +158,7 @@ test:do_execsql2_test(
>           SELECT * FROM v1 ORDER BY 2;
>       ]], {
>           -- <colname-2.8>
> -        "A", 1, "X", 4, "A:1", 1, "B", 2, "C", 3, "X:1", 4, "Y", 5, "Z", 6
> +        "A",1,"X",4,"A_1",1,"B",2,"C",3,"X_1",4,"Y",5,"Z",6
>           -- </colname-2.8>
>       })
>   
> @@ -168,7 +168,7 @@ test:do_execsql2_test(
>           SELECT * FROM v2 ORDER BY 2;
>       ]], {
>           -- <colname-2.9>
> -        "A", 1, "X", 4, "A:1", 11, "X:1", 14, "A:2", 1, "B", 2, "C", 3, "X:2", 4, "Y", 5, "Z", 6, "A:3", 11, "B:1", 12, "C:1", 13, "X:3", 14, "Y:1", 15, "Z:1", 16
> +        "A",1,"X",4,"A_1",11,"X_1",14,"A_2",1,"B",2,"C",3,"X_2",4,"Y",5,"Z",6,"A_3",11,"B_1",12,"C_1",13,"X_3",14,"Y_1",15,"Z_1",16
>           -- </colname-2.9>
>       })
>   
> @@ -259,7 +259,7 @@ test:do_execsql2_test(
>           SELECT v1.a, * FROM v1 ORDER BY 2;
>       ]], {
>           -- <colname-3.8>
> -        "v1.a", 1, "A", 1, "X", 4, "A:1", 1, "B", 2, "C", 3, "X:1", 4, "Y", 5, "Z", 6
> +        "v1.a",1,"A",1,"X",4,"A_1",1,"B",2,"C",3,"X_1",4,"Y",5,"Z",6
>           -- </colname-3.8>
>       })
>   
> @@ -269,7 +269,7 @@ test:do_execsql2_test(
>           SELECT * FROM v2 ORDER BY 2;
>       ]], {
>           -- <colname-3.9>
> -        "A", 1, "X", 4, "A:1", 11, "X:1", 14, "A:2", 1, "B", 2, "C", 3, "X:2", 4, "Y", 5, "Z", 6, "A:3", 11, "B:1", 12, "C:1", 13, "X:3", 14, "Y:1", 15, "Z:1", 16
> +        "A",1,"X",4,"A_1",11,"X_1",14,"A_2",1,"B",2,"C",3,"X_2",4,"Y",5,"Z",6,"A_3",11,"B_1",12,"C_1",13,"X_3",14,"Y_1",15,"Z_1",16
>           -- </colname-3.9>
>       })
>   
> @@ -279,7 +279,7 @@ test:do_execsql2_test(
>           SELECT * FROM v3 ORDER BY 2;
>       ]], {
>           -- <colname-3.10>
> -        "A", 1, "X", 4, "A:1", 1, "B", 2, "C", 3, "X:1", 4, "Y", 5, "Z", 6
> +        "A",1,"X",4,"A_1",1,"B",2,"C",3,"X_1",4,"Y",5,"Z",6
>           -- </colname-3.10>
>       })
>   
> @@ -289,7 +289,7 @@ test:do_execsql2_test(
>           SELECT * FROM v4 ORDER BY 2;
>       ]], {
>           -- <colname-3.11>
> -        "A", 1, "X", 4, "A:1", 11, "X:1", 14, "A:2", 1, "B", 2, "C", 3, "X:2", 4, "Y", 5, "Z", 6, "A:3", 11, "B:1", 12, "C:1", 13, "X:3", 14, "Y:1", 15, "Z:1", 16
> +        "A",1,"X",4,"A_1",11,"X_1",14,"A_2",1,"B",2,"C",3,"X_2",4,"Y",5,"Z",6,"A_3",11,"B_1",12,"C_1",13,"X_3",14,"Y_1",15,"Z_1",16
>           -- </colname-3.11>
>       })
>   
> @@ -380,7 +380,7 @@ test:do_execsql2_test(
>           SELECT * FROM v1 ORDER BY 2;
>       ]], {
>           -- <colname-4.8>
> -        "V1.A", 1, "V1.X", 4, "V1.A:1", 1, "V1.B", 2, "V1.C", 3, "V1.X:1", 4, "V1.Y", 5, "V1.Z", 6
> +        "V1.A",1,"V1.X",4,"V1.A_1",1,"V1.B",2,"V1.C",3,"V1.X_1",4,"V1.Y",5,"V1.Z",6
>           -- </colname-4.8>
>       })
>   
> @@ -390,7 +390,7 @@ test:do_execsql2_test(
>           SELECT * FROM v2 ORDER BY 2;
>       ]], {
>           -- <colname-4.9>
> -        "V2.A", 1, "V2.X", 4, "V2.A:1", 11, "V2.X:1", 14, "V2.A:2", 1, "V2.B", 2, "V2.C", 3, "V2.X:2", 4, "V2.Y", 5, "V2.Z", 6, "V2.A:3", 11, "V2.B:1", 12, "V2.C:1", 13, "V2.X:3", 14, "V2.Y:1", 15, "V2.Z:1", 16
> +        "V2.A",1,"V2.X",4,"V2.A_1",11,"V2.X_1",14,"V2.A_2",1,"V2.B",2,"V2.C",3,"V2.X_2",4,"V2.Y",5,"V2.Z",6,"V2.A_3",11,"V2.B_1",12,"V2.C_1",13,"V2.X_3",14,"V2.Y_1",15,"V2.Z_1",16
>           -- </colname-4.9>
>       })
>   
> @@ -400,7 +400,7 @@ test:do_execsql2_test(
>           SELECT * FROM v3 ORDER BY 2;
>       ]], {
>           -- <colname-4.10>
> -        "V3.A", 1, "V3.X", 4, "V3.A:1", 1, "V3.B", 2, "V3.C", 3, "V3.X:1", 4, "V3.Y", 5, "V3.Z", 6
> +        "V3.A",1,"V3.X",4,"V3.A_1",1,"V3.B",2,"V3.C",3,"V3.X_1",4,"V3.Y",5,"V3.Z",6
>           -- </colname-4.10>
>       })
>   
> @@ -410,7 +410,7 @@ test:do_execsql2_test(
>           SELECT * FROM v4 ORDER BY 2;
>       ]], {
>           -- <colname-4.11>
> -        "V4.A", 1, "V4.X", 4, "V4.A:1", 11, "V4.X:1", 14, "V4.A:2", 1, "V4.B", 2, "V4.C", 3, "V4.X:2", 4, "V4.Y", 5, "V4.Z", 6, "V4.A:3", 11, "V4.B:1", 12, "V4.C:1", 13, "V4.X:3", 14, "V4.Y:1", 15, "V4.Z:1", 16
> +        "V4.A",1,"V4.X",4,"V4.A_1",11,"V4.X_1",14,"V4.A_2",1,"V4.B",2,"V4.C",3,"V4.X_2",4,"V4.Y",5,"V4.Z",6,"V4.A_3",11,"V4.B_1",12,"V4.C_1",13,"V4.X_3",14,"V4.Y_1",15,"V4.Z_1",16
>           -- </colname-4.11>
>       })
>   
> @@ -420,7 +420,7 @@ test:do_execsql2_test(
>           SELECT * FROM v5 ORDER BY 2;
>       ]], {
>           -- <colname-4.12>
> -        "V5.A", 1, "V5.X", 4, "V5.A:1", 1, "V5.B", 2, "V5.C", 3, "V5.X:1", 4, "V5.Y", 5, "V5.Z", 6
> +        "V5.A",1,"V5.X",4,"V5.A_1",1,"V5.B",2,"V5.C",3,"V5.X_1",4,"V5.Y",5,"V5.Z",6
>           -- </colname-4.12>
>       })
>   
> @@ -430,7 +430,7 @@ test:do_execsql2_test(
>           SELECT * FROM v6 ORDER BY 2;
>       ]], {
>           -- <colname-4.13>
> -        "V6.A", 1, "V6.X", 4, "V6.A:1", 11, "V6.X:1", 14, "V6.A:2", 1, "V6.B", 2, "V6.C", 3, "V6.X:2", 4, "V6.Y", 5, "V6.Z", 6, "V6.A:3", 11, "V6.B:1", 12, "V6.C:1", 13, "V6.X:3", 14, "V6.Y:1", 15, "V6.Z:1", 16
> +        "V6.A",1,"V6.X",4,"V6.A_1",11,"V6.X_1",14,"V6.A_2",1,"V6.B",2,"V6.C",3,"V6.X_2",4,"V6.Y",5,"V6.Z",6,"V6.A_3",11,"V6.B_1",12,"V6.C_1",13,"V6.X_3",14,"V6.Y_1",15,"V6.Z_1",16
>           -- </colname-4.13>
>       })
> 
> 
>> Also why don't you add number suffix to a first name? Maybe it is worth
>> naming all fields in the same way?
> 
> IMHO it looks more clear when first member comes as is
> (i.e. without ordinal number).
> 

As you wish. LGTM.