impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6307: CTAS statement fails with duplicate column exception.
Date Sat, 06 Jan 2018 01:13:03 GMT
Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8930
)

Change subject: IMPALA-6307: CTAS statement fails with duplicate column exception.
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8930/1/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java:

http://gerrit.cloudera.org:8080/#/c/8930/1/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java@223
PS1, Line 223: Preconditions.checkState(colDefs.size() + partitionColDefs.size() == types.size());
             :     for (int i = 0; i < colDefs.size(); ++i) colDefs.get(i).setType(types.get(i));
             :     for (int i = 0; i < partitionColDefs.size(); ++i) {
             :       partitionColDefs.get(i).setType(types.get(i + colDefs.size()));
             :     }
I believe it would be nice to add a comment in AnalysisContext.java L405 to indicate what
these types represent for the case of a CTAS. Then it will be more clear why this block is
needed.


http://gerrit.cloudera.org:8080/#/c/8930/1/fe/src/main/java/org/apache/impala/analysis/TableDef.java
File fe/src/main/java/org/apache/impala/analysis/TableDef.java:

http://gerrit.cloudera.org:8080/#/c/8930/1/fe/src/main/java/org/apache/impala/analysis/TableDef.java@160
PS1, Line 160: dataLayout_.getPartitionColumnDefs().clear();
I would add a reset() function in TableDataLayout() and put that there.


http://gerrit.cloudera.org:8080/#/c/8930/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:

http://gerrit.cloudera.org:8080/#/c/8930/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1510
PS1, Line 1510: AnalyzesOk("create table functional.ctas_tbl partitioned by (year) as " +
              :         "with tmp as (select a.timestamp_col, a.year from functional.alltypes
a " +
              :         "left join functional.alltypes b " +
              :         "on b.timestamp_col between a.timestamp_col and a.timestamp_col) "
+
              :         "select a.timestamp_col, a.year from tmp a");
Add a comment. It will not be clear to everyone what this thing is testing.



-- 
To view, visit http://gerrit.cloudera.org:8080/8930
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee053abecd4384e15eec8db10cb06f5ace159da2
Gerrit-Change-Number: 8930
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga <zoram@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Comment-Date: Sat, 06 Jan 2018 01:13:03 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message