impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Date Sat, 17 Sep 2016 01:08:39 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-4008: Don't bake ExprContext pointers into 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.
That sounds reasonable. It may be simpler to just check if evaluator->input_expr_ctxs()
== 1. That's the case for all aggregate evaluator which we codegen today. We may have more
than one expr_ctxs_ for some evaluators (e.g. group_concat) but we don't codegen them.
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 me
Please see replies elsewhere.
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*, ExprContex
If I understand your comment correctly, you meant the extra argument in UpdateTuple(), right
? If so, the extra argument is actually to make the IR generation easier. 

That said, I added a new cross-compiled function to extract the expr_ctxs from the "this"
pointer so we can avoid the extra argument.
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: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message