You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2016/09/13 00:09:06 UTC
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into PAGG/AGG IR code
Michael Ho has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4390
Change subject: IMPALA-4008: Don't bake ExprContext pointers into PAGG/AGG IR code
......................................................................
IMPALA-4008: Don't bake ExprContext pointers into PAGG/AGG IR code
To allow genearated code to be shared across multiple fragment
instances, this change removes the ExprContext pointers baked
into the IR of PAGG / AGG code.
Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
---
M be/src/exec/aggregation-node-ir.cc
M be/src/exec/aggregation-node.cc
M be/src/exec/aggregation-node.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M be/src/exprs/agg-fn-evaluator.h
7 files changed, 273 insertions(+), 158 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/4390/1
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#4).
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
IMPALA-4008: Don't bake ExprContext pointers into IR code
To allow genearated code to be shared across multiple fragment
instances, this change removes the ExprContext pointers baked
into various IR functions (e.g. AGG/PAGG/hash-table).
Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-ir.cc
M be/src/exec/aggregation-node.cc
M be/src/exec/aggregation-node.h
M be/src/exec/hash-table-ir.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M be/src/exprs/agg-fn-evaluator.h
13 files changed, 342 insertions(+), 178 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/4390/4
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#3).
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
IMPALA-4008: Don't bake ExprContext pointers into IR code
To allow genearated code to be shared across multiple fragment
instances, this change removes the ExprContext pointers baked
into various IR functions (e.g. AGG/PAGG/hash-table).
Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-ir.cc
M be/src/exec/aggregation-node.cc
M be/src/exec/aggregation-node.h
M be/src/exec/hash-table-ir.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M be/src/exprs/agg-fn-evaluator.h
13 files changed, 345 insertions(+), 182 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/4390/3
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
PS3, Line 584: GetDoublePtrType
> Maybe GetPtrPtrType? So that it's clear that it means T** not double*.
Done
http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:
PS3, Line 164: std::
> will remove std::
Done
PS3, Line 726: ir_fn
> get_expr_ctx_fn_name?
Done
http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:
PS3, Line 418: The expr contexts are passed explicitly to allow reusable
: /// generated code.
> Is this true?
Done
http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:
Line 157: if (evaluator->input_expr_ctxs().size() == 1) {
> Ok, I figured this out. Some functions like group_concat() take additional
Yes, I believe CodegenUpdateSlot() as it stands now only works with only one ExprContext. I don't intend to address this limitation in this patch but I will add a TODO as you suggested.
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into PAGG/AGG IR code
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into PAGG/AGG IR code
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:
Line 84: // Use NULL if the aggregate evaluator is not codegen'ed or if there is no
It might be simpler just to always set to NULL if input_expr_ctxs is empty. We lose some cross-validation but I feel like the simple code would be nice.
http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/aggregation-node.h
File be/src/exec/aggregation-node.h:
Line 82: std::vector<ExprContext*> agg_expr_ctxs_;
Mention that the ExprContexts are owned by aggregate_evaluators_.
http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:
Line 158: ExprContext* agg_expr_ctx = NULL;
Again, I think it would be simpler to just check if it's empty. Or add a method to AggFnEvaluator like HasInputExprContexts()
http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/partitioned-aggregation-node.h
File be/src/exec/partitioned-aggregation-node.h:
Line 202: /// ExprContexts of 'aggregate_evaluators_'. Used in the codegen'ed version of
Also document ownership here.
Line 205: std::vector<ExprContext*> agg_expr_ctxs_;
What do you think about making this vector<pair<AggFnEvaluator*, ExprContext*>>?
Each pair is just a struct with two pointers so should be simple to access from the IR.
This would better document the correspondence between the two vectors and result in one fewer argument being threaded through everywhere.
I'm mostly ok with the current approach but the extra argument is a bit annoying.
http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exprs/agg-fn-evaluator.h
File be/src/exprs/agg-fn-evaluator.h:
Line 192: std::vector<ExprContext*> input_expr_ctxs_;
Can you add your comment here about when this is empty/non-empty?
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:
Line 387: AggFnEvaluator::Add(aggregate_evaluators_, agg_fn_ctxs_, row, tuple);
> why is this change needed? shouldn't fn_ctxs always be the same as &this->
Yes, this is the same as this->agg_fn_ctxs_. The signature was changed for codegen but it has been undone in the latest version of the patch.
http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:
PS5, Line 92: .
nit: long line
http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:
PS5, Line 164: .
nit: long line.
http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/exec/partitioned-aggregation-node.h
File be/src/exec/partitioned-aggregation-node.h:
PS5, Line 478: .
nit: extra full stop.
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/codegen/gen_ir_descriptions.py
File be/src/codegen/gen_ir_descriptions.py:
Line 48: ["AGG_NODE_GET_FN_CTX", "GetAggFnCtx"],
don't these need to include the class name to disambiguate? like Line 57.
http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/exec/aggregation-node.h
File be/src/exec/aggregation-node.h:
PS5, Line 81: Used in the codegen'ed version of UpdateTuple()
so these are pulled out to make it easier for the codegen code to find this thing, right? maybe say that. but also, do we need it. why doesn't GetAggExprCtx() just grab it from the right place?
http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/exec/partitioned-aggregation-node.h
File be/src/exec/partitioned-aggregation-node.h:
Line 206: std::vector<ExprContext*> agg_expr_ctxs_;
same question. why not just make the accessor get it from aggregate_evaluators_?
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4390
to look at the new patch set (#6).
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
IMPALA-4008: Don't bake ExprContext pointers into IR code
To allow genearated code to be shared across multiple fragment
instances, this change removes the ExprContext pointers baked
into various IR functions (e.g. AGG/PAGG/hash-table).
Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-ir.cc
M be/src/exec/aggregation-node.cc
M be/src/exec/aggregation-node.h
M be/src/exec/hash-table-ir.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M be/src/exprs/agg-fn-evaluator.h
13 files changed, 350 insertions(+), 176 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/4390/6
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 8: Code-Review+2
Carry +2 forward.
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 6:
Will rebase before check-in.
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 4:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:
Line 90: // TODO: Fix CodegenUpdateSlot() to handle AggFnEvaluator with > 1 ExprContexts.
is this comment talking about the DCHECK? i.e. is it saying that if we had an aggregator with more than one ExprContexts that we thought we knew how to codgen, then we'd hit this DCHECK? I'm not really sure what this comment is trying to tell me.
Also this code seems to be implying that agg_expr_ctxs_ can only be used by the codgen path, but is that actually stated or enforced somewhere?
Line 387: }
why is this change needed? shouldn't fn_ctxs always be the same as &this->agg_fn_ctxs_?
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/aggregation-node.h
File be/src/exec/aggregation-node.h:
PS4, Line 75: for each agg fn
this is the same correspondence as your new agg_expr_ctxs_, right? So let's word the comment similarly so there's no question about that.
PS4, Line 146: of AggFnEvaluator
this comment sounds like there are many expression contexts per single AggFnEvaluator. clarify it.
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:
PS4, Line 707: PointerType::get
why do we use this directly rather than codegen->GetPtrType()? Let's be consistent one way or the other. (And we don't even need tuple_row_type here).
Line 1061: PointerType* this_ptr_type = PointerType::get(this_type, 0);
same (and looks like we do this a lot in this file -- okay to cleanup as a second step).
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:
PS4, Line 1510: agg_expr_ctx = evaluator->input_expr_ctxs()[0];
how does this make sense? shouldn't we be codegening based on something that's not thread-specific?
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/partitioned-aggregation-node.h
File be/src/exec/partitioned-aggregation-node.h:
PS4, Line 527: of the AggFnEvaluator.
same
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4390/6/be/src/exec/aggregation-node.h
File be/src/exec/aggregation-node.h:
PS6, Line 81: agg_fn_evaluators_
> does this mean to say aggregate_evaluators_?
Done
http://gerrit.cloudera.org:8080/#/c/4390/6/be/src/exec/partitioned-aggregation-node.h
File be/src/exec/partitioned-aggregation-node.h:
PS6, Line 203: agg_fn_evaluators_
> aggregate_evaluators_?
Done
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 6: Code-Review+2
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/codegen/gen_ir_descriptions.py
File be/src/codegen/gen_ir_descriptions.py:
Line 48: ["AGG_NODE_GET_FN_CTX", "GetAggFnCtx"],
> Not really. Even with the class name, the function name is still a substrin
Yeah, but there's no good reason for those to have different method names, right? I think it'd be better to name them consistently and include the class, but since this is the old agg, I don't feel too strongly so you can leave it if you prefer.
http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/exec/aggregation-node.h
File be/src/exec/aggregation-node.h:
PS5, Line 81: avoid loading agg_fn_evaluators_[i] at runtime.
> It would be slightly faster to avoid double-dereferencing (i.e. aggregate_e
So you are saying we avoid loading aggregate_evaluators[i] entirely on the codegen path? if that's the case, then okay.
http://gerrit.cloudera.org:8080/#/c/4390/6/be/src/exec/aggregation-node.h
File be/src/exec/aggregation-node.h:
PS6, Line 81: agg_fn_evaluators_
does this mean to say aggregate_evaluators_?
http://gerrit.cloudera.org:8080/#/c/4390/6/be/src/exec/partitioned-aggregation-node.h
File be/src/exec/partitioned-aggregation-node.h:
PS6, Line 203: agg_fn_evaluators_
aggregate_evaluators_?
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:
Line 90: // operators with no ExprContext (e.g. count(*)). In cases above, 'agg_expr_ctxs_'
> Rephrased the comment. As the code stands now, CodegenUpdateSlot() assumes
How about adding the TODO to the codgen code that is making this assumption?
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:
Line 84: // Use NULL if the aggregate evaluator is not codegen'ed or if there is no
> It might be simpler just to always set to NULL if input_expr_ctxs is empty.
That sounds reasonable. It may be simpler to just check if evaluator->input_expr_ctxs() == 1. That's the case for all aggregate evaluator which we codegen today. We may have more than one expr_ctxs_ for some evaluators (e.g. group_concat) but we don't codegen them.
http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/aggregation-node.h
File be/src/exec/aggregation-node.h:
Line 82: std::vector<ExprContext*> agg_expr_ctxs_;
> Mention that the ExprContexts are owned by aggregate_evaluators_.
Done
http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:
Line 158: ExprContext* agg_expr_ctx = NULL;
> Again, I think it would be simpler to just check if it's empty. Or add a me
Please see replies elsewhere.
http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/partitioned-aggregation-node.h
File be/src/exec/partitioned-aggregation-node.h:
Line 202: /// ExprContexts of 'aggregate_evaluators_'. Used in the codegen'ed version of
> Also document ownership here.
Done
Line 205: std::vector<ExprContext*> agg_expr_ctxs_;
> What do you think about making this vector<pair<AggFnEvaluator*, ExprContex
If I understand your comment correctly, you meant the extra argument in UpdateTuple(), right ? If so, the extra argument is actually to make the IR generation easier.
That said, I added a new cross-compiled function to extract the expr_ctxs from the "this" pointer so we can avoid the extra argument.
http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exprs/agg-fn-evaluator.h
File be/src/exprs/agg-fn-evaluator.h:
Line 192: std::vector<ExprContext*> input_expr_ctxs_;
> Can you add your comment here about when this is empty/non-empty?
Done
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 4: Code-Review+1
We definitely need to fix this agg codegen stuff just for general sanity, but this fix looks good.
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Dan Hecht, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4390
to look at the new patch set (#7).
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
IMPALA-4008: Don't bake ExprContext pointers into IR code
To allow genearated code to be shared across multiple fragment
instances, this change removes the ExprContext pointers baked
into various IR functions (e.g. AGG/PAGG/hash-table).
Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-ir.cc
M be/src/exec/aggregation-node.cc
M be/src/exec/aggregation-node.h
M be/src/exec/hash-table-ir.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M be/src/exprs/agg-fn-evaluator.h
13 files changed, 350 insertions(+), 176 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/4390/7
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:
Line 90: // TODO: Fix CodegenUpdateSlot() to handle AggFnEvaluator with > 1 ExprContexts.
> is this comment talking about the DCHECK? i.e. is it saying that if we had
Rephrased the comment. As the code stands now, CodegenUpdateSlot() assumes that there is only one ExprContext for the AggFnEvaluator and the TODO is to fix CodegenUpdateSlot() to remove this limitation. Once that limitation is lifted, this DCHECK may need to be updated too.
I am not sure if there is a clean way to enforce the agg_expr_ctxs_ are used in the codegen'ed code only. We can add a DCHECK() to that path as we don't seem to cross-compile DCHECK into it but that seems hacky :-).
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/aggregation-node.h
File be/src/exec/aggregation-node.h:
PS4, Line 75: for each agg fn
> this is the same correspondence as your new agg_expr_ctxs_, right? So let's
Done
PS4, Line 146: of AggFnEvaluator
> this comment sounds like there are many expression contexts per single AggF
Done
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:
PS4, Line 707: PointerType::get
> why do we use this directly rather than codegen->GetPtrType()? Let's be co
Yes, we have been quite inconsistent about this in the code historically. We should converge on codegen->GetPtrType().
Line 1061: PointerType* this_ptr_type = PointerType::get(this_type, 0);
> same (and looks like we do this a lot in this file -- okay to cleanup as a
Done
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:
PS4, Line 1510: agg_expr_ctx = evaluator->input_expr_ctxs()[0];
> how does this make sense? shouldn't we be codegening based on something tha
It's only used to extract the Expr out of ExprContext. Expr in general can be shared among threads. Please Note that ComputeFn() returned by Expr::GetCodegendComputeFn() already takes ExprContext* as its argument so the generated IR function is thread safe too.
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/partitioned-aggregation-node.h
File be/src/exec/partitioned-aggregation-node.h:
PS4, Line 527: of the AggFnEvaluator.
> same
Not sure if it implies there are multiple ExprContext here. I tried changing it a little bit.
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
IMPALA-4008: Don't bake ExprContext pointers into IR code
To allow genearated code to be shared across multiple fragment
instances, this change removes the ExprContext pointers baked
into various IR functions (e.g. AGG/PAGG/hash-table).
Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Reviewed-on: http://gerrit.cloudera.org:8080/4390
Reviewed-by: Michael Ho <kw...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-ir.cc
M be/src/exec/aggregation-node.cc
M be/src/exec/aggregation-node.h
M be/src/exec/hash-table-ir.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M be/src/exprs/agg-fn-evaluator.h
13 files changed, 350 insertions(+), 176 deletions(-)
Approvals:
Michael Ho: Looks good to me, approved
Internal Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 8: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:
Line 157: if (evaluator->input_expr_ctxs().size() == 1) {
> I'm really confused by the number of input expr contexts still. Are there a
Ok, I figured this out. Some functions like group_concat() take additional arguments. The only reason why this is related to codegen is that codegen support for UDAs wasn't added. So I think we should think about how this would work for UDAs with > 1 input, or at least add a TODO explaining the limitation.
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/codegen/gen_ir_descriptions.py
File be/src/codegen/gen_ir_descriptions.py:
Line 48: ["AGG_NODE_GET_FN_CTX", "GetAggFnCtx"],
> don't these need to include the class name to disambiguate? like Line 57.
Not really. Even with the class name, the function name is still a substring of the spillable counterpart's name. The differentiation here is the use of short form "Ctx" instead of "Context".
http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:
Line 90: // operators with no ExprContext (e.g. count(*)). In cases above, 'agg_expr_ctxs_'
> How about adding the TODO to the codgen code that is making this assumption
Done
http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/exec/aggregation-node.h
File be/src/exec/aggregation-node.h:
PS5, Line 81: Used in the codegen'ed version of UpdateTuple()
> so these are pulled out to make it easier for the codegen code to find this
It would be slightly faster to avoid double-dereferencing (i.e. aggregate_evaluators_[i]->input_expr_ctxs()[0]). This means one load instruction per AggFnEvaluator instead of two load instructions.
http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/exec/partitioned-aggregation-node.h
File be/src/exec/partitioned-aggregation-node.h:
Line 206: std::vector<ExprContext*> agg_expr_ctxs_;
> same question. why not just make the accessor get it from aggregate_evaluat
Please see the reply elsewhere.
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
PS3, Line 584: GetDoublePtrType
Maybe GetPtrPtrType? So that it's clear that it means T** not double*.
http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:
PS3, Line 726: ir_fn
get_expr_ctx_fn_name?
ir_fn is pretty ambiguous.
Line 1081: Function* get_expr_ctx_fn =
This is much nicer thanks.
http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:
PS3, Line 418: The expr contexts are passed explicitly to allow reusable
: /// generated code.
Is this true?
http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:
Line 157: if (evaluator->input_expr_ctxs().size() == 1) {
I'm really confused by the number of input expr contexts still. Are there any cases where we support aggregate functions with more than one input expr? I couldn't think of any.
Even though it's a bit tangential, it would be good to clarify this now that we're thinking about it.
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4390
to look at the new patch set (#5).
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
IMPALA-4008: Don't bake ExprContext pointers into IR code
To allow genearated code to be shared across multiple fragment
instances, this change removes the ExprContext pointers baked
into various IR functions (e.g. AGG/PAGG/hash-table).
Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-ir.cc
M be/src/exec/aggregation-node.cc
M be/src/exec/aggregation-node.h
M be/src/exec/hash-table-ir.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M be/src/exprs/agg-fn-evaluator.h
13 files changed, 348 insertions(+), 176 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/4390/5
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:
PS3, Line 164: std::
will remove std::
--
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes