Hello Aman Sinha, Fang-Yu Rao, Riza Suminto, Joe McDonnell, Michael Smith,
Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22591
to look at the new patch set (#9).
Change subject: IMPALA-13841: Refactor AnalysisResult to make it immutable and
simpler
......................................................................
IMPALA-13841: Refactor AnalysisResult to make it immutable and simpler
This refactoring is needed as a pre-step to adding analysis for the
Calcite Planner
The AnalysisResult class should be an class with immutable members with
the results obtained from analyzing the query. The Planner step which
follows should then be able to use the AnalysisResult to obtain the analysis
information. To keep it clean, AnalysisResult should not have any methods
like "createAnalyzer", something that produces a new object.
A new interface (and its implementor), AnalysisDriver has only one method
"analyze" which produces the AnalysisResult. The Calcite planner will
eventually implement this interface to produce its own AnalysisResult
(or a class derived from AnalysisResult) used by the framework.
Jira ticket IMPALA-13840 has been file to deal with some code that was
duplicated.
The code in many places can be made simpler if there were a StatementType enum
dealing with the types of StatementBase. This is true not only for this
code, but also in FrontendBase which has a lot if "if/else" logic dealing
with statement types which should be written as a case statement, imo.
While the goal was to make AnalysisResult immutable, a bit more refactoring
has to be done to achieve this completely. The userHasProfileAccess_ member is
still mutable because it is changed in the AuthorizationChecker. This would
involve a more extensive change. Jira ticket IMPALA-13848 has been filed.
XXX: Code reviewer note: This is in a state where I hope we can get to
a "soft +1", but not a real +1. I think the code will be easier to review
by not indenting the AnalysisDriverImpl contents. I actually think it would
AnalysisDriverImpl contents. I actually think it might even be better
be better to put both AnalysisResult and AnalysisDriverImpl in separate files.
My plan here is to: a) get a soft +1, b) indent the code, c) get a real +2,
d) commit the code, e) create an addendum which moves the code to a separate
file.
...and also remove this XXX: comment from the real commit when ready.
Change-Id: Idaf8854b1ce18c72a49893fc36e2cea77b7a2485
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
8 files changed, 229 insertions(+), 123 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/22591/9
--
To view, visit http://gerrit.cloudera.org:8080/22591
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idaf8854b1ce18c72a49893fc36e2cea77b7a2485
Gerrit-Change-Number: 22591
Gerrit-PatchSet: 9
Gerrit-Owner: Steve Carlin <scar...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com>
Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <scar...@cloudera.com>
