Zach Amsden has posted comments on this change. Change subject: IMPALA-5003: Constant propagation in scan nodes and inline views ......................................................................
Patch Set 11: (15 comments) Thanks for the review! http://gerrit.cloudera.org:8080/#/c/6389/11/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: Line 924: * Propagates constant expression of the form <slot ref> = <constant> to > expressions Done Line 931: for (Expr expr: conjuncts) { > It's probably not that important to worry about perf, but I think it should I thought about this. There are certain problems. 1. If we want to find contradictions, we need to apply the smap to even <slot reg> = <constant> exprs, for other slot refs. This is what produces <5> = <7>, etc.. 2. It is and this is what I originally wanted to do. But we need to apply the smap to all exprs except the original expr itself. There does not seem to be any machinery to apply an smap to all conjuncts except for the conjunct from which the smap was built, nor an obvious and uncomplicated way to build it. 3. This we should probably do. I had a mock-up that did this at one point in time but abandoned it for a simpler design that allowed for subsequent or potentially concurrent optimization. Now that complexity has crept in at least another dimension (inequality elision), it should be more obvious how to do this in a way that doesn't prevent the latter optimization at a future point in time. Line 964: private static boolean collateInequalities(List<Expr> conjuncts, Analyzer analyzer) { > Please avoid scope creep and leave this out. Feel free to file a new JIRA f Will do and I agree. Will cause some minor test refactoring, but that is not a big problem. http://gerrit.cloudera.org:8080/#/c/6389/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: Line 968: * to construct a ValueRange, > whitespace will fix comment Line 993: if (!(comp.getOp() == BinaryPredicate.Operator.GT) && > use != Done Line 996: !(comp.getOp() == BinaryPredicate.Operator.LE)) continue; > use {} for multi-line ifs Done Line 1134: Expr.optimizeConjuncts(preds, analyzer); > I don't think we should add this in this patch because the behavior could b Indeed, optimizing pre-substituted exprs was a bug (caused test failures due to unassigned conjuncts which are fixed in the latest revision). (See line 1157 just below - eliminating duplicates and replacing exprs is not valid here!). Line 1252: TupleDescriptor tupleDesc = tblRef.getDesc(); > inline into the only use in L1256 Done Line 1256: TupleId id = tupleDesc.getId(); > id -> tid Done Line 1266: if (!Expr.optimizeConjuncts(conjuncts, analyzer)) { > Only if expr rewrites are enabled? Optimization should still be done in the form of eliminating duplicates, even if rewrites are disabled. It was my intention to make rewrites only happen if they were enabled, if that is not the case, will fix. Line 1267: // Conjuncts implied false; convert to emptySetNode > EmptySetNode Done Line 1270: return node; > nice! :) http://gerrit.cloudera.org:8080/#/c/6389/11/fe/src/main/java/org/apache/impala/planner/ValueRange.java File fe/src/main/java/org/apache/impala/planner/ValueRange.java: Line 107: !(upperBound_ instanceof LiteralExpr) || !(lowerBound_ instanceof LiteralExpr)) > use {} for multi-line ifs Done http://gerrit.cloudera.org:8080/#/c/6389/11/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java File fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java: Line 48: private Expr rewrite(Expr expr, Analyzer analyzer, boolean reanalyze) > Why are these changes needed? Can we avoid them? Let's discuss in person the best approach here. The reason for re-analyze is that the higher level code has no idea what has been rewritten, so it can only attempt to reanalyze everything. Line 110: public void reset() { > single line Done -- 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: 11 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
