Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-4309: Introduce Expr rewrite phase and supporting classes. ......................................................................
Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: PS2, Line 68: List<ExprRewriteRule> rules = Lists.newArrayList(); : rules.add(BetweenToCompoundRule.INSTANCE) I think these lines can be combined into one, Lists.newArrayList(BetweenToCompoundRule.INSTANCE) unless it was written this way for readability. PS2, Line 327: rewriteSubqueries IMO, the new method name is a little misleading given it is not actually performing the subquery rewrite. May be requiresSubqueryRewrite() or something similar? Same for rewriteExprs() http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: PS2, Line 1037: if (conjunct instanceof BetweenPredicate) { : conjunct = BetweenToCompoundRule.INSTANCE.rewrite(conjunct, this); : } Just curious why this has been added. Shouldn't it work even without it? http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java File fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java: Line 50: public void analyze(Analyzer analyzer) throws AnalysisException { so nice to read the cleaned-up version :) http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/SelectList.java File fe/src/main/java/org/apache/impala/analysis/SelectList.java: Line 88: @Override for consistency http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java: Line 206: stmt.whereClause_ = BetweenToCompoundRule.INSTANCE.rewrite(stmt.whereClause_, analyzer); long line. http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/TableRef.java File fe/src/main/java/org/apache/impala/analysis/TableRef.java: Line 513: @Override for consistency. http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java File fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java: Line 36: numChanges_ = 0; This doesn't seem to be thread safe. Given we are using a static INSTANCE, does it cause any issues while accounting numChanges_ with multiple analyzer instances using the same rule object? -- To view, visit http://gerrit.cloudera.org:8080/4746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2279dc984bcf7742db4fa3b1aa67283ecbb05e6e Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
