[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 13: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 13 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Sat, 15 Dec 2018 01:58:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. A "session fixture" tracks session-level state including default options, database, user. Then a set of per-query fixtures provide various levels of per-query functionality from parse-only, to normal analysis and to special no-rewrite analysis. The rewrite-specific version resides in the only test that needs it: ExprRewriteRulesTest. The other, more general, versions reside in the new test fixture class. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase base class was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate usage of the test fixture. The key value of this work is to allow greater detail when testing future changes. In order to keep the refactoring as simple and clean, no new test cases were added here. Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Reviewed-on: http://gerrit.cloudera.org:8080/11881 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- A fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 4 files changed, 467 insertions(+), 83 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 14 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 12: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1618/ : 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/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 12 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 21:59:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 13: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 13 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 22:05:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 13: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3569/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 13 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 22:05:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 12: Code-Review+2 Thanks for the fixes. This LGTM. -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 12 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 21:33:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Paul Rogers has posted comments on this change. (
http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture
..
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java
File fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java:
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java@171
PS10, Line 171: public StatementBase statement() { return stmt_; }
> I don't actually mean adding Preconditions.checkNotNull but Preconditions.c
Got it, makes sense. Adjusted accordingly.
--
To view, visit http://gerrit.cloudera.org:8080/11881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 12
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Fri, 14 Dec 2018 21:24:25 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11881 to look at the new patch set (#12). Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. A "session fixture" tracks session-level state including default options, database, user. Then a set of per-query fixtures provide various levels of per-query functionality from parse-only, to normal analysis and to special no-rewrite analysis. The rewrite-specific version resides in the only test that needs it: ExprRewriteRulesTest. The other, more general, versions reside in the new test fixture class. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase base class was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate usage of the test fixture. The key value of this work is to allow greater detail when testing future changes. In order to keep the refactoring as simple and clean, no new test cases were added here. Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c --- A fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 4 files changed, 467 insertions(+), 83 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/11881/12 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 12 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1616/ : 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/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 11 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 14 Dec 2018 21:17:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture
..
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java
File fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java:
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java@171
PS10, Line 171: public StatementBase statement() { return stmt_; }
> Actually, if you look inside the checkNull assertion, it simply throws an N
I don't actually mean adding Preconditions.checkNotNull but
Preconditions.checkState instead. I always see Preconditions as a check for
programming error. So, if the code hits Preconditions, that means there's a bug
in the code. With a standard NPE, we can't really sure if the state is valid or
invalid.
--
To view, visit http://gerrit.cloudera.org:8080/11881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 11
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Fri, 14 Dec 2018 20:59:00 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Paul Rogers has posted comments on this change. (
http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture
..
Patch Set 11:
(6 comments)
Thanks for the reviews. Addressed comments and rebased on master.
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java
File fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java:
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java@171
PS10, Line 171: public StatementBase statement() { return stmt_; }
> We need a Preconditions to check if the statement has been analyzed or else
Actually, if you look inside the checkNull assertion, it simply throws an NPE
if the object is null. The key value of that check is to clearly state in the
code that something is not supposed to be null. In a more complex method, it
can check invariants at the top to avoid tracking down bigs inside a complex
bit of code.
Here, the code is a one-liner: if the analysis result is null, Java will throw
an NPE exactly as if the Preconditions.checkNotNull() were called. And, someone
looking at the stack should have pretty good idea what is wrong.
So, the question then becomes, does the Preconditions check add value beyond
what the code itself already checks?
I think the answer here is that the message associated with the Preconditions
can help a developer catch errors when learning now to use the fixture. So,
added Preconditions here and elsewhere with messages to explain correct usage.
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@75
PS10, Line 75: @Override
> Similarly, add Preconditions to check if the statement has been analyzed.
Done
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@78
PS10, Line 78: turn analyzer_;
> nit: we can probably join L78 with L77
Done
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@90
PS10, Line 90: for (ExprRewriteRule r : rules) {
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@160
PS10, Line 160:
> line too long (106 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@174
PS10, Line 174: }
> line too long (106 > 90)
Done
--
To view, visit http://gerrit.cloudera.org:8080/11881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 11
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Fri, 14 Dec 2018 20:44:40 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11881 to look at the new patch set (#11). Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. A "session fixture" tracks session-level state including default options, database, user. Then a set of per-query fixtures provide various levels of per-query functionality from parse-only, to normal analysis and to special no-rewrite analysis. The rewrite-specific version resides in the only test that needs it: ExprRewriteRulesTest. The other, more general, versions reside in the new test fixture class. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase base class was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate usage of the test fixture. The key value of this work is to allow greater detail when testing future changes. In order to keep the refactoring as simple and clean, no new test cases were added here. Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c --- A fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 4 files changed, 467 insertions(+), 82 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/11881/11 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 11 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture
..
Patch Set 10:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java
File fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java:
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java@171
PS10, Line 171: public Analyzer analyzer() { return
analysisResult_.getAnalyzer(); }
We need a Preconditions to check if the statement has been analyzed or else it
will result in an NPE.
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@75
PS10, Line 75: public Analyzer analyzer() { return analyzer_; }
Similarly, add Preconditions to check if the statement has been analyzed.
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@78
PS10, Line 78: List rules,
nit: we can probably join L78 with L77
http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@90
PS10, Line 90:
nit: remove new line
--
To view, visit http://gerrit.cloudera.org:8080/11881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 10
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Comment-Date: Wed, 12 Dec 2018 19:03:12 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1564/ : 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/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 10 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Mon, 10 Dec 2018 19:28:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 9: Fixed the build issue; it resulted from an earlier fix in response to a review comment. Also took this opportunity to revise the code to reflect what has been learned in the last few weeks. Divided up the tasks somewhat differently. The rewrite fixture is in the one test that uses it. (The mechanism it tests is in the process of being deprecated, so the fixture is temporary.) Please take a look to see if the revised form is easier to follow. -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 9 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Mon, 10 Dec 2018 18:53:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@160 PS10, Line 160: ExprRewriteRulesTest.SelectRewriteFixture qf = new ExprRewriteRulesTest.SelectRewriteFixture(session); line too long (106 > 90) http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@174 PS10, Line 174: ExprRewriteRulesTest.SelectRewriteFixture qf = new ExprRewriteRulesTest.SelectRewriteFixture(session); line too long (106 > 90) -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 10 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Mon, 10 Dec 2018 18:52:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11881 to look at the new patch set (#10). Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. A "session fixture" tracks session-level state including default options, database, user. Then a set of per-query fixtures provide various levels of per-query functionality from parse-only, to normal analysis and to special no-rewrite analysis. The rewrite-specific version resides in the only test that needs it: ExprRewriteRulesTest. The other, more general, versions reside in the new test fixture class. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase base class was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate usage of the test fixture. The key value of this work is to allow greater detail when testing future changes. In order to keep the refactoring as simple and clean, no new test cases were added here. Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c --- A fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 4 files changed, 452 insertions(+), 82 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/11881/10 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 10 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 9: It looks like there are a few genuine test failures here. Paul, mind taking a look at the output here https://jenkins.impala.io/job/gerrit-verify-dryrun/3534/consoleFull -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 9 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 07 Dec 2018 06:49:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 9: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3534/ -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 9 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 07 Dec 2018 03:40:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 9: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3534/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 9 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Thu, 06 Dec 2018 23:36:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 8: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3523/ -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 8 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Wed, 05 Dec 2018 03:06:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 8: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3523/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 8 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Tue, 04 Dec 2018 23:10:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 7: Code-Review+2 I think the overall idea makes sense to me. We can iterate over it and add more helper methods as follow up changes once we port the existing tests to use these fixtures. -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 7 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 30 Nov 2018 18:56:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11881 to look at the new patch set (#7). Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. For example, the fixture allows setting per-query options, allows doing full or partial (no rewrite) analysis, provides access to things like the Analyzer, and more. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase base class was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate usage of the test fixture. The key value of this work is to allow greater detail when testing future change requests. In order to keep the refactoring as simple and clean, no new test cases were added here Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c --- A fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java A fe/src/test/java/org/apache/impala/analysis/QueryFixture.java A fe/src/test/java/org/apache/impala/analysis/RewriteFixture.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 6 files changed, 539 insertions(+), 89 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/11881/7 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 7 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 5: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/1376/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 16 Nov 2018 00:27:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 4: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/1375/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Comment-Date: Fri, 16 Nov 2018 00:16:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11881 to look at the new patch set (#5). Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. For example, the fixture allows setting per-query options, allows doing full or partial (no rewrite) analysis, provides access to things like the Analyzer, and more. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase base class was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate usage of the test fixture. The key value of this work is to allow greater detail when testing future change requests. In order to keep the refactoring as simple and clean, no new test cases were added here Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c --- A fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java A fe/src/test/java/org/apache/impala/analysis/QueryFixture.java A fe/src/test/java/org/apache/impala/analysis/RewriteFixture.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 6 files changed, 503 insertions(+), 89 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/11881/5 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/11881/4/fe/src/test/java/org/apache/impala/analysis/QueryFixture.java File fe/src/test/java/org/apache/impala/analysis/QueryFixture.java: http://gerrit.cloudera.org:8080/#/c/11881/4/fe/src/test/java/org/apache/impala/analysis/QueryFixture.java@72 PS4, Line 72: parsedStmt_ = this.analysisFixture_.frontend_.parse(stmt_, analysCtx_.getQueryOptions()); line too long (93 > 90) http://gerrit.cloudera.org:8080/#/c/11881/4/fe/src/test/java/org/apache/impala/analysis/QueryFixture.java@118 PS4, Line 118:new StmtMetadataLoader(this.analysisFixture_.frontend_, analysCtx_.getQueryCtx().session.database, null); line too long (112 > 90) -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 15 Nov 2018 22:15:47 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11881 to look at the new patch set (#4). Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. For example, the fixture allows setting per-query options, allows doing full or partial (no rewrite) analysis, provides access to things like the Analyzer, and more. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase base class was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate usage of the test fixture. The key value of this work is to allow greater detail when testing future change requests. In order to keep the refactoring as simple and clean, no new test cases were added here Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c --- A fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java A fe/src/test/java/org/apache/impala/analysis/QueryFixture.java A fe/src/test/java/org/apache/impala/analysis/RewriteFixture.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 6 files changed, 500 insertions(+), 89 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/11881/4 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture
..
Patch Set 3:
(14 comments)
Some high-level comments about the class structure and a few nits.
The refactoring overall makes sense to me. Will take another deeper pass once
we finalize on the class structure. I think there is too much logic stuffed
into AnalysisFixture class (which also makes it difficult to review). Please
refer to the comments about breaking it and if that makes sense.
http://gerrit.cloudera.org:8080/#/c/11881/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11881/3//COMMIT_MSG@19
PS3, Line 19: the FrontEndBase was left unchanged.
Can you please add a TODO there? There is a lot of common code and this might
confuse other developers as to what to use.
Also, it is worth mentioning that we suggest using these test fixtures starting
now rather than using the "functional" approach.
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java
File fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java:
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@55
PS3, Line 55: *
Add an example usage here? (something like in ExprRewriterTest)
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@56
PS3, Line 56: The QueryFixture holds per-query state which can include a
test-specific
: * set of options. The query can be analyzed in full, or only
analyzed up
: * to rewrites. It provides access to normally-hidden temporary
objects
: * such as the analyzer, analysis context and so on. Tests can
probe
: * these objects to check for conditions of interest.
: *
: * The Rewrite fixture builds on the QueryFixture to add
additional
: * tools needed to probe rewrites of the various clauses within a
: * query.
Move to their respective class definitions?
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@80
PS3, Line 80: private final TQueryCtx queryCtx;
We generally use "_" suffix for class member variables. For ex: queryCtx_.
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@86
PS3, Line 86: private Analyzer analyzer;
Do we need to track this separately? can be inferred from the analysisResult
using getAnalyzer()
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@103
PS3, Line 103: e.printStackTrace();
use log4j logger
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@109
PS3, Line 109:
We probably also need helper functions like parsesOk() etc? (to port ParserTest)
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@114
PS3, Line 114: makeAnalysisContext
move this to ctor?
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@135
PS3, Line 135: public QueryFixture analyzeWithoutRewrite() {
I think this also skips authz. Mention that?
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@204
PS3, Line 204: private static class CountingRewriteRuleWrapper implements
ExprRewriteRule {
Move it inside RewriteFixture? Doesn't seem like to be used else where.
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@205
PS3, Line 205: private int rewrites;
Use "_" suffix.
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@224
PS3, Line 224: public class RewriteFixture {
Move it to its own class? IIUC AnalysisFixture is a more generic thing that we
might use across tests, not sure if it makes sense to put RewriteFixture here.
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@256
PS3, Line 256: query = AnalysisFixture.this.query(stmtStr);
Does it make sense for RewriteFixture to extend AnalysisFixture to avoid this
boilerplate?
http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@284
PS3, Line 284: return
query.selectStmt().getSelectList().getItems().get(0).getExpr();
assert that it is analyzed before calling this?
--
To view, visit http://gerrit.cloudera.org:8080/11881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1335/ : 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/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 08 Nov 2018 23:22:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Paul Rogers has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. For example, the fixture allows setting per-query options, allows doing full or partial (no rewrite) analysis, provides access to things like the Analyzer, and more. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate how it can be used. The key value of this work is to allow greater detail in testing in future change requests. No new test cases were added here in order to keep the refactoring as simple and clean as possible. Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c --- A fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 3 files changed, 480 insertions(+), 89 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/11881/3 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 ) Change subject: IMPALA-7807: Analysis test fixture .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1288/ : 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/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 06 Nov 2018 03:47:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11881/1/fe/src/test/java/org/apache/impala/testutil/TestUtils.java
File fe/src/test/java/org/apache/impala/testutil/TestUtils.java:
http://gerrit.cloudera.org:8080/#/c/11881/1/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@293
PS1, Line 293: return createQueryContext(Catalog.DEFAULT_DB,
System.getProperty("user.name"), options);
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/11881/1/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@301
PS1, Line 301: public static TQueryCtx createQueryContext(String defaultDb,
String user, TQueryOptions options) {
line too long (100 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 06 Nov 2018 03:15:12 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7807: Analysis test fixture
Paul Rogers has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11881 Change subject: IMPALA-7807: Analysis test fixture .. IMPALA-7807: Analysis test fixture Refactors the existing ExprRewriteRulesTest to pull out the framework functionality into a "test fixture" class which can be used for a variety of tests. The fixture allows more variation, and access to more intermediate state, than does the existing function-based framework. For example, the fixture allows setting per-query options, allows doing full or partial (no rewrite) analysis, provides access to things like the Analyzer, and more. The fixture pulls in code from FrontEndBase to handle the low-level tasks of running a query, and from ExprRewriteRulesTest for the rewrite-specific aspects. For now, the FrontEndBase was left unchanged. ExprRewriteRulesTest is refactored to use the new fixture in order to illustrate how it can be used. The key value of this work is to allow greater detail in testing in future change requests. No new test cases were added here in order to keep the refactoring as simple and clean as possible. Testing: since this is a test fixture, the refactored ExprRewriteRulesTest implicitly tests the fixture code. No "production" (non-test) code was changed in this patch. Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c --- A fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 3 files changed, 478 insertions(+), 89 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/11881/1 -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Rogers
