From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A242731228 for ; Sat, 8 Dec 2018 07:18:55 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XhxZL7sN6u50 for ; Sat, 8 Dec 2018 07:18:55 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 95AA527ECD for ; Sat, 8 Dec 2018 07:18:52 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH] sql: set names for constant fields within VIEW References: <20181203220354.81845-1-korablev@tarantool.org> <2d3f83eb-b9db-23c8-b57e-cd79c954cae2@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sat, 8 Dec 2018 15:18:46 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.org On 07/12/2018 18:02, n.pettik wrote: > > >> On 4 Dec 2018, at 15:14, Vladislav Shpilevoy 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; > ]], { > -- > - "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 > -- > }) > > @@ -168,7 +168,7 @@ test:do_execsql2_test( > SELECT * FROM v2 ORDER BY 2; > ]], { > -- > - "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 > -- > }) > > @@ -259,7 +259,7 @@ test:do_execsql2_test( > SELECT v1.a, * FROM v1 ORDER BY 2; > ]], { > -- > - "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 > -- > }) > > @@ -269,7 +269,7 @@ test:do_execsql2_test( > SELECT * FROM v2 ORDER BY 2; > ]], { > -- > - "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 > -- > }) > > @@ -279,7 +279,7 @@ test:do_execsql2_test( > SELECT * FROM v3 ORDER BY 2; > ]], { > -- > - "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 > -- > }) > > @@ -289,7 +289,7 @@ test:do_execsql2_test( > SELECT * FROM v4 ORDER BY 2; > ]], { > -- > - "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 > -- > }) > > @@ -380,7 +380,7 @@ test:do_execsql2_test( > SELECT * FROM v1 ORDER BY 2; > ]], { > -- > - "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 > -- > }) > > @@ -390,7 +390,7 @@ test:do_execsql2_test( > SELECT * FROM v2 ORDER BY 2; > ]], { > -- > - "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 > -- > }) > > @@ -400,7 +400,7 @@ test:do_execsql2_test( > SELECT * FROM v3 ORDER BY 2; > ]], { > -- > - "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 > -- > }) > > @@ -410,7 +410,7 @@ test:do_execsql2_test( > SELECT * FROM v4 ORDER BY 2; > ]], { > -- > - "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 > -- > }) > > @@ -420,7 +420,7 @@ test:do_execsql2_test( > SELECT * FROM v5 ORDER BY 2; > ]], { > -- > - "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 > -- > }) > > @@ -430,7 +430,7 @@ test:do_execsql2_test( > SELECT * FROM v6 ORDER BY 2; > ]], { > -- > - "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 > -- > }) > > >> 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.