amogh-jahagirdar commented on code in PR #12670:
URL: https://github.com/apache/iceberg/pull/12670#discussion_r2019028754
##########
core/src/main/java/org/apache/iceberg/MetadataUpdate.java:
##########
@@ -328,20 +328,20 @@ public void applyTo(TableMetadata.Builder
metadataBuilder) {
}
}
- class RemoveSnapshot implements MetadataUpdate {
- private final long snapshotId;
+ class RemoveSnapshots implements MetadataUpdate {
+ private final Set<Long> snapshotIds;
- public RemoveSnapshot(long snapshotId) {
- this.snapshotId = snapshotId;
+ public RemoveSnapshots(Set<Long> snapshotIds) {
+ this.snapshotIds = snapshotIds;
}
- public long snapshotId() {
- return snapshotId;
+ public Set<Long> snapshotIds() {
+ return snapshotIds;
}
@Override
public void applyTo(TableMetadata.Builder metadataBuilder) {
- metadataBuilder.removeSnapshots(ImmutableSet.of(snapshotId));
+ metadataBuilder.removeSnapshots(snapshotIds);
}
Review Comment:
@ricardopereira33 Ah you are completely right I think, I misread the
protocol, `RemoveSnapshots`
https://github.com/apache/iceberg/blame/main/open-api/rest-catalog-open-api.yaml#L2833
already operates on a list of snapshots. it's seems like the implementation
should actually be more open. Currently looks like there's some strict
validation that there's only 1 element
https://github.com/apache/iceberg/blame/main/core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java#L559
I'm not sure the context on this. But fundamentally I think you are correct
there's no protocol violation here. I'd need to see why this was implemented
this way to begin with I also see this ToDo
https://github.com/apache/iceberg/blame/main/core/src/main/java/org/apache/iceberg/MetadataUpdateParser.java#L420
For general library compatibility though we probably need to keep this
update type, potentially deprecating it in favor of the new one.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
