Dan Hecht 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 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
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.
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
PS4, Line 1510: agg_expr_ctx = evaluator->input_expr_ctxs();
how does this make sense? shouldn't we be codegening based on something that's
PS4, Line 527: of the AggFnEvaluator.
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>