Riza Suminto 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: (5 comments) http://gerrit.cloudera.org:8080/#/c/22591/6/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/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@93 PS6, Line 93: private boolean userHasProfileAccess_ = true; > I would love to...but this is a little beyond the scope of what I wanted to Ack http://gerrit.cloudera.org:8080/#/c/22591/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@95 PS6, Line 95: public AnalysisResult(StatementBase stmt, Analyzer ana > Yeah, now that I think about it, you're right. A bit awkward passing in An Done http://gerrit.cloudera.org:8080/#/c/22591/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@485 PS6, Line 485: authzChecker.postAnalyze(authzCtx); > Idk about this one... Ack. http://gerrit.cloudera.org:8080/#/c/22591/7/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/7/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@481 PS7, Line 481: AnalysisDriverImpl nit: AnalysisDriver 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. Please describe what is this AnalysisDriver and AnalysisDriverImpl try to solve, that is not possible in the old way. This seems to mask the exception handling as well. Before, AnalysisContext.analyze() can propagate AnalysisException. But AnalysisDriver.analyze() is not. The interface method should warn caller that they should check for AnalysisResult.getException() ahead of everything else. I must admit, not going through regular exception handling path (try-catch) feels a bit awkward. -- 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 <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: Mon, 10 Mar 2025 19:28:32 +0000 Gerrit-HasComments: Yes
