Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3481: Use Kudu ScanToken API for scan ranges ......................................................................
Patch Set 1: (13 comments) Thanks for the feedback. I made changes/follow-up comments for everything except for how we do the plan output printing, since Dan is going to provide a printing fn for us. http://gerrit.cloudera.org:8080/#/c/4120/1/be/src/exec/kudu-scan-node.cc File be/src/exec/kudu-scan-node.cc: PS1, Line 225: range > GetNextScanToken returns a ... "range" :). I think we should clean this a b Done http://gerrit.cloudera.org:8080/#/c/4120/1/be/src/exec/kudu-scan-node.h File be/src/exec/kudu-scan-node.h: PS1, Line 35: tablet > Hm, maybe I am confused but a server can have multiple tablets that need to Done PS1, Line 76: string > not sure if y'all follow the same style guide, but this should be std::stri Done Line 79: int next_scan_token_idx_; > I may have missed something, but it looks like you could just use scan_toke I started changing this, but I think it's nice to just be passing string* around as the tokens. I could still use this as a stack if I store them somewhere else (e.g. a mempool), but not sure if it's worth it. PS1, Line 125: ranges > tokens and ranges are sort of used interchangeably. Maybe we should keep on Done http://gerrit.cloudera.org:8080/#/c/4120/1/be/src/exec/kudu-scanner.h File be/src/exec/kudu-scanner.h: PS1, Line 43: 'range' > update comment? Done 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: PS1, Line 64: (<=, >=, ==) > Comment needs to be updated. Done PS1, Line 77: // TODO use the portion of this list that pertains to keys to do partition pruning. > I think we can remove this TODO now, no? Done Line 95: kuduTable_.getKuduMasterAddresses()).build()) { > nit: indentation is a bit weird, maybe it's Impala style though Done PS1, Line 133: e master > I think you mean LEADER replica Done PS1, Line 233: <=, >=, == > Update comment Done PS1, Line 269: column > has it already been validated that the slot refs only refer to columns that Yeah, random exceptions might not come back nicely. I added a validate method which happens before this and throws a more specific error. PS1, Line 264: if (!(predicate.getChild(0) instanceof SlotRef)) return null; : if (!predicate.getChild(1).isLiteral()) return null; : : SlotRef ref = (SlotRef)predicate.getChild(0); : String colName = ref.getDesc().getColumn().getName(); : ColumnSchema column = table.getSchema().getColumn(colName); : ComparisonOp op = getKuduOperator(((BinaryPredicate)predicate).getOp()); : if (op == null) return null; > How about putting all the logic of checking if a predicate can be pushed to Done -- 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: Dan Burkert <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
