Alex Behm has posted comments on this change. Change subject: IMPALA-5003: Generic constant propagation in planner ......................................................................
Patch Set 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/6389/5//COMMIT_MSG Commit Message: Line 7: IMPALA-5003: Generic constant propagation in planner Instead of "Generic constant propagation in planner" I'd propose something like "Constant propagation within scan conjuncts" The constant propagation is not really "generic" because we are consciously missing out on a lot of opportunities. 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., Adds a new ConstantPropagator class that propagates constants within a list of conjuncts... and so on 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: if (e.isConstant() && !Expr.IS_FALSE_LITERAL.apply(e)) it.remove(); Looks wrong. We cannot just drop the 'false' predicate. I think we should either leave the predicate or address the TODO. In any case, I don't think we should remove constant 'false' predicates here. The caller should decide what to do with them. We can, however, remove constant 'true' predicates. 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, > Let me see if that comes out cleaner. Did it not come out cleaner? 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' predicate? I think in those cases we should generate an EmptySetNode instead of a ScanNode. 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_expr_rewrites", so let's use that directly. We can make it more generic in the future if the need arises. 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: # Test multiple forward propagataion Add a similar test for Kudu in one of the Kudu .test files. typo: propagation Line 18: where a.int_col1 = 10 and a.int_col2 = a.int_col1 + 1 and a.int_col3 = a.int_col2 * 5 flip the lhs/rhs of some predicates to test the expr normalization within the constant propagator Line 27: predicates: a.int_col1 = 10, a.int_col2 = 11, a.int_col3 = 55, a.int_col4 = -385 whitespace Line 43: # Another impossibility What's the impossibility? Line 56: # An obviously empty query where the predicate is visible Can we move this into an HBase .test file? It's sometimes better to keep the tests separate. For example, we'd need to skip this entire test if Kudu was not supported in some dev environment (based on the KUDU_IS_SUPPORTED env var) Line 67: ==== Add negative test cases with arbitrary exprs, e.g. slot refs with explicit and implicit casts to show that those are not considered for propagation. Examples: // explicit cast where cast(int_col as string) = 'abc' and cast(int_col as string) > 'xyz' // implicit cast where tinyint_col = 10000 and tinyint_col < 10000 // arbitrary expr where to_date(timestamp_col) = '2017-12-20' and to_date(timestamp_col) substr(replace(to_date(timestamp_col), '-', ''), 1, 4) = '2016' -- 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <zams...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-HasComments: Yes
