impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4561: Replace DISTRIBUTE BY with PARTITION BY in CREATE TABLE
Date Fri, 02 Dec 2016 06:04:27 GMT
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 <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message