Michael Ho has posted comments on this change. Change subject: IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen() ......................................................................
Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/4651/1/be/src/exec/aggregation-node.cc File be/src/exec/aggregation-node.cc: Line 174: bool codegen_enabled = false; > I think we should still avoid putting the AddCodegenMsg in all of the Prepa I thought about something similar but this has the side effect of putting "Codegen disabled: ...." to all exec nodes when codegen is disabled.This may be confusing for non-codegen capable operators. As mentioned before, the current plan is to populate the pointers to JIT compiled functions in Prepare() once the infrastructure for sharing generated code is in place so we may need AddCodegenMsg() there in case codegen is disabled or codegen fails (in which case the pointers are null). http://gerrit.cloudera.org:8080/#/c/4651/2/be/src/exec/aggregation-node.cc File be/src/exec/aggregation-node.cc: PS2, Line 168: false > Can you add more info, e.g. "by DISABLE_CODEGEN query option". Done http://gerrit.cloudera.org:8080/#/c/4651/2/be/src/exec/hash-join-node.cc File be/src/exec/hash-join-node.cc: Line 147: runtime_profile()->AddCodegenMsg(false, "", "Build Side"); > I was thinking it simplifies things if we just have Done http://gerrit.cloudera.org:8080/#/c/4651/1/be/src/exec/old-hash-table.cc File be/src/exec/old-hash-table.cc: Line 419: LlvmCodeGen* codegen = state->codegen(); > Certainly, the codegen'ed IR function may make sense to be owned by the cod On a second thought, we may actually pass in the Expr* directly so may as well be passing in the CodeGen object only. This function may eventually become static too. http://gerrit.cloudera.org:8080/#/c/4651/2/be/src/exec/old-hash-table.cc File be/src/exec/old-hash-table.cc: Line 632: Status status = build_expr_ctxs_[i]->root()->GetCodegendComputeFn(codegen, &expr_fn); > Long line I think Done http://gerrit.cloudera.org:8080/#/c/4651/2/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: PS2, Line 294: CodegenProcessBatchStreaming > Let's fix these to thread the codegen argument through (instead of getting Done here and other places too. http://gerrit.cloudera.org:8080/#/c/4651/2/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: Line 552: switch (children_.size()) { > I feel like 8 is low enough that people might realistically hit it. Maybe w Done Line 603: DCHECK(false) << "Interpreted path not implemented. We should have " > Update this dcheck message, it's referring to the old auto-enable thing Done Line 667: DCHECK(false) << "Interpreted path not implemented. We should have " > Update this dcheck message, it's referring to the old auto-enable thing Done -- To view, visit http://gerrit.cloudera.org:8080/4651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I207566bc9f4c6a159271ecdbc4bbdba3d78c6651 Gerrit-PatchSet: 2 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
