impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4809: Add Stability for codegen constants
Date Wed, 01 Feb 2017 02:39:18 GMT
Michael Ho has posted comments on this change.

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


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5848/1//COMMIT_MSG
Commit Message:

PS1, Line 16: overloaded
> I think this would be useful once we want to do codegen caching and need to
Good point about its usability for codegen caching but it seems a bit premature at this point
IMHO.


http://gerrit.cloudera.org:8080/#/c/5848/1/be/src/exprs/expr.h
File be/src/exprs/expr.h:

PS1, Line 266: RUNTIME_CONSTANT
> I think it would make sense to move this into a thrift enum - see my other 
If this is done solely for codegen, it may make sense to move this to codegen related header.
It appears to me the type of constant we are talking about is more like CpuInfo::IsSupported(CpuInfo::SSE4_2)
in cross-compiled code.


-- 
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: Yes

Mime
View raw message