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 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/22591/5/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/5/fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java@25 PS5, Line 25: public interface AnalysisDriver > Just in case we need to release Impala 4.5.1 to fix planner bug, is this ea I think so? I'm not familiar with what would be different. But this is more of a refactor of code. That's why this is a standalone commit and not released with Calcite at all. The test framework only had a very minor change. An I already ran this on jenkins. I mean, the only way it wouldn't be compatible is if someone were using some Analysis java structures outside of the code base. I'm not aware of any? http://gerrit.cloudera.org:8080/#/c/22591/5/fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java@28 PS5, Line 28: public StatementBase getStmt(); : : public Analyzer getAnalyzer(); > What is the expectation of return value of each method here? Well, these aren't strictly necessary and I could get rid of them if you think it causes confusion in any way. The only place AnalysisDriver is used is within AnalysisContext and it is used to "analyze" and produce the "AnalysisResult" (AnalysisContext:484). I don't expect AnalysisDriver to be used anywhere else. These two methods are merely helper methods when the constructor of AnalysisResult is called like this (see line AnalysisContext.java:649 in patch 5): // "this" is an AnalysisDriver object return new AnalysisResult(this); Within the AnalysisResult constructor (AnalysisContext:99-103), it calls these 2 getter methods to fill in its "final" variables. One other approach I could have done would get rid of these getter methods. I could easily have made the AnalysisResult constructor look like this: return new AnalysisResult(analyzer, stmt); I have absolutely no preference as to which one to use. The parameter list in the constructor is small enough that a wrapper object doesn't really benefit too much. -- 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: 5 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> Gerrit-Comment-Date: Sat, 08 Mar 2025 05:37:15 +0000 Gerrit-HasComments: Yes
