Tim Armstrong has posted comments on this change.

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


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS3, Line 584: GetDoublePtrType
Maybe GetPtrPtrType? So that it's clear that it means T** not double*.


http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

PS3, Line 726: ir_fn
get_expr_ctx_fn_name?

ir_fn is pretty ambiguous.


Line 1081:   Function* get_expr_ctx_fn =
This is much nicer thanks.


http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:

PS3, Line 418:  The expr contexts are passed explicitly to allow reusable
             :   /// generated code.
Is this true?


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

Line 157:     if (evaluator->input_expr_ctxs().size() == 1) {
I'm really confused by the number of input expr contexts still. Are there any 
cases where we support aggregate functions with more than one input expr? I 
couldn't think of any. 

Even though it's a bit tangential, it would be good to clarify this now that 
we're thinking about it.


-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to