Alex Behm has posted comments on this change. Change subject: Refactor CREATE TABLE grammar rules in prep for PRIMARY KEY ......................................................................
Patch Set 4: (13 comments) Thanks a lot for doing this cleanup and splitting up the original analyze(). Looks much better to me. 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 Line 988: parser.parseError("external", SqlParserSymbols.KW_EXTERNAL); indent 2 Line 990: RESULT = new CreateTableDataSrcStmt(new CreateTableStmt(tbl_def, weird indentation Line 994: | tbl_def_without_col_defs:tbl_def this seems to belong in create_tbl_like_stmt 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? We typically use the plural "Utils" suffix 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. Represents the PARTITION BY and DISTRIBUTED BY clauses of a DDL statements. That the parser uses this should then be clear Line 25: class TableDataArrangementParams { TableDataLayout? (it's shorter) Line 28: private final List<ColumnDef> partitionColDefs_ = Lists.newArrayList(); Is it valid to have both partitionColDefs and distributeParams? If not I suggest adding static helper functions like createPartitionedLayout() or createDistributedLayout() 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 Line 29: class TableDefClause { how about just TableDef Line 43: private final TableDataArrangementParams dataArrangement_; // Partition/distribute by clauses. Line 45: private TableName fullyQualifiedTableName_; // Result of analysis. 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? brief comment -- 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
