impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4617: remove IsConstant() analysis from be
Date Fri, 06 Jan 2017 22:47:32 GMT
Tim Armstrong 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_
Done


Line 204:   private boolean isConstantCached_;
> The Cached suffix seems weird because this is set during analyze(), i.e. th
Done


Line 262:   public final void analyze(Analyzer analyzer) throws AnalysisException {
> This pattern seems useful and could help with additional cleanup, but it's 
This seemed like the easiest way to enforce the invariant that isAnalyzed_ mean that isConstant_
has a valid value.

If we separate it out into a separate step, then we need to make sure that every Expr subclass
calls analyzeConstness() before calling a different function to set isAnalyzed_.


Line 274:   protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
> The FoldConstantsRule can now be simplified because we had special logic to
I did attempt to do that before but ran into some problems. Unfortunately it was a few weeks
ago so my memory is a little fuzzy.

IIRC we run the rule on exprs that cannot yet be analyzed, I think because they haven't been
substituted so the SlotRefs can't be resolved. The current FoldConstantsRule code avoids analyzing
exprs with SlotRefs. Would be nice to untangle all of that but it seemed like it would get
involved. 

Actually it looks like this is exactly the problem referred to in the FoldConstantsRule class
comment.


Line 286:       analyzer.incrementCallDepth();
> Unfortunate that the increment and decrement are now "disconnected". I was 
I think I can just move the decrement below here. I think I just missed that before.


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 preconditi
Done


Line 975:    * @return true if this expression should be treated as constant within the scope
> Simplify comment:
I think it's confusing without some clarification of what the scope of the constness is. E.g.
now() is considered constant but every evaluation does not return the same value always.

I added some examples to clarify this so it's a bit less abstract.


-- 
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message