impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()
Date Wed, 12 Oct 2016 06:03:46 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message