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 <k...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to