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 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/22591/4/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/22591/4/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@95 PS4, Line 95: public AnalysisResult(AnalysisDriverImpl analysisDriver) { > Why wouldn't this take an AnalysisDriver interface instead of the impl? Heh. I was asking myself the same question when I was applying this to the Calcite Planner commit. Fixed. http://gerrit.cloudera.org:8080/#/c/22591/4/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@99 PS4, Line 99: public AnalysisResult(AnalysisDriverImpl analysisDriver, ImpalaException e) { > This interface was surprising. Is there some reason it's better than lettin Yeah, heh. So I don't have too big of a preference on this one. If you look at the previous commit (or 2), I have it a different way. The thing is that if we do it the other way, it's still a bit awkward. In the previous iteration, the caller always needed an AnalysisResult, so the code would a) instantiate the AnalysisResult and b) put the exception in a variable and then use that variable to check to see if an exception was thrown. It seemed more natural to me that if we are gonna have an Exception, it is really the Result of the Analysis that caused the exception and thus should be captured there. A note about the previous implementation is that we could refactor the code so that we didn't need an AnalysisResult. But it still seems slightly awkward because we'd have to grab the Analyzer and StatementBase from the AnalysisDriver in the "catch" part of the code. So...Idk. Do you think the other implementation is better? I didn't think so, but I don't feel strongly about it, so I am open to implementing this differently. -- 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: 4 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: Sat, 08 Mar 2025 00:50:39 +0000 Gerrit-HasComments: Yes
