[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into PAGG/AGG IR code
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 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 agg_expr_ctxs_; What do you think about making this vector>? 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 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 Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into PAGG/AGG IR code
Michael Ho has uploaded a new change for review. http://gerrit.cloudera.org:8080/4390 Change subject: IMPALA-4008: Don't bake ExprContext pointers into PAGG/AGG IR code .. IMPALA-4008: Don't bake ExprContext pointers into PAGG/AGG IR code To allow genearated code to be shared across multiple fragment instances, this change removes the ExprContext pointers baked into the IR of PAGG / AGG code. Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5 --- M be/src/exec/aggregation-node-ir.cc M be/src/exec/aggregation-node.cc M be/src/exec/aggregation-node.h M be/src/exec/partitioned-aggregation-node-ir.cc M be/src/exec/partitioned-aggregation-node.cc M be/src/exec/partitioned-aggregation-node.h M be/src/exprs/agg-fn-evaluator.h 7 files changed, 273 insertions(+), 158 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/4390/1 -- To view, visit http://gerrit.cloudera.org:8080/4390 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho
