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
