gaborkaszab commented on code in PR #16353:
URL: https://github.com/apache/iceberg/pull/16353#discussion_r3714854173
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1451,6 +1451,7 @@ private Builder rewriteSnapshotsInternal(Collection<Long>
idsToRemove, boolean s
}
removeStatistics(snapshotId);
removePartitionStatistics(snapshotId);
+ removeEncryptionKey(snapshot.keyId());
Review Comment:
I gave the PR to Claude and it might have found an issue with the current
design:
`suppressHistoricalSnapshots()` method on `TableMetadata.Builder` may
collide with the approach here. Let's say we create `TableMetadata` like below
setting up lazy snapshot loading (considering 2 snapshots, a current and a
historical one):
```
TableMetadata lazyMetadata =
TableMetadata.buildFrom(metadata)
.suppressHistoricalSnapshots()
.setSnapshotsSupplier(() -> ImmutableList.of(historical,
current))
.discardChanges()
.build();
```
It will have only the referenced snapshots and omit the rest. Due to the
current implementation it removes encryption keys for the unloaded snapshots.
When we load the rest of the snapshots through `snapshotSupplier` we won't have
encryption keys for all the snapshots, so time travel to such snapshots would
fail.
Whether this is a realistic scenario or not, I can't judge, but you can
repro this through the public API of the `TableMetadata` builder.
WDYT?
--
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]
