From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 04832469719 for ; Thu, 12 Mar 2020 03:14:51 +0300 (MSK) References: <20200309150258.32584-1-roman.habibov@tarantool.org> <6B917D0D-6B5F-4CC3-A7ED-43B0768B2537@tarantool.org> From: Vladislav Shpilevoy Message-ID: <83e93b75-de08-865d-a8c2-9e899e27cc09@tarantool.org> Date: Thu, 12 Mar 2020 01:14:50 +0100 MIME-Version: 1.0 In-Reply-To: <6B917D0D-6B5F-4CC3-A7ED-43B0768B2537@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] sql: use unify pattern for column names List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov , tarantool-patches@dev.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 > 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 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 > ]=], { > -- > 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.