Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4309: Introduce Expr rewrite phase and supporting classes. ......................................................................
Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/4746/4/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: PS4, Line 378: rewriter_.reset(); : analysisResult_.stmt_.rewriteExprs(rewriter_); : reAnalyze = rewriter_.changed(); > Don't really agree. I understand what you're saying. However, as is now, you have a class that keeps track of modifications to other classes it acts upon. So now you have to make sure that you reset the state of the rewriter before you apply it to a different entity. Re #2, I believe there is a way to have each class track modifications to its own state but it will probably require some kind of visitor pattern. Let me think about it and maybe I can suggest something more concrete. http://gerrit.cloudera.org:8080/#/c/4746/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: PS4, Line 60: rewrite > Sorry, missed this. ha, correct. I was thinking of the action. But rewriter is not ambiguous :) -- 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: 4 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: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
