Joe McDonnell 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: Code-Review+1 (6 comments) This is looking good to me, just a couple minor nits 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 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 different JIRA in this commit message. 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 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 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 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() and why it doesn't throw an exception. Authorization must be enforced whether analysis succeeds or fails, and it relies on information generated by this analyze() method (e.g. columns accessed, etc). To convey that information, analyze() must always return an AnalysisResult with enough information to do authorization whether analysis succeeds or fails. A failed AnalysisResult carries an exception with the analysis failure. How passionate are people about the exception handling? Is someone passionate enough about it to come back and change this later? If not, we can drop the comment about exception handling / IMPALA-13848 and document the interaction between IMPALA-13848 and this API on IMPALA-13848 itself. I personally would keep it the way it is even after a fix for IMPALA-13848. -- 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 <[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: Tue, 11 Mar 2025 04:31:25 +0000 Gerrit-HasComments: Yes
