Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
Patch Set 4:
Line 90: // TODO: Fix CodegenUpdateSlot() to handle AggFnEvaluator with >
> 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 :-).
PS4, Line 75: for each agg fn
> this is the same correspondence as your new agg_expr_ctxs_, right? So let's
PS4, Line 146: of AggFnEvaluator
> this comment sounds like there are many expression contexts per single AggF
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
PS4, Line 1510: agg_expr_ctx = evaluator->input_expr_ctxs();
> 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.
PS4, Line 527: of the AggFnEvaluator.
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-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>