Alex Behm has posted comments on this change. Change subject: IMPALA-5003: Constant propagation in scan nodes and inline views ......................................................................
Patch Set 11: (15 comments) 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 Line 931: for (Expr expr: conjuncts) { It's probably not that important to worry about perf, but I think it should be simple enough to change this algorithm to avoid redundant and unnecessary work. For example: - we only need to apply the smap to non <slot ref> = <constant> exprs - it's cheaper to create one big smap, rather than one smap with a single mapping - each propagation round definitely reduces the set of exprs we need to consider in the next round 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 for this improvement. 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 Line 993: if (!(comp.getOp() == BinaryPredicate.Operator.GT) && use != Line 996: !(comp.getOp() == BinaryPredicate.Operator.LE)) continue; use {} for multi-line ifs Line 1134: Expr.optimizeConjuncts(preds, analyzer); I don't think we should add this in this patch because the behavior could be confusing to us and users (might get different plans depending on small changes to a query, even worse you might get better plans if you use more inline views!). Also why would we even optimize the pre-substituted exprs, i.e. before L1139? Line 1252: TupleDescriptor tupleDesc = tblRef.getDesc(); inline into the only use in L1256 Line 1256: TupleId id = tupleDesc.getId(); id -> tid Line 1266: if (!Expr.optimizeConjuncts(conjuncts, analyzer)) { Only if expr rewrites are enabled? Line 1267: // Conjuncts implied false; convert to emptySetNode EmptySetNode 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 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? Not re-analyzing is an issue, but I'd prefer to solve that differently. Line 110: public void reset() { single line -- 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
