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

["n.pettik" <korablev@tarantool.org>]

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