rdblue commented on code in PR #8096:
URL: https://github.com/apache/iceberg/pull/8096#discussion_r1276920154
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1646,7 +1674,7 @@ private static List<HistoryEntry> updateSnapshotLog(
List<HistoryEntry> newSnapshotLog = Lists.newArrayList();
for (HistoryEntry logEntry : snapshotLog) {
long snapshotId = logEntry.snapshotId();
- if (snapshotsById.containsKey(snapshotId)) {
+ if (snapshotsById.containsKey(snapshotId) ||
lazySnapshotLoadingEnabled) {
Review Comment:
Right now, I think all we need to do is detect whether we need to alter the
snapshot history at all:
```java
boolean hasRemovals =
changes(MetadataUpdate.RemoveSnapshot.class).findAny().isPresent();
```
That will tell us whether any snapshots were removed from history and
whether we need to clear history when a snapshot is missing. If there aren't
removals, then we should use this branch.
Alternatively, you could detect whether the snapshot supplier is non-null
because that indicates that some snapshots are missing. But I'm concerned that
if we do that, then when we fix the current behavior that loads all snapshots
at write time we will potentially keep extra history that doesn't correspond to
a snapshot. I'd rather only keep history if we know it is safe to do so because
there weren't any removed snapshots.
--
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]
