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 Sat, 08 Oct 2016 01:58:04 GMT
Michael Ho has posted comments on this change.

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


Patch Set 1:

(3 comments)

FWIW, this patch probably needs to wait for https://gerrit.cloudera.org/#/c/4402. One problem
noticed is that the coordinator will now start remote fragments later due to the time to create
codegen module. With the aforementioned patch Henry is working on, the root fragment will
be started asynchronously together with remote fragments so the bottleneck will be gone. Will
address other comments in the upcoming patch.

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()) {
> I don't think we need to add more functions than the current solution: we n
Please let me know if the approach in the new patch is okay with you.


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();
> Maybe it would make more sense to have the codegen'd exprs owned by the Llv
Certainly, the codegen'ed IR function may make sense to be owned by the codegen object. However,
it seems odd to have to poke the codegen object to get the type of an expression when codegen
is disabled. I will see how the changes to Expr goes before deciding on it.


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 tried. Unfortunately, the test vector seems to override things. I can add a set statement
here but it'll be a no-op.


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

Mime
View raw message