[Impala-ASF-CR] IMPALA-4617: remove IsConstant() analysis from be

["Alex Behm (Code Review)" <ge...@cloudera.org>]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4617: remove IsConstant() analysis from be
......................................................................


Patch Set 5:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5415/5/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

Line 181:   private boolean isAnalyzed_ = false;
let's move this below fn_


Line 204:   private boolean isConstantCached_;
The Cached suffix seems weird because this is set during analyze(), i.e. this member is no different than other members set during analyze() that don't have "Cached".

Let's just call this isConstant_


Line 262:   public final void analyze(Analyzer analyzer) throws AnalysisException {
This pattern seems useful and could help with additional cleanup, but it's not clear what analysisDone() actually does (from the function name). I'd prefer to have a separate analyzeConstness() to set the isConstant_. That way it is clear that the new function is called during analysis, and we can add other analyzeeThisOrThat() that can be overridden independently.

The other thing that analysisDone() does is set isAnalyzed_ which I think can just be set directly in analyze().


Line 274:   protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
The FoldConstantsRule can now be simplified because we had special logic to avoid repeated expr traversals due to isConstant(). We can only fold analyzed exprs so we can simplify the logic.


Line 286:       analyzer.incrementCallDepth();
Unfortunate that the increment and decrement are now "disconnected". I was hoping we might be able to move this into analyze() above, but I see that ExtractExpr needs some special logic there. Maybe you have an idea how it can easily be cleaned up (this patch is already an improvement and we should not cram in too much).


Line 539:     msg.is_constant = isConstantCached_; // Valid because expr is analyzed.
no need for comment imo, it's produced by analyze() and that's a precondition of this function


Line 975:    * @return true if this expression should be treated as constant within the scope
Simplify comment:

"Returns true if every evaluation of this expr returns the same value."

Clarify that this function may incorrectly return false if the expr is not analyzed.


-- 
To view, visit http://gerrit.cloudera.org:8080/5415
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I24b4c9d6e8e57fd73d906a9c36200e1a84497b90
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes