Alex Behm has posted comments on this change. Change subject: Preview: Refactor CREATE TABLE grammar rules in prep for PRIMARY KEY ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/2865/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 384: create_tbl_prefix_without_col_defs, create_tbl_stmt, This rule is particularly difficult to deal with and split up. The general approach looks reasonable to me, but I think we should try to minimize the number of production rules and simplify as much as possible. For example, the create_tbl_with_or_without_col_defs_stmt seems to be used only once. Can we inline it into that single place? I'm wondering whether there is a more logical way to split up the rules and also create corresponding "clauses" or similar. For example, these things seem logically grouped: 1. CREATE [EXTERNAL] TABLE <name> [COLUMN DEFS] 2. [PARTITIONED BY / DISTRIBUTE BY] 3. other options Now that I had a closer look, maybe we can have another chat and brainstorm. I'd not go ahead and follow my suggestions just yet :) Line 931: create_tbl_without_col_defs_or_partitions_stmt:create_tbl_stmt instead of adding new productions with long names, can we combine the shorter individual productions? Line 987: create_tbl_without_col_defs_or_partitions_stmt ::= Candidate for inlining because the rule is so simple. Line 996: create_tbl_prefix_without_col_defs ::= inline? Line 1011: opt_distribute_param_list:distribute tbl_properties:tbl_props Unfortunate that this is almost at the end. All the other similar clauses like PARTITIONED BY, CLUSTERED BY, SKEWED BY (latter two from Hive) come after the table comment in Hive. -- 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: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Casey Ching <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
