impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3481: Use Kudu ScanToken API for scan ranges
Date Mon, 29 Aug 2016 18:21:43 GMT
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 <mj@cloudera.com>
Gerrit-Reviewer: Dan Burkert <dan@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <todd@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message