Alex Behm has posted comments on this change.

Change subject: IMPALA-4571: InList predicates not being pushed to Kudu scans
......................................................................


Patch Set 2:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/5316/2//COMMIT_MSG
Commit Message:

Line 7: IMPALA-4571: InList predicates not being pushed to Kudu scans
Push IN predicates to Kudu.

(or something along those lines which summarizes what this change does)


Line 12: An InPredicate can be pushed to the scan if:
might be more concise to mention that only these predicates are supported:
<SlotRef> IN (<LiteralExpr>, <LiteralExpr>, ...)


Line 13: 1) It has a list of values (i.e. not a subquery)
list of literal values


Line 17:    exactly.
one requirement missing from this list is is that all values in the IN list 
must be literals


http://gerrit.cloudera.org:8080/#/c/5316/2/fe/src/main/java/org/apache/impala/analysis/InPredicate.java
File fe/src/main/java/org/apache/impala/analysis/InPredicate.java:

Line 54:     Preconditions.checkArgument(isAnalyzed_);
Not really a requirement, why not just inline this one-line function at the 
caller?


http://gerrit.cloudera.org:8080/#/c/5316/2/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java:

Line 366:     if (predicate.isNotIn() || !predicate.hasValuesList()) return 
false;
the hasValuesList() check seems unecessary, it is already covered by checking 
the children below


Line 371:     PrimitiveType type = ref.getType().getPrimitiveType();
unused?


Line 397:   private static Object getKuduInListValue(PrimitiveType type, Expr 
e) {
Move to LiteralExpr.valueAsObject() or something like that?


Line 399:     if (type != e.getType().getPrimitiveType()) return null;
This is a post-condition of InPredicate.analyze(), you don't need to check it 
here again.


Line 409:       default: return null;
It sounds like a bug if we hit this case because it implies we have a SlotRef 
on a type that is not supported by Kudu.

Is there an easy way to check if a type is supported by Kudu? If so, you can 
add a Preconditions check for that instead on the type of the uncast SlotRef.


http://gerrit.cloudera.org:8080/#/c/5316/2/testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
File 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test:

Line 108: double_col in (0.0) and
string_col?


Line 117: double_col in (cast('inf' as double))
* add a NOT IN predicate
* add an IN predicate with a SlotRef in the IN list


http://gerrit.cloudera.org:8080/#/c/5316/2/testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
File testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test:

Line 1084:    kudu predicates: l_shipmode IN ('AIR', 'AIR REG'), l_shipinstruct 
= 'DELIVER IN PERSON'
nice improvements!


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8988d4819d20d467b48e286917e347ca00f60cf0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to