Dan Burkert has posted comments on this change.

Change subject: KUDU-1065: [java client] Flexible Partition Pruning
......................................................................


Patch Set 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4299/3/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java:

Line 311:    * Not for public consumption: use either predicates or primary key 
bounds instead.
> given it's package-private, do you need this comment?
Done


Line 325:    * Not for public consumption: use either predicates or primary key 
bounds instead.
> same
Done


http://gerrit.cloudera.org:8080/#/c/4299/3/java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java:

Line 197:     if (len > 1) {
> is this 'if' really necessary? seems like if len <= 1, the len - 2 would be
Done


http://gerrit.cloudera.org:8080/#/c/4299/3/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java:

PS3, Line 171:       // The range component is constrained.
             :       constrainedIndex = hashBuckets.size();
> not 100% following here, can you explain further?
Done


http://gerrit.cloudera.org:8080/#/c/4299/3/java/kudu-client/src/main/java/org/apache/kudu/util/ByteVec.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ByteVec.java:

Line 116:   public void reserve(int additional) {
> I find it surprising that 'reserve' here is reserving _additional_ bytes, v
Done


http://gerrit.cloudera.org:8080/#/c/4299/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
File 
java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java:

PS3, Line 97:     for (Partition partition : partitions) {
            :       if (!pruner.shouldPrune(partition)) scannedPartitions++;
            :     }
> do we expect loops like this in real life? this seems o(n^2). Should we off
This is true, but currently shouldPrune is only called in these tests.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib27b54841d87cf854175ab8cdfa8798b337d71f9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to