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 <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
