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 2018/11/11 21:15:58 UTC

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing 

Paul Rogers has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11920


Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................

IMPALA-7842: Make query fragments available for unit testing

Today, the FrontEnd class uses a functional model to generate a plan:
pass in SQL text to the createExecRequest() method and get back a Thrift
plan ready for serialization.

For unit testing, however, we need access to the intermediate plan
fragments produced by the planner. Inspecting only the Thrift version
results in loss of important details.

This fix introduces a new planner context (PlanCtx) class that passes
information into the planner, and can pass the plan fragments back out.
There is no functional change, just a refactoring of the existing code.

The changes also introduces a new test case that uses this feature to
verify plan cardinality.

Tests: The new test case demonstrates the functionality.

Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
2 files changed, 207 insertions(+), 15 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/11920/1
-- 
To view, visit http://gerrit.cloudera.org:8080/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Philip Zeyliger, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11920

to look at the new patch set (#6).

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................

IMPALA-7842: Expose physical plan for unit testing

The FrontEnd class uses a functional model to generate a plan: pass
in SQL text to the createExecRequest() method and get back a Thrift
plan ready for serialization.

For unit testing, however, we need access to the intermediate physical
plan tree produced by the planner. Inspecting only the Thrift version
results in loss of important details.

This fix introduces a new planner context (PlanCtx) class that passes
information into the planner, and passses the physical plan back out.
Code that used the prior version (pass in a query context and explain
string) are changed to use the new form.

Testing:
* There is no functional change, just a refactoring of the existing
  code. Ran all FE test to verify no regressions.
* Introduces a new test case that uses this feature to verify plan
  cardinality.

Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
5 files changed, 258 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/11920/6
-- 
To view, visit http://gerrit.cloudera.org:8080/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................


Patch Set 4:

(9 comments)

Refactor looks fine to me. Just some nits.

http://gerrit.cloudera.org:8080/#/c/11920/4/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/11920/4/fe/src/main/java/org/apache/impala/service/Frontend.java@181
PS4, Line 181: <p>
remove


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/Frontend.java@186
PS4, Line 186: protected final TQueryCtx queryCtx_;
             :     protected final StringBuilder explainBuf_;
             :     protected boolean capturePlan_;
             :     protected List<PlanFragment> plan_;
Please add one-liner docs for these.


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/Frontend.java@217
PS4, Line 217:       return explainBuf_.toString();
nit: single line


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/Frontend.java@1220
PS4, Line 1220: TQueryCtx
?


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/Frontend.java@1221
PS4, Line 1221: This is the most general version. Populates the EXPLAIN string if
              :    * a buffer is provided. Returns the internal plan tree if requested.
              :    *
              :    * Retained for legacy use. New code should use
              :    * {@link #createExecRequest(PlanCtx)}.
Not sure if this applies given the current signature change.


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java@171
PS4, Line 171:  
nit: remove space


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
File fe/src/test/java/org/apache/impala/planner/CardinalityTest.java:

http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@36
PS4, Line 36: PlannerTestBase
Do we need to rebase this on top of the Analysis/Query fixture changes?


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@111
PS4, Line 111:   protected void verifyCardinality(String query, long expected) {
method doc


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@119
PS4, Line 119: // Set up the query context. Note that we need to deep copy it before
             :     // planning each time since planning modifies it.
Not sure I got this, where is the deep copy?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 03 Dec 2018 18:56:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................


Patch Set 1:

(2 comments)

Structure seems fine to me. I thought about changing the return type or adding methods, but this doesn't seem worse, and is consistent with what we're already doing with explain.

http://gerrit.cloudera.org:8080/#/c/11920/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/11920/1/fe/src/main/java/org/apache/impala/service/Frontend.java@188
PS1, Line 188:     public TQueryCtx queryCtx_;
             :     public StringBuilder explainBuf_;
             :     public boolean capturePlan_;
             :     public List<PlanFragment> plan_;
Are these intentionally public?


http://gerrit.cloudera.org:8080/#/c/11920/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1227
PS1, Line 1227:    * Create a populated TExecRequest corresponding to the supplied TQueryCtx.
This should be on line 1220?

There are now two public entrypoints here; document which one you expect for which purposes?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Nov 2018 21:44:39 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................


Patch Set 2:

(2 comments)

Thanks, Phil, for the review. Addressed your comments.

http://gerrit.cloudera.org:8080/#/c/11920/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/11920/1/fe/src/main/java/org/apache/impala/service/Frontend.java@188
PS1, Line 188:     protected final TQueryCtx queryCtx_;
             :     protected final StringBuilder explainBuf_;
             :     protected boolean capturePlan_;
             :     protected List<PlanFragment> pla
> Are these intentionally public?
Made these protected, some final.


http://gerrit.cloudera.org:8080/#/c/11920/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1227
PS1, Line 1227:       StringBuilder explainString)
> This should be on line 1220?
There are actually three public entry points. Retained the older two to avoid changing more code than necessary. Can clean that up later.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Nov 2018 21:41:28 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11920

to look at the new patch set (#2).

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................

IMPALA-7842: Make query fragments available for unit testing

Today, the FrontEnd class uses a functional model to generate a plan:
pass in SQL text to the createExecRequest() method and get back a Thrift
plan ready for serialization.

For unit testing, however, we need access to the intermediate plan
fragments produced by the planner. Inspecting only the Thrift version
results in loss of important details.

This fix introduces a new planner context (PlanCtx) class that passes
information into the planner, and can pass the plan fragments back out.
There is no functional change, just a refactoring of the existing code.

The changes also introduces a new test case that uses this feature to
verify plan cardinality.

Tests: The new test case demonstrates the functionality.

Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
2 files changed, 219 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/11920/2
-- 
To view, visit http://gerrit.cloudera.org:8080/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 7: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 7
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Dec 2018 04:01:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................

IMPALA-7842: Expose physical plan for unit testing

The FrontEnd class uses a functional model to generate a plan: pass
in SQL text to the createExecRequest() method and get back a Thrift
plan ready for serialization.

For unit testing, however, we need access to the intermediate physical
plan tree produced by the planner. Inspecting only the Thrift version
results in loss of important details.

This fix introduces a new planner context (PlanCtx) class that passes
information into the planner, and passses the physical plan back out.
Code that used the prior version (pass in a query context and explain
string) are changed to use the new form.

Testing:
* There is no functional change, just a refactoring of the existing
  code. Ran all FE test to verify no regressions.
* Introduces a new test case that uses this feature to verify plan
  cardinality.

Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Reviewed-on: http://gerrit.cloudera.org:8080/11920
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
5 files changed, 258 insertions(+), 38 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 9
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Philip Zeyliger, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11920

to look at the new patch set (#5).

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................

IMPALA-7842: Expose physical plan for unit testing

The FrontEnd class uses a functional model to generate a plan: pass
in SQL text to the createExecRequest() method and get back a Thrift
plan ready for serialization.

For unit testing, however, we need access to the intermediate physical
plan tree produced by the planner. Inspecting only the Thrift version
results in loss of important details.

This fix introduces a new planner context (PlanCtx) class that passes
information into the planner, and passses the physical plan back out.
Code that used the prior version (pass in a query context and explain
string) are changed to use the new form.

Testing:
* There is no functional change, just a refactoring of the existing
  code. Ran all FE test to verify no regressions.
* Introduces a new test case that uses this feature to verify plan
  cardinality.

Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
5 files changed, 237 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/11920/5
-- 
To view, visit http://gerrit.cloudera.org:8080/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1482/ : 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/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Nov 2018 21:42:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11920

to look at the new patch set (#3).

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................

IMPALA-7842: Make query fragments available for unit testing

Today, the FrontEnd class uses a functional model to generate a plan:
pass in SQL text to the createExecRequest() method and get back a Thrift
plan ready for serialization.

For unit testing, however, we need access to the intermediate plan
fragments produced by the planner. Inspecting only the Thrift version
results in loss of important details.

This fix introduces a new planner context (PlanCtx) class that passes
information into the planner, and can pass the plan fragments back out.
There is no functional change, just a refactoring of the existing code.

The changes also introduces a new test case that uses this feature to
verify plan cardinality.

Tests: The new test case demonstrates the functionality.

Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
2 files changed, 219 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/11920/3
-- 
To view, visit http://gerrit.cloudera.org:8080/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1526/ : 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/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 02:11:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1495/ : 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/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Dec 2018 00:43:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................


Patch Set 4:

(1 comment)

Cleaned up the API a bit. Rebased on latest master.

http://gerrit.cloudera.org:8080/#/c/11920/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/11920/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1227
PS1, Line 1227:   public TExecRequest createExecRequest(PlanCtx planCtx)
> There are actually three public entry points. Retained the older two to avo
Went back and inspected the code. Turns out we can remove calls to this (old) entry point so we retain just one.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Dec 2018 00:23:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 8: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 8
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Dec 2018 04:01:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Philip Zeyliger, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11920

to look at the new patch set (#7).

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................

IMPALA-7842: Expose physical plan for unit testing

The FrontEnd class uses a functional model to generate a plan: pass
in SQL text to the createExecRequest() method and get back a Thrift
plan ready for serialization.

For unit testing, however, we need access to the intermediate physical
plan tree produced by the planner. Inspecting only the Thrift version
results in loss of important details.

This fix introduces a new planner context (PlanCtx) class that passes
information into the planner, and passses the physical plan back out.
Code that used the prior version (pass in a query context and explain
string) are changed to use the new form.

Testing:
* There is no functional change, just a refactoring of the existing
  code. Ran all FE test to verify no regressions.
* Introduces a new test case that uses this feature to verify plan
  cardinality.

Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
5 files changed, 258 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/11920/7
-- 
To view, visit http://gerrit.cloudera.org:8080/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 7
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 8:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3529/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 8
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Dec 2018 04:01:32 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1359/ : 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/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Nov 2018 22:31:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................


Patch Set 3:

Rebased on latest master. Bharath, can you take a look at your convenience? Thanks!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Nov 2018 21:11:54 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1537/ : 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/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Dec 2018 01:00:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................


Patch Set 4:

Pre-review tests passed: https://jenkins.impala.io/job/pre-review-test/241/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 03 Dec 2018 07:12:42 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 6: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11920/6/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/11920/6/fe/src/main/java/org/apache/impala/service/Frontend.java@1225
PS6, Line 1225: Optionally fills in the EXPLAIN string
I think the explain string is not optional whereas the thrift plan is?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Dec 2018 16:19:59 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 5:

I think you missed a few comments outside of Frontend class.

https://gerrit.cloudera.org/#/c/11920/4..5


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 05:05:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 7:

(1 comment)

Bharath, thanks for the additional review. Corrected the comment you identified. Rebased on the latest master.

http://gerrit.cloudera.org:8080/#/c/11920/6/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/11920/6/fe/src/main/java/org/apache/impala/service/Frontend.java@1225
PS6, Line 1225: Fills in the EXPLAIN string and option
> I think the explain string is not optional whereas the thrift plan is?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 7
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Dec 2018 21:11:04 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 5:

(5 comments)

Bharath, thanks much for the review. Addressed your comments.

http://gerrit.cloudera.org:8080/#/c/11920/4/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/11920/4/fe/src/main/java/org/apache/impala/service/Frontend.java@181
PS4, Line 181: 
> remove
Done


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/Frontend.java@186
PS4, Line 186: // The query context.
             :     protected final TQueryCtx queryCtx_;
             :     // The explain string built from the query plan.
             :     protected final StringBuilder expla
> Please add one-liner docs for these.
Done


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/Frontend.java@217
PS4, Line 217:     public List<PlanFragment> getPlan() { return plan_; }
> nit: single line
Done


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/Frontend.java@1220
PS4, Line 1220: 
> ?
Done


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/Frontend.java@1221
PS4, Line 1221: 
              : 
              :   /**
              :    * Create a TExecRequest for the query and query context provided in the plan
              :    * context. Optionally fills in the EXP
> Not sure if this applies given the current signature change.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Dec 2018 01:47:32 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1351/ : 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/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Sun, 11 Nov 2018 21:52:03 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................


Patch Set 1:

Dry-run tests passed: https://jenkins.impala.io/job/pre-review-test/226/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Comment-Date: Sun, 11 Nov 2018 21:16:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Philip Zeyliger, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11920

to look at the new patch set (#4).

Change subject: IMPALA-7842: Make query fragments available for unit testing
......................................................................

IMPALA-7842: Make query fragments available for unit testing

The FrontEnd class uses a functional model to generate a plan: pass
in SQL text to the createExecRequest() method and get back a Thrift
plan ready for serialization.

For unit testing, however, we need access to the intermediate plan
fragments produced by the planner. Inspecting only the Thrift version
results in loss of important details.

This fix introduces a new planner context (PlanCtx) class that passes
information into the planner, and can pass the plan fragments back out.
Code that used the prior version (pass in a query context and explain
string) are changed to use the new form.

Testing:
* There is no functional change, just a refactoring of the existing
  code. Ran all FE test to verify no regressions.
* Introduces a new test case that uses this feature to verify plan
  cardinality.

Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
5 files changed, 236 insertions(+), 37 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/11920/4
-- 
To view, visit http://gerrit.cloudera.org:8080/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 6:

(3 comments)

Bharath, thanks for the review. Addressed the comments overlooked in the previous pass.

http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java@171
PS4, Line 171: e
> nit: remove space
Done


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
File fe/src/test/java/org/apache/impala/planner/CardinalityTest.java:

http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@36
PS4, Line 36: PlannerTestBase
> Do we need to rebase this on top of the Analysis/Query fixture changes?
Good point. The fixture handles only analysis and rewrites. We should extend it for the full plan case as well. I'll do this as s separate patch in which I'll also modify some of the other tests to use the fixture.


http://gerrit.cloudera.org:8080/#/c/11920/4/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@119
PS4, Line 119: /
             :   protected void verifyCardinality(String query, long
> Not sure I got this, where is the deep copy?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Dec 2018 00:31:48 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1549/ : 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/11920
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 7
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Dec 2018 21:31:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Expose physical plan for unit testing

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Expose physical plan for unit testing
......................................................................


Patch Set 8: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 8
Gerrit-Owner: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pa...@yahoo.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Dec 2018 07:55:59 +0000
Gerrit-HasComments: No