gaborkaszab commented on code in PR #6621:
URL: https://github.com/apache/iceberg/pull/6621#discussion_r1081178346
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -494,6 +494,17 @@ private void setHmsTableParameters(
// remove any props from HMS that are no longer present in Iceberg table
props
obsoleteProps.forEach(parameters::remove);
+ // altering owner
+ if (metadata.properties().get(HiveCatalog.HMS_TABLE_OWNER) != null) {
+ tbl.setOwner(metadata.properties().get(HiveCatalog.HMS_TABLE_OWNER));
+ }
+
+ // dropping owner: instead of leaving the owner blank/null, the owner will
be
+ // default to whoever is making the current drop operation
+ if (obsoleteProps.contains(HiveCatalog.HMS_TABLE_OWNER)) {
Review Comment:
I wondering if the "drop ownership" use-case makes any sense. In case I'm
not satisfied with the current owner of a table instead of dropping the owner I
could assign it to someone (e.g. myself). I feel that setting the owner to the
user who runs the updateProperties() query seems more like a side-effect rather
than what a user might expect.
However, I'm open for other opinions here.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -328,6 +329,140 @@ public void testCreateTableCustomSortOrder() throws
Exception {
}
}
+ @Test
+ public void testAlterTableOwner() throws IOException, TException {
+ alterTableAndVerifyOwner(
+ DB_NAME,
+ "tbl_alter_owner_1",
+ ImmutableMap.of(HiveCatalog.HMS_TABLE_OWNER, "some_owner"),
+ ImmutableMap.of(HiveCatalog.HMS_TABLE_OWNER, "some_other_owner"),
+ "some_owner",
+ "some_other_owner");
+ alterTableAndVerifyOwner(
+ DB_NAME,
+ "tbl_alter_owner_2",
+ ImmutableMap.of(),
+ ImmutableMap.of(HiveCatalog.HMS_TABLE_OWNER, "another_owner"),
+ UserGroupInformation.getCurrentUser().getUserName(),
+ "another_owner");
+ alterTableAndVerifyOwner(
+ DB_NAME,
+ "tbl_alter_owner_noop_1",
+ ImmutableMap.of(HiveCatalog.HMS_TABLE_OWNER, "some_owner"),
+ ImmutableMap.of(),
+ "some_owner",
+ "some_owner");
+ alterTableAndVerifyOwner(
+ DB_NAME,
+ "tbl_alter_owner_noop_2",
+ ImmutableMap.of(HiveCatalog.HMS_TABLE_OWNER, "some_owner"),
+ ImmutableMap.of("unrelated_prop", "val"),
+ "some_owner",
+ "some_owner");
+ alterTableAndVerifyOwner(
+ DB_NAME,
+ "tbl_alter_owner_noop_3",
+ ImmutableMap.of(),
+ ImmutableMap.of(),
+ UserGroupInformation.getCurrentUser().getUserName(),
+ UserGroupInformation.getCurrentUser().getUserName());
+ }
+
+ private void alterTableAndVerifyOwner(
+ String db,
+ String tbl,
+ Map<String, String> properties,
+ Map<String, String> updates,
+ String expectedOwnerPostCreate,
+ String expectedOwnerPostAlter)
+ throws IOException, TException {
+ Schema schema = getTestSchema();
+ PartitionSpec spec = PartitionSpec.builderFor(schema).bucket("data",
16).build();
+ TableIdentifier tableIdent = TableIdentifier.of(db, tbl);
+ String location = temp.newFolder(tbl).toString();
+ try {
+ Table table = catalog.createTable(tableIdent, schema, spec, location,
properties);
+ org.apache.hadoop.hive.metastore.api.Table hmsTable =
metastoreClient.getTable(db, tbl);
+ Assert.assertEquals(expectedOwnerPostCreate, hmsTable.getOwner());
+
Assert.assertFalse(hmsTable.getParameters().containsKey(HiveCatalog.HMS_TABLE_OWNER));
+ UpdateProperties updateOps = table.updateProperties();
+ updates.forEach(updateOps::set);
+ updateOps.commit();
+ hmsTable = metastoreClient.getTable(db, tbl);
+ Assert.assertEquals(expectedOwnerPostAlter, hmsTable.getOwner());
+
Assert.assertFalse(hmsTable.getParameters().containsKey(HiveCatalog.HMS_TABLE_OWNER));
+ } finally {
+ catalog.dropTable(tableIdent);
+ }
+ }
+
+ @Test
+ public void testRemoveTableOwner() throws IOException, TException {
+ removeTableOwnerAndVerify(
+ DB_NAME,
+ "tbl_remove_owner_1",
+ ImmutableMap.of(HiveCatalog.HMS_TABLE_OWNER, "some_owner"),
+ ImmutableSet.of(HiveCatalog.HMS_TABLE_OWNER),
+ "some_owner",
+ UserGroupInformation.getCurrentUser().getUserName());
+ removeTableOwnerAndVerify(
+ DB_NAME,
+ "tbl_remove_owner_noop_1",
+ ImmutableMap.of(),
+ ImmutableSet.of(HiveCatalog.HMS_TABLE_OWNER),
+ UserGroupInformation.getCurrentUser().getUserName(),
+ UserGroupInformation.getCurrentUser().getUserName());
+ removeTableOwnerAndVerify(
+ DB_NAME,
+ "tbl_remove_owner_noop_2",
+ ImmutableMap.of(HiveCatalog.HMS_TABLE_OWNER, "some_owner"),
+ ImmutableSet.of(),
+ "some_owner",
+ "some_owner");
+ removeTableOwnerAndVerify(
+ DB_NAME,
+ "tbl_remove_owner_noop_3",
+ ImmutableMap.of(HiveCatalog.HMS_TABLE_OWNER, "some_owner"),
+ ImmutableSet.of("unrelated_prop"),
+ "some_owner",
+ "some_owner");
+ removeTableOwnerAndVerify(
+ DB_NAME,
+ "tbl_remove_owner_noop_4",
+ ImmutableMap.of(),
+ ImmutableSet.of(),
+ UserGroupInformation.getCurrentUser().getUserName(),
+ UserGroupInformation.getCurrentUser().getUserName());
+ }
+
+ private void removeTableOwnerAndVerify(
Review Comment:
nit: this seems almost identical to alterTableAndVerifyOwner except 1 line
that decides if we are altering or removing. Can you please merge these two
function together and make the updateProperties operation pluggable or being
decided by some parameter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
