Hugo-WB commented on code in PR #16353:
URL: https://github.com/apache/iceberg/pull/16353#discussion_r4004296056
##########
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:
Hey! Sorry for the slowness here. Thanks for the review!
I think this is a valid point! And definitely does break the current
implementation. It also does seem to be used
[here](https://github.com/apache/iceberg/blob/c07a081c8ab8687cd0531101db64922f6dbb2de6/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L546),
in the exact case where SnapshotMode::REFS, so the rest catalog would've ran
`suppressHistoricalSnapshots`.
Downside here is that `SnapshotMode::REFS` will now return a lot of the
encryption keys 😠and we are not necessarily reducing the size of the
loadtable RPC.
I do feel `setSnapshotsSupplier`'s API does seem to be a bit at odds with
the way encryption keys are set up within the TableMetadata 😅 . If the goal of
it is to supply snapshots that aren't currently within metadata, we should
figure out how it can also supply encryption keys for those snapshots. Curious
as to why manifest list encryption keys are stored at the metadata layer and
not at the snapshot layer.
I've moved the `removeEcnryptionKey` into `!suppress` block so that we don't
remove encryption keys for now. Lmk what you think!
Updated:
https://github.com/Hugo-WB/iceberg/commit/d78c75291aceca6de386a44cf4d6156afe717d89
--
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]
