impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3481: Use Kudu ScanToken API for scan ranges
Date Thu, 25 Aug 2016 21:02:32 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-3481: Use Kudu ScanToken API for scan ranges

Patch Set 1:


Overall it looks good. At a high level, 'ranges' and 'tokens' seem to be used interchangeably
which is a bit confusing. Also, I think we need more planner tests that exercise the logic
of pushing predicates to Kudu.
File be/src/exec/

PS1, Line 225: range
GetNextScanToken returns a ... "range" :). I think we should clean this a bit.
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 be scanned, correct?
Don't we create one scanner per node?

PS1, Line 125: ranges
tokens and ranges are sort of used interchangeably. Maybe we should keep one?
File be/src/exec/kudu-scanner.h:

PS1, Line 43: 'range'
update comment?
File fe/src/main/java/com/cloudera/impala/planner/

PS1, Line 64: (<=, >=, ==)
Comment needs to be updated.

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?

PS1, Line 233: <=, >=, ==
Update comment

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 Kudu into a single
function? That would include this block and also some parts from extractKuduConjuncts() fn
(e.g. L242-247).
File fe/src/test/java/com/cloudera/impala/planner/

Is this part used in any of the planner tests? I don't remember seeing it anywhere.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I160e5849d372755748ff5ba3c90a4651c804b220
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-HasComments: Yes

View raw message