From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 dev.tarantool.org (Postfix) with ESMTPS id D90CD42EF5C for ; Wed, 24 Jun 2020 02:12:26 +0300 (MSK) References: <20200611151853.24398-1-roman.habibov@tarantool.org> <20200611151853.24398-2-roman.habibov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 24 Jun 2020 01:12:25 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v3 1/2] sql: use unify pattern for column names List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: roman Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! >> 3. What if I specify first column by myself, and the second >> is an expression? Will it be _1 or _2? > > tarantool> CREATE TABLE j (column_1 SCALAR PRIMARY KEY); > --- > - row_count: 1 > ... > > tarantool> INSERT INTO j VALUES(1); > --- > - row_count: 1 > ... > > tarantool> SELECT column_1, column_1 COLLATE "unicode_ci" FROM j ORDER BY column_1; > --- > - metadata: >   - name: COLUMN_1 >     type: scalar >   - name: COLUMN_1 >     type: scalar >   rows: >   - [1, 1] > ... > > If we want that the second expression to be named "COLUMN_2". should we use a > hash table in struct Parse. We will push the names of all columns in the request > to it and generate names without collisions. I don't now anymore ways. > I would leave it as it is now. I didn't say anything about behaviour change. I just asked a simple question, and I don't see an answer here. I will repeat it: what if my query contains 2 columns - one normal column name and the second is an expression. Will the expression be visible as COLUMN_1 or as COLUMN_2 in the result set? SELECT mycol, mycol + 1 FROM test; Will 'mycol + 1' be stored as COLUMN_1 or COLUMN_2? It is a simple question to clarify in the documentation request. Because it will be asked if you don't explicitly say what will happen. >>> @@ -4951,6 +4951,18 @@ selectExpander(Walker * pWalker, Select * p) >>> || (pE->pLeft != 0 && pE->pLeft->op == TK_ID)); >>> if (pE->op == TK_DOT && pE->pRight->op == TK_ASTERISK) >>> break; >>> + /* >>> + * It is necessary to generate auto names for >>> + * expressions before clauses such as ORDER BY, >>> + * GROUP BY will be resolved. These names can be >>> + * found inside these clauses during resolving. >>> + */ >>> + if (pEList->a[k].zName == NULL && pE->op != TK_DOT && >>> + ((pE->flags & EP_Leaf) == 0 || pE->op != TK_ID)) { >>> + uint32_t idx = ++pParse->autoname_i; >>> + pEList->a[k].zName = >>> + sqlDbStrDup(db, sql_generate_column_name(idx)); >> 4. I don't think I understand what is happening here. Looks too >> complicated. Why do you need so many checks, what do these checks >> mean, how are they related to ORDER BY/GROUP BY? And how is it >> related to select expansion (the function is called selectExpander())? >> Why does it work fine when I use normal column names in ORDER BY/GROUP BY, >> and does not work here without this crutch? > Here I name all items (generate names) in expression list if it is needed. > I think this operation can be attributed to "expanding". Expanding is before > name resolving. At the resolving all items must be named. > > You can use normal column names in ORDER BY, GROUP BY etc, I checked it in Postgre > (Postgre names expression the same way). I think it is OK. Can you use auto names in PostgreSQL too? Inside the query. Are they a full featured column name, or just sugar for result set metadata? See 5 comments below. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 4b069ad..7a56136 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -1989,6 +1987,8 @@ sqlColumnsFromExprList(Parse * parse, ExprList * expr_list, >              memcpy(space_def->fields[i].name, zName, name_len); >              space_def->fields[i].name[name_len] = '\0'; >          } > +        sqlDbFree(db, expr_list->a[i].zName); > +        expr_list->a[i].zName = zName; 1. I deleted this hunk and nothing changed in the tests. Why do you need it? >      } >  cleanup: >      sqlHashClear(&ht); > @@ -4792,6 +4792,25 @@ selectPopWith(Walker * pWalker, Select * p) >      } >  } >   > +/** > + * Determine whether to generate a name for @a expr or not. > + * > + * Auto generated names is needed for every item in  a