From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 2BC9D4696C3 for ; Fri, 1 May 2020 22:37:49 +0300 (MSK) References: <20200426230900.23258-1-roman.habibov@tarantool.org> <20200426230900.23258-2-roman.habibov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <6a301d10-d7af-a8ef-4ebe-061dfc6e74a7@tarantool.org> Date: Fri, 1 May 2020 21:37:44 +0200 MIME-Version: 1.0 In-Reply-To: <20200426230900.23258-2-roman.habibov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 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 Khabibov , tarantool-patches@dev.tarantool.org Thanks for the patch! See 2 comments below. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index f39484013..d65266586 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -1815,6 +1815,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList, > vdbe_metadata_set_col_nullability(v, i, -1); > const char *colname = pEList->a[i].zName; > const char *span = pEList->a[i].zSpan; > + p = sqlExprSkipCollate(p); 1. Why? Firstly, would be cool to have a comment here. Secondly, I commented the line out and got this diff hunk (among others): [016] --- sql/full_metadata.result Fri May 1 20:51:21 2020 [016] +++ sql/full_metadata.reject Fri May 1 20:58:53 2020 [016] @@ -76,10 +76,9 @@ [016] execute("SELECT c COLLATE \"unicode\" FROM t;") [016] | --- [016] | - metadata: [016] - | - span: c COLLATE "unicode" [016] - | type: string [016] - | is_nullable: true [016] - | name: C [016] + | - type: string [016] + | span: c COLLATE "unicode" [016] + | name: COLUMN_1 [016] | collation: unicode [016] | rows: [016] | - ['aSd'] So you basically return {c COLLATE \"unicode\"} as normal column {C}. Looks like a bad idea. Peter confirmed it in his comment: Peter: CREATE TABLE j (s1 SCALAR PRIMARY KEY); SELECT s1 COLLATE "unicode_ci" FROM j; I see that the name of the column is COLUMN_1. Okay. For a while I thought that S1 would be okay too, but now I realize that would be confusing. When you say "column COLLATE", this is not very different for a string, than saying 'column + 1' for an integer column. This becomes a new value, not the original column. > if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) { > char *zCol; > int iCol = p->iColumn; > @@ -1847,19 +1848,19 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList, > if (space->sequence != NULL && > space->sequence_fieldno == (uint32_t) iCol) > vdbe_metadata_set_col_autoincrement(v, i); > - if (colname != NULL) > + if (span != NULL) > vdbe_metadata_set_col_span(v, i, span); 2. Is it related to the issue? Looks like it is not. I reverted it and got some tests failing, but definitely not related to this issue. Please, extract it into a separate commit. Just in case I will say - it is not bad to make small commits. On the contrary, the smaller the commit the better. The most important condition - keep them atomic. So as it wouldn't be possible to remove anything and still solve the declared task. Even if a commit becomes just one line of code changes + a few tests. Moreover, this commit would need to be backported. It is a bug.