Fang-Yu Rao 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: (5 comments) Thanks for the detailed comments Steve! I have replied to each comment. Hopefully I did not misunderstand them. 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' Thanks for the detailed analysis! I think the code would be cleaner if we move "handleAuthorizationException()" from StatementBase to ParsedStatement. handleAuthorizationException() was added recently in IMPALA-12648 (Add KILL QUERY statement) and only has non-empty implementation for the KILL QUERY statement. Thus it should not be too difficult to move "handleAuthorizationException()" to ParsedStatement. 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 Thanks Steve! I will remove it in the next patch. 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 = > The intention was to make CalciteParsedStatement an immutable object, so I > really hate that Calcite did this. I could provide an example query of which the SqlNode would be changed by Calcite's own validate(). SELECT ABS(BIGINT_COL) FROM functional.alltypestiny This SqlNode is a SqlSelect and thus has a field 'from' representing its FROM clause. Before the validation, this 'from' was a SqlIdentifier for the table name "functional.alltypestiny", whereas after the validation, the validation, it becomes a SqlBasicCall representing an alias "functional.alltypestiny AS ALLTYPESTINY". > Let's create a "CalciteParsedStatement.validate(SqlNode) method. Let's put > the validate logic in there. I will try to implement this in the next patch set. To do this we may also have to move some fields of CalciteAnalysisResult to CalciteParsedStatement, e.g., reader_, typeFactory_. And as pointed out above, we may have to pass a SqlValidator into this method in CalciteParsedStatement. 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 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. Yes. The current patch calls validate() 3 times. - The first time is sqlValidator_.validate(), which is the original validate() implemented by SqlValidatorImpl. Based on my observation, this method is not called to validate column names involved in tables underlying a view. - The second time is impalaPrivReqsReg.validate(), which takes care of registering column-level privilege requests for columns not in tables referenced in a view. Since ImpalaPrivilegeRequestsRegistrar extends SqlValidatorImpl, impalaPrivReqsReg.validate() does not register column-level privilege requests for columns in tables underlying a view. - The third time is validate(parsedSqlNode) within ImpalaPrivilegeRequestsRegistrar#registerPrivReqsInView(), which registers column-level requests for columns in tables underlying a view. This is necessary in that if the requesting user does not have the SELECT privileges on the tables or columns underlying a view, Impala should disallow the user from accessing the details in the query profile in impala-shell via the 'profile' command. > 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? Registering table-level privilege requests before any "validate" call is possible. This could be done by the already existing method registerTablePrivReqs(Set<TableName> tableNames, FeCatalog catalog) in ImpalaPrivilegeRequestsRegistrar. However, registering column-level privilege requests before any "validate" call seems to be difficult. Recall that Impala's classic frontend registers the column-level privilege request for a column only after the column name could be successfully resolved. When the planner is Calcite, this corresponds to scope.fullyQualify(id) returning without any exception. Since SqlValidatorImpl#validateIdentifier() is a place where we know whether a given column name could be successfully resolved, we register the column-level privilege requests in validateIdentifier(). > We can also then have the two overridden classes call > super.validateIdentifer() and super.validateCall() to ensure the Calcite > validation is done. Based on the discussion above, does it sound okay to slightly extend SqlValidatorImpl to also take are of column-level and function-level privilege requests? This way we could just have one "validate" call that validates the query and also registers the column-level and function-level privilege requests. On the other hand, in the proposal above, we would still need the "validate" call to register the column-level privilege requests for columns in tables referenced by views because the original validate() in SqlValidatorImpl does not seem to expand views to validate the underlying columns if a view was added to CalciteCatalogReader as a ViewTable (refer to CalciteDb#createViewTable()). On a related note, even though we added a view as a ViewTableMacro as discussed at https://gerrit.cloudera.org/c/22883/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaViewTableMacro.java#34 that would validate the columns in tables underlying a view, we still need to register column-level privilege requests for columns in tables underlying a view. This would require a custom validator that is able to register privilege requests and it does not seem straightforward to provide a custom validator when Schemas.analyzeView() is invoked. If this (slightly extending SqlValidatorImpl to also take are of column-level and function-level privilege requests) sounds okay, I will try to implement this in the next patch set. 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 f Thanks for pointing this out! My change here is not really necessary. I will revert this part of change in the next patch set. -- 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: Mon, 28 Jul 2025 21:53:18 +0000 Gerrit-HasComments: Yes
