impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
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:

(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 <kwho@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message