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-4008: Don't bake ExprContext pointers into IR code
Date Thu, 22 Sep 2016 20:40:40 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message