impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into PAGG/AGG IR code
Date Thu, 15 Sep 2016 21:30:02 GMT
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:

File be/src/exec/

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.
File be/src/exec/aggregation-node.h:

Line 82:   std::vector<ExprContext*> agg_expr_ctxs_;
Mention that the ExprContexts are owned by aggregate_evaluators_.
File be/src/exec/

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()
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.
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
To unsubscribe, visit

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

View raw message