gaborkaszab commented on code in PR #6621:
URL: https://github.com/apache/iceberg/pull/6621#discussion_r1100359365
##########
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:
Thanks for the heads-up, @haizhou-zhao!
I did some debugging and HMS_TABLE_OWNER indeed makes it into the
TableMetadata not just to HMS. However, when I introduced this property my
intention was to use it only for table creation to pass this to HMS and not to
make it part of the table properties nor the table metadata. This is the reason
I explicitly remove it from the table properties
[here](https://github.com/apache/iceberg/blob/eeb055a37705b8c8ff1e13acefe0ba24382c66fe/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java#L374).
The reason this had to be provided as a table property is that this approach
didn't require any Iceberg API changes, and the purpose was to pass it as table
property, give it to HMS during table creation and then drop from the table
properties as it have never existed there. Unfortunately as I see, as a side
effect it made it into table metadata, that is unintentional (nice catch, btw).
So long story short, I don't think that any of the Iceberg clients use
HMS_TABLE_OWNER as a table metadata at all. I'm certain that it is not used for
determining the owner of the table (I'm not sure how other catalogs keep track
of this info, if they do at all).
So that's why I was confused why we want to keep the owner in the metadata
sync with the one in HMS because I only intended to add this piece of info into
HMS and wasn't aware that accidentally I added it to the metadata as well.
I'm wondering if it makes sense to get rid of HMS_TABLE_OWNER from the table
metadata? It would make altering the owner difficult, right?
A remark on the "drop ownership" part: well, it might be possible to drop
other properties, but I still feel that dropping ownership simply doesn't make
any sense especially that after dropping we have to set something as an owner
and regardless of whoever we set, it feels like a side effect. I'd vote for not
allow dropping 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
