Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22591 )
Change subject: IMPALA-13841: Refactor AnalysisResult to make it immutable and simpler ...................................................................... Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/22591/7/fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java File fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java: http://gerrit.cloudera.org:8080/#/c/22591/7/fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java@22 PS7, Line 22: /** : * Interface for the analyze method which returns an AnalysisResult. : */ > The docs can be more detailed. So my default thinking all along (as shown with my first commit) is to leave the legacy code alone as much as possible. But... Maybe I thought of a way to handle the Exception external to AnalysisResult, but before I implement, lemme run it past you... The whole reason we need AnalysisResult object is that we need the Planner to have the result of the analysis. And the planner needs the mutated Analyzer and StatementBase objects. The legacy code also requires the AnalysisResult to be instantiated within this method. But here's the thing... Internal to this method, we also have access to the AnalysisDriver. This object also has the most recent mutated Analyzer and StatementBase. So if I replace all analysisResult_.getStmt() and analysisResult_.getAnalyzer() calls with analysisDriver_ WITHIN this method, we wouldn't need the AnalysisResult object in the exception case. I would have to re-insert the getStmt() and getAnalyzer() methods on the AnalysisDriver interface to do this. Did that make sense? If so, I'll code an alternative version that would prolly be more inline to what you're suggesting and prolly a better design. -- 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: comment Gerrit-Change-Id: Idaf8854b1ce18c72a49893fc36e2cea77b7a2485 Gerrit-Change-Number: 22591 Gerrit-PatchSet: 7 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Mon, 10 Mar 2025 20:12:35 +0000 Gerrit-HasComments: Yes
