Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201706270005.v5R053Ul026610@ip-10-146-233-104.ec2.internal>
Date: Tue, 27 Jun 2017 00:05:03 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Alex Behm <alex.behm@cloudera.com>, Juan Yu <jyu@cloudera.com>,
        Michael Ho <kwho@cloudera.com>,
        Mostafa Mokhtar <mmokhtar@cloudera.com>
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5483=3A_Automatically_disable_codegen_for_small_queries=0A?=
In-Reply-To: <gerrit.1497291618000.I273bcee58641f5b97de52c0b2caab043c914b32e@gerrit.cloudera.org>
References: <gerrit.1497291618000.I273bcee58641f5b97de52c0b2caab043c914b32e@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.7
archived-at: Tue, 27 Jun 2017 00:05:09 -0000

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5483: Automatically disable codegen for small queries
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7153/6/testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test
File testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test:

Line 6: Planner Hints: Disable codegen
> The term "hint" already has a different meaning as in "straight_join" kind 
As mentioned in the other comment the codegen hint isn't equivalent to the query option. It seems a bit questionable that with the other query options we can't distinguish between user-set options and planner-set options in the profile (although you can usually infer for single node plan).

Disabling codegen does seem like a significant enough planner decision that it should be somewhat visible in the explain plan. It's a bit less of a problem for the single node optimisation since then there's usually a visible change to the plan shape. We can test it like testComputeStatsMtDop() but if there's nothing in the explain plan it's harder to understand what the planner did.

"Planner Hint" might not be the best term if we do include it. Not sure what would be better. Just a plain string like "Codegen disabled by planner"?


http://gerrit.cloudera.org:8080/#/c/7153/3/testdata/workloads/functional-query/queries/QueryTest/codegen.test
File testdata/workloads/functional-query/queries/QueryTest/codegen.test:

Line 2: ---- QUERY
> The hint is applied to all FE->BE expr evaluations, and the single-node opt
I think we have coverage that the query produces the expected results when the hint is set, but we don't have any coverage that codegen was actually disabled.

The distinction between the hint and the query option exists for a reason - there are some exprs that can only be evaluated with codegen enabled. In those cases the hint is overridden. But if the query option is set explicitly, then the option takes precedence and the query fails because the exprs can't be evaluated.

b7eeb8bf85ef24b730c9ba2891f5a8075ba9605e added some coverage of that interaction - it runs test_udfs with exec_single_node_rows_threshold set to a couple of different values, which exercises that case.


-- 
To view, visit http://gerrit.cloudera.org:8080/7153
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I273bcee58641f5b97de52c0b2caab043c914b32e
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes