Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22716 )
Change subject: IMPALA-13011: [WIP] Support authorization for Calcite in Impala ...................................................................... Patch Set 11: (2 comments) I've yet to go through the ImpalaPrivilegeRequestsRegistrar code, but a couple of general notes on this first pass. http://gerrit.cloudera.org:8080/#/c/22716/11/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/22716/11/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@195 PS11, Line 195: if (!analysisResult.getAnalyzer().getQueryCtx().getClient_request() Imo, I think we should avoid using this query option in the main code. We'd start having sprinkles of it here and there. The main code should set the course of action and the libraries can set/override the behavior. In addition to this, while this may sound a bit like overkill, but I'd also like to avoid overloaded methods in AnalysisResult. It currently has a lot of non-Calcite statement methods which are really ugly and can be cleaned up. But I'd prefer any override to be done within an interface, and in this case, I think we should put it in ParsedStatement. So can we put "handleAuthorizationException" in ParsedStatement? We then can always call this method and have the Calcite version be empty. Thanks! http://gerrit.cloudera.org:8080/#/c/22716/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteAnalysisDriver.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteAnalysisDriver.java: http://gerrit.cloudera.org:8080/#/c/22716/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteAnalysisDriver.java@131 PS11, Line 131: CalciteParsedStatement parsedStmtForPrivReqsReg = This seems surprising to me. Calcite mutates the SqlNode? Yuck. The intention was to make CalciteParsedStatement an immutable object, so I really hate that Calcite did this. I suppose if this is true though, I would prefer to code this a different way in order to keep the original CalciteParseStatement immutable. I'm assuming the SqlNode.clone(SqlNode) method clones the entire SqlNode tree (I sure hope it does). Let's create a "CalciteParsedStatement.validate(SqlNode) method. Let's put the validate logic in there. Within that object, it can clone the SqlNode before validating and return a validated SqlNode tree. This will allow CalciteParsedStatement to retain its immutability. I know this is being a little nitty, but this makes CalciteParsedStatement safer to use by any object in the future. -- To view, visit http://gerrit.cloudera.org:8080/22716 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9a7f7e4dc9a86a2da9e387832e552538e34029c1 Gerrit-Change-Number: 22716 Gerrit-PatchSet: 11 Gerrit-Owner: Fang-Yu Rao <[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, 26 Jul 2025 20:47:27 +0000 Gerrit-HasComments: Yes
