Casey Ching has posted comments on this change. Change subject: Refactor CREATE TABLE grammar rules in prep for PRIMARY KEY ......................................................................
Patch Set 4: (13 comments) http://gerrit.cloudera.org:8080/#/c/2865/4/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 400: nonterminal ArrayList<ColumnDef> column_def_list, partitioned_col_defs, > partition_column_defs Done Line 988: parser.parseError("external", SqlParserSymbols.KW_EXTERNAL); > indent 2 Done Line 990: RESULT = new CreateTableDataSrcStmt(new CreateTableStmt(tbl_def, > weird indentation Ya not sure what happened there Line 994: | tbl_def_without_col_defs:tbl_def > this seems to belong in create_tbl_like_stmt The two LIKE forms return different types so they were kept separate. I didn't want to consolidate them, that would be too unrelated to the Kudu stuff. http://gerrit.cloudera.org:8080/#/c/2865/4/fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java File fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java: Line 161: AnalysisUtil.throwIfNotNullOrNotEmpty(getDistributeParams(), > Where does AnalysisUtil come from? Did you add it in this patch? That was Matt's suggestion. Fixed in the parent patch. http://gerrit.cloudera.org:8080/#/c/2865/4/fe/src/main/java/com/cloudera/impala/analysis/TableDataArrangementParams.java File fe/src/main/java/com/cloudera/impala/analysis/TableDataArrangementParams.java: Line 22: * This is a helper class for the parser to store some state without needing to create a > Better to describe what this class represents, e.g. Done Line 25: class TableDataArrangementParams { > TableDataLayout? (it's shorter) Done Line 28: private final List<ColumnDef> partitionColDefs_ = Lists.newArrayList(); > Is it valid to have both partitionColDefs and distributeParams? If not I su Added but its a strange thing to do. The parser and analysis code decides what is valid. It's slightly better for maintenance reasons if this class doesn't have that knowledge. http://gerrit.cloudera.org:8080/#/c/2865/4/fe/src/main/java/com/cloudera/impala/analysis/TableDefClause.java File fe/src/main/java/com/cloudera/impala/analysis/TableDefClause.java: Line 28: > brief comment Done Line 29: class TableDefClause { > how about just TableDef Done Line 43: private final TableDataArrangementParams dataArrangement_; > // Partition/distribute by clauses. Done Line 45: private TableName fullyQualifiedTableName_; > // Result of analysis. Done http://gerrit.cloudera.org:8080/#/c/2865/4/fe/src/main/java/com/cloudera/impala/analysis/TableOptionsClause.java File fe/src/main/java/com/cloudera/impala/analysis/TableOptionsClause.java: Line 31: class TableOptionsClause { > TableDefOptions? Done -- To view, visit http://gerrit.cloudera.org:8080/2865 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9a2b9e380a0b90c0e2e6f10f6905cab5164cb3c4 Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Casey Ching <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
