haizhou-zhao commented on code in PR #6045:
URL: https://github.com/apache/iceberg/pull/6045#discussion_r1020581546


##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -365,6 +374,13 @@ public boolean dropNamespace(Namespace namespace) {
 
   @Override
   public boolean setProperties(Namespace namespace, Map<String, String> 
properties) {
+    Preconditions.checkArgument(
+        (properties.get(HMS_DB_OWNER_TYPE) == null) == 
(properties.get(HMS_DB_OWNER) == null),
+        "Setting "

Review Comment:
   See my other comments for why I think preconditions for createNamespace and 
setProperty are different.



##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -365,6 +374,13 @@ public boolean dropNamespace(Namespace namespace) {
 
   @Override
   public boolean setProperties(Namespace namespace, Map<String, String> 
properties) {
+    Preconditions.checkArgument(
+        (properties.get(HMS_DB_OWNER_TYPE) == null) == 
(properties.get(HMS_DB_OWNER) == null),

Review Comment:
   If your table originally have group ownership, and imagine you set property 
only on owner but not explicitly on owner type for this particular table, then 
it will be hard to define what exactly the user wants in this case. Does the 
user want to switch ownership to a different group, or does the user want to 
switch to individual ownership under a different person's name? I believe it's 
better not guess user intention in this case and force the users to be explicit 
about exactly what ownership they want to switch to both in terms of owner and 
owner type.
   
   In summary, I think createNamespace and setProperty are different because we 
can assume that when user calls createNamespace, then the namespace never 
existed before, nor is there a previous ownership (and it's easier to say, 
under this circumstances that "if no owner type specificed, then take 
owner-type=USER as default"). With setProperty, we do have to consider what 
state of ownership is there before the change.



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

Reply via email to