impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
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:

(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.
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.


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_.
Done


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 me
Please see replies elsewhere.


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.
Done


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.


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?
Done


-- 
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 <kwho@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message