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

["gaborkaszab (via GitHub)" <gi...@apache.org>]

gaborkaszab commented on code in PR #6621:
URL: https://github.com/apache/iceberg/pull/6621#discussion_r1097453375


##########
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:
   @haizhou-zhao, sorry for the delay with my answer.
   
   I might miss something here but I don't think that table ownership should be present in the Iceberg Metadata files nor should be a table property, it's rather an attribute of the table in HMS that could be provided during table creation. If you check [this line](https://github.com/apache/iceberg/blob/333227fbd13821365cec1bdbfcb9314a239bea0f/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java#L374) I deliberately remove the HMS_TABLE_OWNER from the table properties because this information should only present for tables in HiveCatalog in HMS. The reason TableMetadata.properties() is used for setting the table ownership is that there was no other way to pass this piece of information to HMS without changing the Iceberg API. But HMS_TABLE_OWNER is eventually not added as an actual table param.
   
   Following this logic I don't think that removing table ownership makes sense as a table should always have an owner. I'd vote for rejecting such requests rather than setting some default value. If the current user wants to grab the ownership of the table then it can change the ownership rather than removing it.
   So in general how I imagine this is to allow changing ownership via setting a table property (but at the end this shouldn't end up as an actual table property) and I'd not allow the user to remove table ownership.



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