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: (4 comments) More comments. Still haven't looked at some of the details within ImpalaPrivilegeRequestsRegistrar, but before I do that, I'd like to resolve the comment I had about the "validate()" method. http://gerrit.cloudera.org:8080/#/c/22716/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaViewTable.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaViewTable.java: http://gerrit.cloudera.org:8080/#/c/22716/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaViewTable.java@24 PS11, Line 24: import org.checkerframework.checker.nullness.qual.Nullable; nit: unused, I think 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 inte Actually, we'll also have to pass in the SqlValidator into that method since we can't create the validator in the CalciteParsedStatement object (it's too early) http://gerrit.cloudera.org:8080/#/c/22716/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteAnalysisDriver.java@143 PS11, Line 143: impalaPrivReqsReg.validate(parsedStmtForPrivReqsReg.getParsedSqlNode()); I'm a little concerned by this. I noticed validate() is also called within the ImpalaPrivReqsReg object. That would make 3 times that we call validate, once for the original validate, once here, and once within the ImpalaPrivReqsReg class If I understand correctly, and I'm making a big assumption here, it's probably because you want to take advantage of the recursive nature of the call, and that makes sense. But we'd be getting all the other baggage of the "validate" function. We are overriding two of the calls, validateIdentifier and validateCall, but there's a lot of other stuff going on in the validate() method. Is there anyway we can take advantage of the original "validate" call? Can we do all the registering up in a separate class before any validation is called? That would make the registration part a little cleaner, if possible. If this is possible, we can then have a simpler validation class and we would only have to call it once. We can also then have the two overridden classes call super.validateIdentifer() and super.validateCall() to ensure the Calcite validation is done. I could very well be missing something. If it's not possible, let's discuss. http://gerrit.cloudera.org:8080/#/c/22716/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteAnalysisDriver.java@148 PS11, Line 148: analysisResult = new CalciteAnalysisResult(this); Don't really care too much, but any reason why you decided to change this from "return new CalciteAnalysisResult()" to putting it at the end? I don't mind this too much...but did you change it just for debugging purposes? -- 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 23:05:47 +0000 Gerrit-HasComments: Yes
