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 9: (6 comments) http://gerrit.cloudera.org:8080/#/c/22591/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22591/9//COMMIT_MSG@23 PS9, Line 23: file > Nit: filed Done http://gerrit.cloudera.org:8080/#/c/22591/9//COMMIT_MSG@25 PS9, Line 25: This is true not only for this : code, but also in FrontendBase which has a lot if "if/else" logic dealing : with statement types which should be written as a case statement, imo. > Can we drop this sentence? I don't think we need to advocate for a differen Done http://gerrit.cloudera.org:8080/#/c/22591/9//COMMIT_MSG@34 PS9, Line 34: XXX: Code reviewer note: This is in a state where I hope we can get to : a "soft +1", but not a real +1. I think the code will be easier to review : by not indenting the AnalysisDriverImpl contents. I actually think it would : AnalysisDriverImpl contents. I actually think it might even be better : be better to put both AnalysisResult and AnalysisDriverImpl in separate files. : My plan here is to: a) get a soft +1, b) indent the code, c) get a real +2, : d) commit the code, e) create an addendum which moves the code to a separate : file. : : ...and also remove this XXX: comment from the real commit when ready. > Remove I removed the indentations, but I kept them in the same file. I think the commit change is still readable with the indentation, but becomes a little harder if the move is done in the same commit. If you think they should be in a separate file, I will commit an addendum and move them to separate files. http://gerrit.cloudera.org:8080/#/c/22591/9/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/9/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@525 PS9, Line 525: // XXX: temporarily removed indentation for code review purposes, this needs : // to be changed before committing > Redo indentation and remove this Done http://gerrit.cloudera.org:8080/#/c/22591/9/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@558 PS9, Line 558: //XXX: second place where indentation needs to be reapplied > Reapply indentation and remove this comment Done http://gerrit.cloudera.org:8080/#/c/22591/9/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/9/fe/src/main/java/org/apache/impala/analysis/AnalysisDriver.java@29 PS9, Line 29: * The "analyze" method here does not throw an exception. The exception handling : * should be handled by analyze and the exception should be placed within the : * AnalysisResult. While it would be cleaner to deal with the exception through : * normal exception handling, the AuthorizationChecker requires the existence of : * an AnalysisResult object and unfortunately mutates the AnalysisResult object. : * IMPALA-13848 has been filed to deal with this issue. > Nit: I would like to add some information about the contract of analyze() a Added the information. I'm not super passionate about it, but having said that, I do think the code would be better. If the code were designed better and if AnalysisResult weren't mutable, then the Analyzer and StatementBase could be retrieved locally here off of the AnalysisDriver (side note: an even better design would be if those objects weren't mutable within AnalysisDriver). In that case, the AnalysisResult wouldn't need to be instantiated in the error case, the exception could be placed in a variable, and the analyzeAndAuthorize method could choose which exception to throw. But I don't think the exception handling is that bad. A little weird, but not that bad. So I removed the comment and if some future person comes in and changes it, I'd be ok with that too. -- 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: 9 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: Tue, 11 Mar 2025 11:02:23 +0000 Gerrit-HasComments: Yes
