impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()
Date Mon, 10 Oct 2016 16:40:56 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()

Patch Set 2:

File be/src/exec/

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()) {
  } else {
    AddCodegenMsg(false, "by DISABLE_CODEGEN query option");
  for (ExecNode* child : children_) {

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.
File be/src/exec/

Line 632:       Status status = build_expr_ctxs_[i]->root()->GetCodegendComputeFn(codegen,
Long line I think
File be/src/exec/

PS2, Line 294: CodegenProcessBatchStreaming
Let's fix these to thread the codegen argument through (instead of getting it via 'this')
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I207566bc9f4c6a159271ecdbc4bbdba3d78c6651
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message