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-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
Date Wed, 07 Dec 2016 02:11:28 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5390/1/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

Line 182:   1: required bool is_show_col_stats
replace the two flags by an enum and use the thrift enum in the stmt code?


http://gerrit.cloudera.org:8080/#/c/5390/1/fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowPartitionsStmt.java:

Line 64:       if (kuduTable.getPrimaryKeyColumnNames().isEmpty()) {
Not possible. Kudu tables must have PKs.


http://gerrit.cloudera.org:8080/#/c/5390/1/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java:

Line 31:   protected final boolean isShowColStats_;
The subclasses and flags are getting kind of confusing.
For example, setting both isShowColsStats_ and isShowRangePartitions_ is illegal.

One idea for cleaning it up might be to introduce an enum for the different uses and move
all the code inside here (i.e. merge the logic of ShowPartitions inside here).

Another alternative might be to have one subclass for the different uses.

Any other ideas?


http://gerrit.cloudera.org:8080/#/c/5390/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

Line 432:       for (String partition: partitions) {
Do you know if the partitions are sorted? Might be good to do that, otherwise it might be
hard to search for a specific partition.


-- 
To view, visit http://gerrit.cloudera.org:8080/5390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf5b2fdd02938a42fa59ec98884e4ac915dd1f65
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message