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 Mon, 26 Sep 2016 18:54:56 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code

Patch Set 5:

File be/src/codegen/

Line 48:   ["AGG_NODE_GET_FN_CTX", "GetAggFnCtx"],
> don't these need to include the class name to disambiguate? like Line 57.
Not really. Even with the class name, the function name is still a substring of the spillable
counterpart's name. The differentiation here is the use of short form "Ctx" instead of "Context".
File be/src/exec/

Line 90:       // operators with no ExprContext (e.g. count(*)). In cases above, 'agg_expr_ctxs_'
> How about adding the TODO to the codgen code that is making this assumption
File be/src/exec/aggregation-node.h:

PS5, Line 81: Used in the codegen'ed version of UpdateTuple()
> so these are pulled out to make it easier for the codegen code to find this
It would be slightly faster to avoid double-dereferencing (i.e. aggregate_evaluators_[i]->input_expr_ctxs()[0]).
This means one load instruction per AggFnEvaluator instead of two load instructions.
File be/src/exec/partitioned-aggregation-node.h:

Line 206:   std::vector<ExprContext*> agg_expr_ctxs_;
> same question. why not just make the accessor get it from aggregate_evaluat
Please see the reply elsewhere.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message