Alex Behm has posted comments on this change. Change subject: IMPALA-4561: Replace DISTRIBUTE BY with PARTITION BY in CREATE TABLE ......................................................................
Patch Set 1: (7 comments) To avoid more rounds, maybe we should sync on the exact class/var names before you make changes. http://gerrit.cloudera.org:8080/#/c/5317/1/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: Line 380: struct TPartitionParam { TKuduPartitionParam? http://gerrit.cloudera.org:8080/#/c/5317/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 404: nonterminal TableDataLayout opt_tbl_data_layout, distributed_data_layout; also change distributed_data_layout Line 411: nonterminal PartitionParam partition_hash_param; KuduPartitionParam? Seems better to be specific and not confuse it with HDFS partitioning, since "Param" is very generic. Line 1084: tbl_def.getPartitionColumnDefs().addAll(data_layout.getPartitionColumnDefs()); regarding my rename suggestion: I'd imagine these two lines here are rather confusing for any one but the two of us I understand that doing another rename is extremely annoying, but some parts of the code are pretty confusing now because to other readers the difference between partition "column defs" and "params" seems quite unclear. http://gerrit.cloudera.org:8080/#/c/5317/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: Line 88: public List<ColumnDef> getPartitionColumnDefs() { these two getters are also confusing http://gerrit.cloudera.org:8080/#/c/5317/1/fe/src/main/java/org/apache/impala/analysis/RangePartition.java File fe/src/main/java/org/apache/impala/analysis/RangePartition.java: Line 113: public void analyze(Analyzer analyzer, List<ColumnDef> partitioningColDefs) partColDefs? http://gerrit.cloudera.org:8080/#/c/5317/1/fe/src/main/java/org/apache/impala/analysis/TableDataLayout.java File fe/src/main/java/org/apache/impala/analysis/TableDataLayout.java: Line 25: * Represents the PARTITION BY and PARTITIONED BY clauses of a DDL statement. Fingers crossed. -- To view, visit http://gerrit.cloudera.org:8080/5317 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0e07c41eabb4c8cb95754cf04293cbd9e03d6ab2 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-HasComments: Yes
