amogh-jahagirdar commented on code in PR #10755:
URL: https://github.com/apache/iceberg/pull/10755#discussion_r1697317660


##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1425,6 +1460,7 @@ private boolean hasChanges() {
           || (discardChanges && !changes.isEmpty())
           || metadataLocation != null
           || suppressHistoricalSnapshots
+          || hasRemovedSpecs

Review Comment:
   > The RemoveUnusedSpecs will succeed without actually removing unused specs 
for REST catalog.
   @advancedxy This indicates there's a problem with the current implementation 
then imo. If there was a way to avoid the REST spec change I think it would've 
been OK as long as we can guarantee failure on the client side until the 
support was added for the metadata update type. But I think that's unavoidable, 
and I agree with @rdblue that we should probably just add the metadata update 
type. 
   
   I also am reasonably confident that a REST catalog can safely handle this 
RemovePartitionSpec update.  



##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1425,6 +1460,7 @@ private boolean hasChanges() {
           || (discardChanges && !changes.isEmpty())
           || metadataLocation != null
           || suppressHistoricalSnapshots
+          || hasRemovedSpecs

Review Comment:
   > The RemoveUnusedSpecs will succeed without actually removing unused specs 
for REST catalog.
   
   @advancedxy This indicates there's a problem with the current implementation 
then imo. If there was a way to avoid the REST spec change I think it would've 
been OK as long as we can guarantee failure on the client side until the 
support was added for the metadata update type. But I think that's unavoidable, 
and I agree with @rdblue that we should probably just add the metadata update 
type. 
   
   I also am reasonably confident that a REST catalog can safely handle this 
RemovePartitionSpec update.  



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