amogh-jahagirdar commented on code in PR #12670:
URL: https://github.com/apache/iceberg/pull/12670#discussion_r2019189952
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1436,12 +1436,14 @@ public Builder removeSnapshots(Collection<Long>
idsToRemove) {
private Builder rewriteSnapshotsInternal(Collection<Long> idsToRemove,
boolean suppress) {
List<Snapshot> retainedSnapshots =
Lists.newArrayListWithExpectedSize(snapshots.size() -
idsToRemove.size());
+ Set<Long> removeSnapshotIds = Sets.newHashSet();
Review Comment:
Could we rename this to `removedSnapshotIds` ? I think the past tense better
indicates that these are snapshots that are actually removed and produced in
the change set
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -419,18 +420,18 @@ public void testAddSnapshotFromJson() throws IOException {
@Test
public void testRemoveSnapshotsFromJson() {
String action = MetadataUpdateParser.REMOVE_SNAPSHOTS;
- long snapshotId = 2L;
- String json = String.format("{\"action\":\"%s\",\"snapshot-ids\":[2]}",
action);
- MetadataUpdate expected = new MetadataUpdate.RemoveSnapshot(snapshotId);
+ Set<Long> snapshotIds = Sets.newHashSet(2L, 3L);
Review Comment:
+1. Once we update the PR to add back the existing RemoveSnapshot as
deprecated we should leave these test cases as is, and new ones for the new
path. Down the line once we remove the deprecated class, we could go ahead and
update this but for now I'd leave 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: [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]
