Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/23569 )
Change subject: IMPALA-14507: Register column-level privilege requests for INSERT ...................................................................... Patch Set 8: (7 comments) http://gerrit.cloudera.org:8080/#/c/23569/8/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java File fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java: http://gerrit.cloudera.org:8080/#/c/23569/8/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@273 PS8, Line 273: * 4. Privilege level is not SELECT. nit: need to update this as well http://gerrit.cloudera.org:8080/#/c/23569/8/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java: http://gerrit.cloudera.org:8080/#/c/23569/8/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java@31 PS8, Line 31: public class AuthorizableColumn extends Authorizable { Most of the code in this class is identical to AuthorizableTable. Can we let AuthorizableColumn inherit AuthorizableTable and just override getType() and getColumnName()? http://gerrit.cloudera.org:8080/#/c/23569/8/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java@79 PS8, Line 79: public List<String> getColumns() { : return columns_; } nit: can be in one line http://gerrit.cloudera.org:8080/#/c/23569/8/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/23569/8/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@53 PS8, Line 53: ALLOWED_TABLE_PRIVILEGES I was confused why REFRESH, ALTER, DROP are not here. Then I realized this is only used for hierarchical auth stmt. Maybe rename it to ALLOWED_HIER_AUTH_TABLE_PRIVILEGES or ALLOWED_HIER_AUTH_TBL_PRIVILEGES or HIER_AUTH_TBL_PRIVS. http://gerrit.cloudera.org:8080/#/c/23569/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/23569/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@731 PS7, Line 731: .table(authorizable.getTableName()) > Yes. We should set the owner user here. Is there a test that can cover this, i.e. a test that fails without this fix? http://gerrit.cloudera.org:8080/#/c/23569/7/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/23569/7/tests/authorization/test_ranger.py@1489 PS7, Line 1489: admin_client.execute("drop database if exists {0} cascade" > Done Just realized even inserting to columns not used in the masking policies is not allowed. I think it's fine to be consistent with Hive's such behavior. http://gerrit.cloudera.org:8080/#/c/23569/8/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/23569/8/tests/authorization/test_ranger.py@1522 PS8, Line 1522: create_query[1] nit: might be more readable to use a variable like 'tbl', e.g. change L1519 to be for create_stmt, tbl in create_queries: -- To view, visit http://gerrit.cloudera.org:8080/23569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2ef61801d3b394c56702b193c250492a62b111df Gerrit-Change-Number: 23569 Gerrit-PatchSet: 8 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 18 Dec 2025 13:22:32 +0000 Gerrit-HasComments: Yes
