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 14: (3 comments) Hi all, I have addressed Quanlong's comments on patch set 13. Let me know if there are additional suggestions. Thanks! http://gerrit.cloudera.org:8080/#/c/23569/13/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java File fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java: http://gerrit.cloudera.org:8080/#/c/23569/13/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@256 PS13, Line 256: 4, events.size()); : assertEventEquals("@column", "insert", "functiona > nit: this long string can be extracted as a variable. Sure. I will make this a variable in the next patch. http://gerrit.cloudera.org:8080/#/c/23569/13/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@458 PS13, Line 458: "values (1, 1)", events.get(0).getRequestData()); Thanks Quanlong! Based on my observation, we deal with non-existing tables and non-existing columns differently. We register the privilege request for a table whether or not the table exists, whereas we register the privilege request for a column only if the column exists. The details are provided as follows. In Analyzer#getTable() (https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/Analyzer.java), we register the table-level privilege request regardless of whether the table exists. Specifically, getTableNoThrow() won't throw an exception even though the table does not exist. The exception will be thrown from getTable(fqTableName.getDb(), fqTableName.getTbl(), /* must_exist */ true) for a non-existing table. public FeTable getTable(TableName tableName, boolean addAccessEvent, boolean addColumnPrivilege, Privilege... privilege) throws AnalysisException, TableLoadingException { ... FeTable table = getTableNoThrow(fqTableName.getDb(), fqTableName.getTbl()); final String tableOwner = table == null ? null : table.getOwnerUser(); for (Privilege priv : privilege) { if (priv == Privilege.ANY || addColumnPrivilege) { registerPrivReq(builder -> builder.allOf(priv) .onAnyColumn(fqTableName.getDb(), fqTableName.getTbl(), tableOwner) .build()); } else { registerPrivReq(builder -> builder.allOf(priv) .onTable(fqTableName.getDb(), fqTableName.getTbl(), tableOwner) .build()); } } // Propagate the AnalysisException if the table/db does not exist. table = getTable(fqTableName.getDb(), fqTableName.getTbl(), /* must_exist */ true); ... return table; } On the other hand, in InsertStmt#analyze() (https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java), we register the column-level privilege request for a column only if the column exists. public void analyze(Analyzer analyzer) throws AnalysisException { ... Column column = table_.getColumn(columnName); if (column == null) { throw new AnalysisException( "Unknown column '" + columnName + "' in column permutation"); } ... analyzer.registerPrivReq(builder -> builder .allOf(Privilege.INSERT) .onColumn(table_.getTableName().getDb(), table_.getTableName().getTbl(), column.getName(), table_.getOwnerUser()).build()); ... } > How about the case when a column does not exist? I will add a few new test cases for this scenario in the next patch. 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@1496 PS7, Line 1496: > Thanks for the explanation! Ack -- 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: 14 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: Fri, 02 Jan 2026 23:41:57 +0000 Gerrit-HasComments: Yes
