impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4008: Don't bake ExprContext pointers into IR code
Date Mon, 26 Sep 2016 17:56:10 GMT
Dan Hecht has posted comments on this change.

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


Patch Set 5:

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


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 thing, right?
maybe say that.  but also, do we need it. why doesn't GetAggExprCtx() just grab it from the
right place?


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


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