Tim Armstrong has posted comments on this change. Change subject: IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen() ......................................................................
Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/4651/1//COMMIT_MSG Commit Message: PS1, Line 7: IMPALA-3686 > I am not 100% sure the extent which IMPALA-3638 is trying to cover so I am Treating it as a separate issue sounds good to me. http://gerrit.cloudera.org:8080/#/c/4651/1/be/src/exec/aggregation-node.cc File be/src/exec/aggregation-node.cc: Line 174: if (state->codegen_enabled()) { > Don't want to introduce another function per exec node just for that. I upd I don't think we need to add more functions than the current solution: we need a generic tree-walking function in ExecNode and a specialised Codegen() function in each subclass. It's probably less code since each CodegenFunction() doesn't need to check if codegen is enabled or call ExecNode::Codegen() http://gerrit.cloudera.org:8080/#/c/4651/2/be/src/exec/aggregation-node.cc File be/src/exec/aggregation-node.cc: PS2, Line 168: Can you add more info, e.g. "by DISABLE_CODEGEN query option". 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: } I was thinking it simplifies things if we just have "Codegen Disabled: by DISABLE_CODEGEN query option" instead of enumerating the different types of codegen that were enabled. 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(); > Not so sure about this yet as we may need RuntimeState for accessing Expr i Maybe it would make more sense to have the codegen'd exprs owned by the LlvmCodegen object? We have to enforce a 1:1 relationship between the codegen'd exprs and the LLVM module anyway, so this might make it more explicit. http://gerrit.cloudera.org:8080/#/c/4651/1/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: Line 136: // TODO: don't do this for child exprs > Not sure what that means ? Does it mean we shouldn't codegen child exprs ? I think the problem is that it's generating function pointers for all the child exprs that won't be used, so it can't internalise the functions in the IR module. 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: case 0: I feel like 8 is low enough that people might realistically hit it. Maybe we should just stamp out the variants up to say 20? I used the BOOST_PP_REPEAT_FROM_TO macro here to accomplish something similar https://gerrit.cloudera.org/#/c/4494/7/be/src/util/bit-packing.inline.h Line 603: << "codegen'd the wrapper"; Update this dcheck message, it's referring to the old auto-enable thing Line 667: << "codegen'd the wrapper"; Update this dcheck message, it's referring to the old auto-enable thing http://gerrit.cloudera.org:8080/#/c/4651/2/testdata/workloads/functional-query/queries/QueryTest/udf-errors.test File testdata/workloads/functional-query/queries/QueryTest/udf-errors.test: Line 28: ---- CATCH Can we just add the set disable_codegen=1 flag before each of these queries? I think that would make it clearer what's happening. -- 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: 1 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
