Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201610080158.u981w4uS021604@ip-10-146-233-104.ec2.internal>
Date: Sat, 8 Oct 2016 01:58:04 +0000
From: "Michael Ho (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: kwho@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4080=2C_IMPALA-3638=3A_Introduce_ExecNode=3A=3ACodegen=28=29=0A?=
In-Reply-To: <gerrit.1475780751000.I207566bc9f4c6a159271ecdbc4bbdba3d78c6651@gerrit.cloudera.org>
References: <gerrit.1475780751000.I207566bc9f4c6a159271ecdbc4bbdba3d78c6651@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Sat, 08 Oct 2016 01:58:10 -0000

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