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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
