impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4403: Implement SHOW RANGE PARTITIONS for Kudu tables
Date Wed, 07 Dec 2016 18:22:57 GMT
Lars Volker has posted comments on this change.

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


Patch Set 2:

(5 comments)

Thanks for the reviews, please see PS3.

http://gerrit.cloudera.org:8080/#/c/5390/2//COMMIT_MSG
Commit Message:

Line 1: Parent:     87fd39ad (IMPALA-4477: Bump Kudu version to latest master (60aa54e))
> can you add a test cases:
Done.
1) by adding a "show range partitions" query to kudu_alter.test. I change the analysis to
throw an exception for non-range partitioned tables.
2) added test AnalyzeDDLTest#TestShowRangePartitions()

I'll work on 3) next while waiting for the next round of review comments.


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

PS2, Line 187: PARTITION
> PARTITIONS
Done


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

Line 421:     resultSchema.addToColumns(new TColumn("Partition Specifier", Type.STRING.toThrift()));
> Maybe Range Partition Specifier?
Done


Line 421:     resultSchema.addToColumns(new TColumn("Partition Specifier", Type.STRING.toThrift()));
> In the Kudu web UI we are calling it 'RANGE (col1, col2, col3, ...) PARTITI
Thanks Dan. I changed it to RANGE (col1, col2, col3, ...).


http://gerrit.cloudera.org:8080/#/c/5390/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

PS2, Line 364:       // TODO Should we just pass params here? Or will this make the interfaces
depend too
             :       // much on thrift types?
> we'll have to branch somewhere... I'm not sure it's clearly  better to hand
We could also branch here so we don't need an additional method there, but pass params to
simplify the signatures. However it's stylistic and I'm also good with keeping this change
minimal.


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburkert@apache.org>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message