impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5003: [DRAFT] Generic constant propagation in planner
Date Wed, 15 Mar 2017 04:38:33 GMT
Alex Behm has posted comments on this change.

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


Patch Set 3:

(17 comments)

We still need tests. I suggest either adding a new unittest similar to ExprRewriteTest, or
just adding the tests in there.

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

Line 79:     rewriter_ = ExprRewriter.createExprRewriter(
I think we don't actually need the rewriter_ as a member, and instead we can create it on
demand in analyze() below using the createExprRewriter() in Analyzer (see my other comment
on adding this convenience function)


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

Line 2777:                       + " to " + slotIds.second.toString());
I'm not even sure this message helps at all. Remove?


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

Line 1: 
extra newline


Line 44: public final class ConstantPropagator {
brief class comment


Line 51:   //public static ConstantPropagator INSTANCE = new ConstantPropogator();
remove


PS3, Line 54:   
comment on thrown exception and state of conjuncts in case of error


Line 56:   public static boolean propagateConstants(List<Expr> conjuncts, Analyzer analyzer)
make private, we can still make it public later when the need arises, but for now we only
expect optimizeConjuncts() to be called


Line 63:     while (changed) {
brief comment, maybe with a simple example to show why we need to do this repeatedly


Line 80:             try {
no need for try/catch if we're just going to throw e anyway


Line 87:           output.set(i, rewritten);
Seems cleaner to only do this when there was a change. That's how our expr rewrites generally
work (returned expr == original expr, if there were no changes)


Line 90:       if (changed) Collections.copy(conjuncts, output);
Move this outside of the loop. You can iterate over output in L65


Line 95:   // Propogates constants, removes duplicates and optionally performs expr rewriting
Mention that errors in the rewrite are logged, but ignored, and the 'conjuncts' remain unchanged
then


Line 101:         for (int i = 0; i < conjuncts.size(); ++i) {
rewriter.rewriteList()


Line 107:     catch (AnalysisException e) {
goes after the } in L106


Line 108:       LOG.error("Not able to analyze after rewrite: " + e.toString());
LOG.warn and print the conjuncts in this message


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,
To clean up this retrieval of enable_expr_rewrites, consider adding an createExprRewriter()
function in Analyzer.


http://gerrit.cloudera.org:8080/#/c/6389/3/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 {
I see what you are doing, but I don't think we need to be quite as general yet. The ExprRewriter
framework is relatively new and there are still many missing rules. I'd like to keep it as
simple as possible to add new rules and make modifications to how/where rules are applied.

The downside of the current approach is that rule implementers need to be aware of all the
different uses of the ExprRewriter to pick the set of RuleSets it should be added to.

How about we just keep the two original rule sets? The required ones and then required + all
others. I understand we may sometimes be invoking rules without a hope of them firing.

If excessive rewrites attempts become a perf problem, we can still revisit.


-- 
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: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message