Re: [PR] Core: Clean up encryption keys as part of RemoveSnapshots [iceberg]

["gaborkaszab (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org