You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Paul Rogers (Code Review)" <ge...@cloudera.org> on 2019/02/12 00:20:43 UTC
[Impala-ASF-CR] IMPALA-8182: Add single-node plan to PlanCtx
Paul Rogers has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12436
Change subject: IMPALA-8182: Add single-node plan to PlanCtx
......................................................................
IMPALA-8182: Add single-node plan to PlanCtx
Historically, the Impala planner is a function: AST in one end, Thrift
plan out the other. The planner itself creates a rich intermediate plan
representation, but none of those objects were available for testing,
forcing all tests, no matter how detailed, to run as end-to-end tests:
SQL in one end, verify a (very abbreviated) DESCRIBE output out the
other.
A recent change introduced the PlanCtx that provided access to the
parallelized plan fragments. Doing so allowed adding a variety of
cardinality tests.
It turns out that additional testing requires access to the "single node
plan" in order to verify things like join cardinality.
This ticket asks to modify the PlanCtx to capture all the internal plan
nodes: both single node and distributed, to enable full unit testing.
Tests: This fix is refactoring only. Reran all FE tests.
Change-Id: I983473f259df8af17d8dd38e497fe33e839cd0dc
---
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
3 files changed, 37 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/12436/1
--
To view, visit http://gerrit.cloudera.org:8080/12436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I983473f259df8af17d8dd38e497fe33e839cd0dc
Gerrit-Change-Number: 12436
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-8182: Add single-node plan to PlanCtx
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12436 )
Change subject: IMPALA-8182: Add single-node plan to PlanCtx
......................................................................
Patch Set 1:
A simple one: just providing access to single plan nodes for testing.
Pre-review tests passed: https://jenkins.impala.io/job/pre-review-test/301/
--
To view, visit http://gerrit.cloudera.org:8080/12436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983473f259df8af17d8dd38e497fe33e839cd0dc
Gerrit-Change-Number: 12436
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 00:21:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8182: Add single-node plan to PlanCtx
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12436 )
Change subject: IMPALA-8182: Add single-node plan to PlanCtx
......................................................................
Patch Set 1:
(6 comments)
Few minor comments, lgtm otherwise.
http://gerrit.cloudera.org:8080/#/c/12436/1/fe/src/main/java/org/apache/impala/planner/Planner.java
File fe/src/main/java/org/apache/impala/planner/Planner.java:
http://gerrit.cloudera.org:8080/#/c/12436/1/fe/src/main/java/org/apache/impala/planner/Planner.java@66
PS1, Line 66: public static final class QueryPlan {
Comment.
http://gerrit.cloudera.org:8080/#/c/12436/1/fe/src/main/java/org/apache/impala/planner/Planner.java@99
PS1, Line 99: Returns a list of plan fragments for executing an analyzed parse tree.
: * May return a single-node or distributed executable plan. If enabled (through a
: * query option), computes runtime filters for dynamic partition pruning.
Update
http://gerrit.cloudera.org:8080/#/c/12436/1/fe/src/main/java/org/apache/impala/planner/Planner.java@141
PS1, Line 141: plan.rootFragment_ = rootFragment;
Set this after L179 instead of doing it twice.
http://gerrit.cloudera.org:8080/#/c/12436/1/fe/src/main/java/org/apache/impala/planner/Planner.java@244
PS1, Line 244: Return a list of plans
Update.
http://gerrit.cloudera.org:8080/#/c/12436/1/fe/src/main/java/org/apache/impala/planner/Planner.java@258
PS1, Line 258: plan.parallelPlans_ = parallelPlans;
Do it in L253?
http://gerrit.cloudera.org:8080/#/c/12436/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/12436/1/fe/src/main/java/org/apache/impala/service/Frontend.java@212
PS1, Line 212: public List<PlanFragment> getPlan() { return planRoots_; }
Looks like there are very limited callers for this. Redo them to use the new way?
--
To view, visit http://gerrit.cloudera.org:8080/12436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983473f259df8af17d8dd38e497fe33e839cd0dc
Gerrit-Change-Number: 12436
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 19:42:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8182: Add single-node plan to PlanCtx
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12436
to look at the new patch set (#2).
Change subject: IMPALA-8182: Add single-node plan to PlanCtx
......................................................................
IMPALA-8182: Add single-node plan to PlanCtx
Historically, the Impala planner is a function: AST in one end, Thrift
plan out the other. The planner itself creates a rich intermediate plan
representation, but none of those objects were available for testing,
forcing all tests, no matter how detailed, to run as end-to-end tests:
SQL in one end, verify a (very abbreviated) DESCRIBE output out the
other.
A recent change introduced the PlanCtx that provided access to the
parallelized plan fragments. Doing so allowed adding a variety of
cardinality tests.
It turns out that additional testing requires access to the "single node
plan" in order to verify things like join cardinality.
This patch generalizes the previous patch to provide access to all
intermediate plan structures created during planning. Doing so enables
full unit testing of all planning steps.
Tests: This fix is refactoring only. Reran all FE tests.
Change-Id: I983473f259df8af17d8dd38e497fe33e839cd0dc
---
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
3 files changed, 66 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/12436/2
--
To view, visit http://gerrit.cloudera.org:8080/12436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983473f259df8af17d8dd38e497fe33e839cd0dc
Gerrit-Change-Number: 12436
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-8182: Add single-node plan to PlanCtx
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12436 )
Change subject: IMPALA-8182: Add single-node plan to PlanCtx
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12436/2/fe/src/main/java/org/apache/impala/planner/Planner.java
File fe/src/main/java/org/apache/impala/planner/Planner.java:
http://gerrit.cloudera.org:8080/#/c/12436/2/fe/src/main/java/org/apache/impala/planner/Planner.java@89
PS2, Line 89: public List<PlanFragment> finalPlan() { return finalPlan_; }
this is not the final plan when mt_dop is set? (createParallelPlan())?
--
To view, visit http://gerrit.cloudera.org:8080/12436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983473f259df8af17d8dd38e497fe33e839cd0dc
Gerrit-Change-Number: 12436
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Feb 2019 00:50:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8182: Add single-node plan to PlanCtx
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has abandoned this change. ( http://gerrit.cloudera.org:8080/12436 )
Change subject: IMPALA-8182: Add single-node plan to PlanCtx
......................................................................
Abandoned
Will do as part of an actual test later.
--
To view, visit http://gerrit.cloudera.org:8080/12436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I983473f259df8af17d8dd38e497fe33e839cd0dc
Gerrit-Change-Number: 12436
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-8182: Add single-node plan to PlanCtx
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12436 )
Change subject: IMPALA-8182: Add single-node plan to PlanCtx
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2075/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983473f259df8af17d8dd38e497fe33e839cd0dc
Gerrit-Change-Number: 12436
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 00:54:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8182: Add single-node plan to PlanCtx
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12436 )
Change subject: IMPALA-8182: Add single-node plan to PlanCtx
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/2110/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/12436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983473f259df8af17d8dd38e497fe33e839cd0dc
Gerrit-Change-Number: 12436
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Feb 2019 22:35:47 +0000
Gerrit-HasComments: No