[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6621: [HiveCatalog] Support Altering and Dropping Table Ownership

[GitBox <gi...@apache.org>]

szehon-ho commented on code in PR #6621:
URL: https://github.com/apache/iceberg/pull/6621#discussion_r1081718764


##########
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(

Review Comment:
   Nit: I think in most tests, we put the private at end of file?  So the ```@Test``` are all in the top and easier to find.



##########
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:
   Also too don't know of the use-case, but makes sense to me to set it to null here.



-- 
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-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org