Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18561 )
Change subject: IMPALA-11281: Load table metadata for ResetMetadataStmt ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/18561/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18561/1//COMMIT_MSG@22 PS1, Line 22: a previous test case. Can we add this test case? E.g. explicitly run "invalidate metadata functional.alltypestiny" using the admin user, then run this with the test user and expect it fails. http://gerrit.cloudera.org:8080/#/c/18561/1/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java: http://gerrit.cloudera.org:8080/#/c/18561/1/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@142 PS1, Line 142: set in : // BaseAuthorizationChecker#authorizePrivilegeRequest(), which in turn allows : // RangerAuthorizationChecker#authorizeByTableMasking() nit: I tend to not comment such implementation details since they could be stale if we update the code and forget to update this comment. I think saying "the information about the columns of the table could be used to check whether masking is enabled for any column in the table" is enough. Can we also comment that the results are used to load the metadata, and comment the other case, i.e. REFRESH with partition spec? http://gerrit.cloudera.org:8080/#/c/18561/1/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java@149 PS1, Line 149: if (tableName_ != null) { : if (authzProvidedByRanger_) { : tblRefs.add(new TableRef(tableName_.toPath(), null)); : } else if (partitionSpec_ != null) { : tblRefs.add(new TableRef(tableName_.toPath(), null)); : } : } nit: the two cases are identical, we can simplify this to if (tableName_ != null && (authzProvidedByRanger_ || partitionSpec_ != null)) { tblRefs.add(new TableRef(tableName_.toPath(), null)); } http://gerrit.cloudera.org:8080/#/c/18561/1/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/18561/1/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@355 PS1, Line 355: ((ResetMetadataStmt) stmt).setAuthzProvidedByRanger(true); Can we simply add the tableName in such case? Then we don't need any changes in ResetMetadataStmt.java. if (stmt instanceof ResetMetadataStmt && fe_.getAuthzFactory().getAuthorizationConfig().isEnabled() && fe_.getAuthzFactory().supportsTableMasking()) { TableRef tableName = ((ResetMetadataStmt) stmt).getTableName(); if (tableName != null) tblRefs.add(tableName); } else { stmt.collectTableRefs(tblRefs); } -- To view, visit http://gerrit.cloudera.org:8080/18561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0c90b413974223886661697f11844d99a68fdebf Gerrit-Change-Number: 18561 Gerrit-PatchSet: 1 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 24 May 2022 06:57:39 +0000 Gerrit-HasComments: Yes
