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 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:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/codegen/gen_ir_descriptions.py
File be/src/codegen/gen_ir_descriptions.py:

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


http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:

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
Done


http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/exec/aggregation-node.h
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.


http://gerrit.cloudera.org:8080/#/c/4390/5/be/src/exec/partitioned-aggregation-node.h
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 http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message