impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4586: don't constant fold in backend
Date Wed, 07 Dec 2016 19:38:46 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4586: don't constant fold in backend
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5391/6//COMMIT_MSG
Commit Message:

PS6, Line 26: In a future change we should remove the IsConstant()
            : analysis logic from the backend entirely and pass the information from
            : the frontend.
> I think we'd keep this patch, since we'd still only want to cache literals.
hmm, not sure I understand that.  if the frontend gets the right definition for isConstant()
and only does safe optimizes based on that, why shouldn't the backend also optimize those
cases? i.e. if isConstant() is correctly computed by the frontend and used to trigger this
backend optimization, then isn't this backend optimization safe?


http://gerrit.cloudera.org:8080/#/c/5391/6/fe/src/main/java/org/apache/impala/service/FeSupport.java
File fe/src/main/java/org/apache/impala/service/FeSupport.java:

Line 124:     Preconditions.checkState(!expr.contains(SlotRef.class));
> I'm just trying to reduce the number of places we conflate the "isConstant(
I'm fine with it. I just wanted to confirm your intent, and it sounds like we're on the same
page.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0c76e3c8a8d92749256c312080ecd7aac5d99ce7
Gerrit-PatchSet: 6
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