Tim Armstrong has posted comments on this change. Change subject: IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen() ......................................................................
Patch Set 2: (4 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; > Please let me know if the approach in the new patch is okay with you. I think we should still avoid putting the AddCodegenMsg in all of the Prepare() functions. We can do that easily if you have ExecNode::CodegenTree() that does roughly: if (state->codegen_enabled()) { Codegen(); } else { AddCodegenMsg(false, "by DISABLE_CODEGEN query option"); } for (ExecNode* child : children_) { CodegenTree(child); } I think that more clearly expresses the logic and requires less code spread around all the exec nodes. I feel less strongly about whether RuntimeState should be an argument or not in this staging patch. It shouldn't be in the end state I don't think. 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 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 it via 'this') 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: create function if not exists udf_test_errors.nine_args(int, int, int, int, int, int, int, int, int) returns int > I tried. Unfortunately, the test vector seems to override things. I can add Hmm, weird. Maybe add a comment in the .py file then to explain why its disabling codegen. -- 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 <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
