advancedxy commented on code in PR #10755:
URL: https://github.com/apache/iceberg/pull/10755#discussion_r1697200471
##########
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:
> I might be missing something, so all the metadata changes have to be sent
to the REST service? I didn't work with a REST catalog before and don't see how
changes are sent to the REST service. It would be great that some reference or
code could be pointed to.
I took a look at the REST catalog related code today, it seems the impl in
this PR doesn't work with Iceberg tables backend by REST catalog as there's no
update added for RemovePartitionSpec. The `RemoveUnusedSpecs` will succeed
without actually removing unused specs for REST catalog.
If supporting REST catalog is a must requirement, I think we have to go
through a REST spec change to add new update type to reflect that. The only
concern is how to enforce the removed spec is indeed not used any more? Do all
the similar calculation in `org.apache.iceberg.rest.CatalogHandlers#commit`
like how we did in `BaseRemoveUnusedSpecs`? Or is that necessary?
WDYT? @RussellSpitzer @rdblue @amogh-jahagirdar
--
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
