Michael Ho 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*. Done http://gerrit.cloudera.org:8080/#/c/4390/3/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: PS3, Line 164: std:: > will remove std:: Done PS3, Line 726: ir_fn > get_expr_ctx_fn_name? Done 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? Done 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) { > Ok, I figured this out. Some functions like group_concat() take additional Yes, I believe CodegenUpdateSlot() as it stands now only works with only one ExprContext. I don't intend to address this limitation in this patch but I will add a TODO as you suggested. -- 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
