Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201610212203.u9LM3Wcb018748@ip-10-146-233-104.ec2.internal>
Date: Fri, 21 Oct 2016 22:03:32 +0000
From: "Dimitris Tsirogiannis (Code Review)" <gerrit@cloudera.org>
To: Alex Behm <alex.behm@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Tim Armstrong <tarmstrong@cloudera.com>,
        Bharath Vissapragada <bharathv@cloudera.com>
Reply-To: dtsirogiannis@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4309=3A_Introduce_Expr_rewrite_phase_and_supporting_classes=2E=0A?=
In-Reply-To: <gerrit.1476767303000.I2279dc984bcf7742db4fa3b1aa67283ecbb05e6e@gerrit.cloudera.org>
References: <gerrit.1476767303000.I2279dc984bcf7742db4fa3b1aa67283ecbb05e6e@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Fri, 21 Oct 2016 22:03:41 -0000

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.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes