impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5003: Generic constant propagation in planner
Date Mon, 20 Mar 2017 23:17:27 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-5003: Generic constant propagation in planner
......................................................................


Patch Set 3:

(12 comments)

I'll post a WIP update to this diff as there was some refactoring that turned out to be necessary,
and some new revelations about HBase key filtering have come up.

http://gerrit.cloudera.org:8080/#/c/6389/5//COMMIT_MSG
Commit Message:

Line 7: IMPALA-5003: [DRAFT] Generic constant propagation in planner
> Instead of "Generic constant propagation in planner" I'd propose something 
Done


Line 9: Rather than specialize the constant propagation framework to be specific
> Maybe rephrase to what the new code does and where it is applied, e.g.,
Done


http://gerrit.cloudera.org:8080/#/c/6389/5/fe/src/main/java/org/apache/impala/analysis/ConstantPropagator.java
File fe/src/main/java/org/apache/impala/analysis/ConstantPropagator.java:

Line 115
> Looks wrong. We cannot just drop the 'false' predicate. I think we should e
No, you missed the ! - we certainly can't drop the false predicate, but we do want to drop
constant true predicates.  I can probably (e.isConstant() && Expr.IS_TRUE_LITERAL)
to make this more clear - this skips dropping constants that don't evaluate to bools, but
there shouldn't be any such conjuncts anyway.


http://gerrit.cloudera.org:8080/#/c/6389/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:

Line 1217:     ConstantPropagator.optimizeConjuncts(conjuncts, analyzer,
> Did it not come out cleaner?
It did, but required some refactoring.


http://gerrit.cloudera.org:8080/#/c/6389/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:

Line 1217:     ConstantPropagator.optimizeConjuncts(conjuncts, analyzer,
> Have you tried running a few queries that end up with a constant 'false' pr
In practice this is more complicated.  In many places we've already created a ScanNode of
some sort and end up adding impossible false filters on to it.  For HBase key filtering, this
is actually made worse by optimizing (key = 'a' && false) => (false), and then
the key = 'a' conjunct is not picked up by key range optimization and we end up with an unbounded
key range scan across all data on all nodes :(

I'm trying to rework so that this converts to an EmptySetNode, but this is actually more difficult
than it should be.  Ideally we would pull the conjunct optimization to figure out impossible
scans first and have a generic way to create an EmptySetNode similar across all scan types,
but some more refactoring would be needed to do that cleanly (turns out this already works
for inline views).


http://gerrit.cloudera.org:8080/#/c/6389/5/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java
File fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java:

Line 48:   public enum RuleSet {
> This enum seems to add an unnecessary indirection. It's the same as "enable
Done


http://gerrit.cloudera.org:8080/#/c/6389/5/testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test
File testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test:

Line 16
> Add a similar test for Kudu in one of the Kudu .test files.
Will do.

Also, this word 'propagation' is my nemesis of all words in the English language.


Line 18
> flip the lhs/rhs of some predicates to test the expr normalization within t
Done


Line 27
> whitespace
I noticed this too after the diff was sent


Line 43
> What's the impossibility?
copypasta mistake


Line 56
> Can we move this into an HBase .test file? It's sometimes better to keep th
Done


Line 67
> Add negative test cases with arbitrary exprs, e.g. slot refs with explicit 
I think we also want to test with analytic expressions and functions returning bool as conjuncts


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I79750a8edb945effee2a519fa3b8192b77042cb4
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <zamsden@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message