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 6: (3 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; > Can we make this final as well? I would love to...but this is a little beyond the scope of what I wanted to change here. The setter is in the AuthorizationChecker authorize() method, which is currently called after the AnalysisResult is instantiated. As mentioned in the below comment, this does indeed seem off and needs a redesign, but yeah, it's a bit beyond the scope of what I wanted to change at this point. http://gerrit.cloudera.org:8080/#/c/22591/6/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@95 PS6, Line 95: public AnalysisResult(AnalysisDriver analysisDriver) { > Lets not use AnalysisDriver as parameter. Yeah, now that I think about it, you're right. A bit awkward passing in AnalysisDriver. If the parameter list became longer, we should use a builder class rather than pass in AnalysisDriver. 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); > Can we make the authorization checking as part of AnalysisDriver as well? Idk about this one... If you look at this method holistically, it is called "analyzeAndAuthorize". Everything in this method has to do with authorization checking except for the two AnalysisDriver lines (instantiating the class and calling the implementation specific (either Calcite or original) analysis code). So if I moved authorization checking inside of AnalysisDriver, it would a) cause AnalysisDriver to contain code that is common to both implementations, and b) basically eliminate the need for this method. I do think that there is something off about the design here in general? Can't put my finger on it though, but this seems a bit out of scope for what I'm trying to accomplish here. But yeah, I think I prefer to keep this line here with the general authorize code. Thoughts? -- 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: 6 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 18:15:49 +0000 Gerrit-HasComments: Yes
