You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zach Amsden (Code Review)" <ge...@cloudera.org> on 2017/02/01 01:36:23 UTC
[Impala-ASF-CR] IMPALA-4809: Add Stability for codegen constants
Zach Amsden has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5848
Change subject: IMPALA-4809: Add Stability for codegen constants
......................................................................
IMPALA-4809: Add Stability for codegen constants
Having two boolean parameters to a constructor is error prone, so
I thought instead we could express the 'constance' of values by
using an enum class. I chose 'Stability' as a name since one can
imagine extending this to other semi-stable things in a spectrum
between Literal expressed constant, runtime constant, query-local
constant, memoizable constexpr, dynamic variable.
Right now those terms may be overloaded into a single notion in the
sense of is_constant() but they are quite distinct concepts. For
example, CPU flags are RUNTIME_CONSTANT, while query options are
QUERY_CONSTANT.
Testing: Going to run standard test suite. There should be no
side effects at all from this change.
Change-Id: I0431bcd84b908bcf130a6d618f57f8f7c5498428
---
M be/src/exprs/expr.cc
M be/src/exprs/expr.h
M be/src/exprs/literal.cc
M be/src/exprs/null-literal.h
M be/src/exprs/slot-ref.cc
5 files changed, 38 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5848/1
--
To view, visit http://gerrit.cloudera.org:8080/5848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0431bcd84b908bcf130a6d618f57f8f7c5498428
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-4809: Add Stability for codegen constants
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4809: Add Stability for codegen constants
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has abandoned this change.
Change subject: IMPALA-4809: Add Stability for codegen constants
......................................................................
Abandoned
Conflicts with Michael's constant injection refactor.
--
To view, visit http://gerrit.cloudera.org:8080/5848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I0431bcd84b908bcf130a6d618f57f8f7c5498428
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4809: Add Stability for codegen constants
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4809: Add Stability for codegen constants
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5848/1//COMMIT_MSG
Commit Message:
PS1, Line 16: overloaded
From the codegen perspective, we only care if a value is compilation time constant (compilation in the LlvmCodeGen::FinalizeModule()). Not entirely clear to me the benefit of distinguishing between these various types of constant values ? I could be missing something.
PS1, Line 17: is_constant()
I believe this is a concept confined to Expr only. This is a different notion from say other runtime constants such as the constants in a hash table (e.g. stores_duplicates).
http://gerrit.cloudera.org:8080/#/c/5848/1/be/src/exprs/expr.h
File be/src/exprs/expr.h:
PS1, Line 266: RUNTIME_CONSTANT
This seems odd to be in Expr class if an example of this class of constants is CPUInfo's flags.
--
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 <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4809: Add Stability for codegen constants
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4809: Add Stability for codegen constants
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4809: Add Stability for codegen constants
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5848/1//COMMIT_MSG
Commit Message:
PS1, Line 16: overloaded
> From the codegen perspective, we only care if a value is compilation time c
I think this would be useful once we want to do codegen caching and need to reason about whether codegen output is re-usable.
Agree we can solve IMPALA-4809 with Expr::GetConstant() or the mechanism in this patch (if that ever makes it in):
https://gerrit.cloudera.org/#/c/3843/
http://gerrit.cloudera.org:8080/#/c/5848/1/be/src/exprs/expr.h
File be/src/exprs/expr.h:
Line 266: RUNTIME_CONSTANT, // Does not change over an execution
Concretely, does this mean that it doesn't change within the scope of a process - it can be different if you start another daemon or are on a different system?
PS1, Line 266: RUNTIME_CONSTANT
> This seems odd to be in Expr class if an example of this class of constants
I think it would make sense to move this into a thrift enum - see my other comment about doing the analysis in the FE.
Line 267: QUERY_CONSTANT, // Does not change over a single query
We may also want to think about how it varies across different Impala daemons. E.g. does QUERY_CONSTANT mean it's the same on all Impala daemons for that query, or just on the current daemon?
I think that is effectively a different dimension. I don't think we necessarily need to add more complexity, just that we should document the assumptions.
E.g. one thing we've thought about hypothetically is compiling on one daemon then distributing it others.
--
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 <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes