Todd Lipcon has posted comments on this change. Change subject: IMPALA-3481: Use Kudu ScanToken API for scan ranges ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/4120/1//COMMIT_MSG Commit Message: Line 36: work), add TPC-DS tests that demonstrate pruning. I think the pruning isn't quite working on the Kudu side quite yet. You can follow https://issues.apache.org/jira/browse/KUDU-1065 http://gerrit.cloudera.org:8080/#/c/4120/1/fe/src/main/java/com/cloudera/impala/planner/KuduScanNode.java File fe/src/main/java/com/cloudera/impala/planner/KuduScanNode.java: Line 95: kuduTable_.getKuduMasterAddresses()).build()) { nit: indentation is a bit weird, maybe it's Impala style though PS1, Line 133: e master I think you mean LEADER replica PS1, Line 269: column has it already been validated that the slot refs only refer to columns that exist? eg if the metastore has some outdated information about the columns, this will throw an IllegalArgumentException. Would that propagate up to the user in a reasonable way? http://gerrit.cloudera.org:8080/#/c/4120/1/fe/src/test/java/com/cloudera/impala/planner/PlannerTestBase.java File fe/src/test/java/com/cloudera/impala/planner/PlannerTestBase.java: PS1, Line 39: import org.kududb.client.Client.ScanTokenPB; : import org.kududb.client.shaded.com.google.protobuf.ByteString; : import org.kududb.client.shaded.com.google.protobuf.InvalidProtocolBufferException; yea, this is pretty evil, there's no compatibility guarantee about these APIs so would strongly discourage using them PS1, Line 281: KUDU KEYRANGE > Is this part used in any of the planner tests? I don't remember seeing it a KEYRANGE isn't really accurate -- this makes it seem like it's a primary key range, where in fact it's a partition key range (which may not be one and the same) Line 297: result.append(Arrays.toString(upperBound.toByteArray())); maybe we should provide you a nicer toString() type method you can use? In addition to being an evil use of our internal APIs, this isn't particularly useful for users to look at. You're just going to see something like [0,0,0,13,5,2,4,3,...] which is hardly friendly. -- To view, visit http://gerrit.cloudera.org:8080/4120 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I160e5849d372755748ff5ba3c90a4651c804b220 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
