[GitHub] [iceberg] danielcweeks commented on a change in pull request #1156: Add Spark 3 SQL tests

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

danielcweeks commented on a change in pull request #1156:
URL: https://github.com/apache/iceberg/pull/1156#discussion_r451838028



##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -110,9 +110,17 @@ public Transaction newReplaceTableTransaction(
       throw new NoSuchTableException("No such table: " + identifier);
     }
 
-    String baseLocation = location != null ? location : defaultWarehouseLocation(identifier);
     Map<String, String> tableProperties = properties != null ? properties : Maps.newHashMap();
-    TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, baseLocation, tableProperties);
+
+    TableMetadata metadata;
+    if (ops.current() != null) {

Review comment:
       I'm not clear on whether this really should be the right behavior.  Basically we're saying that a replace table will keep the existing location (as opposed to using defaults).  I suspect we don't have create or replace with location semantics, but this is making some assumptions that a replacement is somehow the same as the old.  If we were to go with id based pathing convention, this wouldn't work.
   
   I don't think this is an issue at this point, but it might make sense to push this down to the location provider.




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

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