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-4809: Add Stability for codegen constants
Date Wed, 01 Feb 2017 04:14:45 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4809: Add Stability for codegen constants
......................................................................


Patch Set 1:

IMPALA-4809 isn't about inlining an expression related value, it's just that the way the code
is currently factored, Expr routines deal with doing codegen tricks involving FunctionContext's.
 They really don't belong in Expr in the first place.  Michael is in the process of cleaning
that up.  So, let's not make IMPALA-4809 dependent on this change.

I agree a concept like this may be useful and needed in the future, but until we finish cleaning
up some surrounding code and have a need for this, I don't think we should introduce this
new concept (and it seems more of a codegen related concept than an Expr related concept).

As an aside, see Codegen::ReplaceCallSites() and other related code for non-expr places we
do constant injection.  That's another way you could do 4809, but ultimately you'll need to
get at the query options through FunctionContext anyway.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0431bcd84b908bcf130a6d618f57f8f7c5498428
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <zamsden@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: No

Mime
View raw message