Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/22716 )
Change subject: IMPALA-13011: Support authorization for Calcite in Impala ...................................................................... Patch Set 17: (6 comments) Hi all, I have tried to address Steve's comments on patch set 16. In addition, I also changed/corrected how we register privilege requests when a view could be defined on top of other views to match what the classic Impala frontend does in this case. Let me know if there are additional suggestions. Thanks! http://gerrit.cloudera.org:8080/#/c/22716/16/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java File fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/22716/16/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@83 PS16, Line 83: // We should remove this after IMPALA-14295 is resolved. > If I'm understanding correctly, this variable is only set because this feat Sure. I created IMPALA-14295 (Support column masking and row filtering for Calcite planner) and will add the code comment here so we won't forget to remove this variable after IMPALA-14295 is fixed. http://gerrit.cloudera.org:8080/#/c/22716/16/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/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaViewTable.java@28 PS16, Line 28: private final FeView table_; > nit: final Done http://gerrit.cloudera.org:8080/#/c/22716/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ImpalaSqlValidatorImpl.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ImpalaSqlValidatorImpl.java: http://gerrit.cloudera.org:8080/#/c/22716/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ImpalaSqlValidatorImpl.java@91 PS16, Line 91: // A view would be an instance of RelOptTableImpl if the view was created via > I'm thinking that having the register* functions here just pollutes the Val Sure. I will make the suggested change in the next patch set. http://gerrit.cloudera.org:8080/#/c/22716/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ImpalaSqlValidatorImpl.java@95 PS16, Line 95: FeView view = ((ImpalaViewTable) validatorTable.table()).getFeView(); > Nit: we don't need this if check anymore Done http://gerrit.cloudera.org:8080/#/c/22716/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ImpalaSqlValidatorImpl.java@124 PS16, Line 124: > Can we just call registerTablePrivReqs rather than repeat this for loop or I could try to create a method registerTablePrivReq() that registers the privilege request for a given TableName. This method will be used by both registerTablePrivReqs() and registerPrivReqsInView(). Hope this could make the code more concise. http://gerrit.cloudera.org:8080/#/c/22716/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ImpalaSqlValidatorImpl.java@209 PS16, Line 209: > Can we make reference to https://issues.apache.org/jira/browse/IMPALA-13095 Done -- 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: 17 Gerrit-Owner: Fang-Yu Rao <fangyu....@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: Fri, 08 Aug 2025 23:57:35 +0000 Gerrit-HasComments: Yes
