advancedxy commented on code in PR #10755:
URL: https://github.com/apache/iceberg/pull/10755#discussion_r1697807013
##########
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:
> We don't currently have operations that can't be supported by the REST
protocol, so this is an area where we should be careful.
Yes, after taking a look at the related code, I think we should strive to
make all operations supported by REST protocol.
> It's unlikely that a new snapshot would be written with a spec that is
being removed because this already validates that the default spec is not being
removed. A conflict here would require that a concurrent writer is using a spec
other than the default or has changed the default spec. There's already a
validation for the second case, assert-default-spec-id. For the first case,
this could require that no branch states have changed using
assert-ref-snapshot-id.
Thanks for the inspiring explanation. I wasn't worrying about the normal
path, which I am also confident that REST catalog can safely handle. I'm more
concerned of misusage, such that users issue a RemovePartitionSpec request
without going through the `RemoveUnusedSpec` API or accidentally call
`TableMetadata.Builder.removePartitionSpecs` directly. That's why there's
method defined in `TableMetadata` class in the first place, to hide the
Builder's method and ensure single point of access.
For the REST catalog, as it's open by default to all kinds of clients. It's
more likely to be affected by the misusage. That's why I'm proposing in the
catalog handler side do the check as well:
```
Do all the similar calculation in
org.apache.iceberg.rest.CatalogHandlers#commit like how we did in
BaseRemoveUnusedSpecs?
```
However, it's heavy operation, does that worth it?
--
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
