Fang-Yu Rao 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 18: (1 comment) http://gerrit.cloudera.org:8080/#/c/23569/14/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/14/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@156 PS14, Line 156: // We allow the ALL privilege because for the UPSERT operation against Kudu : // tables, we set the required privilege to ALL since we don't have an UPSERT : // privilege yet. Refer to InsertStmt#analyzeTargetTable(). : // CREATE privilege would be registered in CreateTableAsSelectStmt#analyze(). : // In addition, VIEW_METADATA privilege would be registered in : // CopyTestCaseStmt#analyze(). : Preconditions.checkState(ALLOWED_HIER_AUTHZ_TABLE_PRIVILEGES : .contains(privReq.getPrivilege())); : requests = tablePrivReqs.computeIfAbsent(tableName, k -> new ArrayList<>()); : requests.add(privReq); : } else { : Preconditions.checkState(privReq.getAuthorizable().getType() == Type.COLUMN); : requests = columnPrivReqs.computeIfAbsent(ta > The revised condition suggested by me above would actually fail the followi I am also thinking about why we added this Preconditions check in the first place. One reason I could think of right now is that by ensuring that the table-level privilege request always precedes the column-level privilege requests for a given table, we could save some time spent on authorization via short circuiting. That is, when the requesting user is granted the required privilege on the given table, we could just skip the authorization of the columns in this table. This was true until https://github.com/apache/impala/commit/b37dd05e8f35bba5d9126ef79b35c1831f966f1b (IMPALA-8933: Enforce ranger deny policy). However, it's no longer clear to me if we still need this Preconditions check after IMPALA-8933. I could try to keep this Preconditions check, but just want to point out that removing this check could possibly be an option too. -- 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: 18 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: Tue, 13 Jan 2026 02:57:53 +0000 Gerrit-HasComments: Yes
