Tim Armstrong has posted comments on this change. Change subject: IMPALA-4008: Don't bake ExprContext pointers into PAGG/AGG IR code ......................................................................
Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/aggregation-node.cc File be/src/exec/aggregation-node.cc: Line 84: // Use NULL if the aggregate evaluator is not codegen'ed or if there is no It might be simpler just to always set to NULL if input_expr_ctxs is empty. We lose some cross-validation but I feel like the simple code would be nice. http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/aggregation-node.h File be/src/exec/aggregation-node.h: Line 82: std::vector<ExprContext*> agg_expr_ctxs_; Mention that the ExprContexts are owned by aggregate_evaluators_. http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: Line 158: ExprContext* agg_expr_ctx = NULL; Again, I think it would be simpler to just check if it's empty. Or add a method to AggFnEvaluator like HasInputExprContexts() http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exec/partitioned-aggregation-node.h File be/src/exec/partitioned-aggregation-node.h: Line 202: /// ExprContexts of 'aggregate_evaluators_'. Used in the codegen'ed version of Also document ownership here. Line 205: std::vector<ExprContext*> agg_expr_ctxs_; What do you think about making this vector<pair<AggFnEvaluator*, ExprContext*>>? Each pair is just a struct with two pointers so should be simple to access from the IR. This would better document the correspondence between the two vectors and result in one fewer argument being threaded through everywhere. I'm mostly ok with the current approach but the extra argument is a bit annoying. http://gerrit.cloudera.org:8080/#/c/4390/2/be/src/exprs/agg-fn-evaluator.h File be/src/exprs/agg-fn-evaluator.h: Line 192: std::vector<ExprContext*> input_expr_ctxs_; Can you add your comment here about when this is empty/non-empty? -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
