[GitHub] [iceberg] rdblue commented on a change in pull request #2603: Core: Deal with expired parent snapshot in MergingSnapshotProducer#validateDataFilesExist

[GitBox <gi...@apache.org>]

rdblue commented on a change in pull request #2603:
URL: https://github.com/apache/iceberg/pull/2603#discussion_r652149167



##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -314,16 +315,29 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
           "Cannot determine history between starting snapshot %s and current %s",
           startingSnapshotId, currentSnapshotId);
 
+      // if current snapshot's parent was expired and starting snapshot is null,
+      // add all manifests of current snapshot and break
+      Long parentId = currentSnapshot.parentId();
+      boolean shouldAddManifestsAndBreak =
+          startingSnapshotId == null && parentId != null && ops.current().snapshot(parentId) == null;

Review comment:
       I think that the logic using `shouldAddManifestsAndBreak` is incorrect. The situation this is trying to account for is when the entire table history should be considered (`startingSnapshotId` is `null`), but only partial history is available (`parentId` is not `null` but the snapshot is missing). I think the idea here is that this can account for lost history by looking across all manifests for a delete.
   
   I don't think that this is implemented correctly for a few reasons:
   * If the oldest available snapshot is an append snapshot, then the outer check below for operation will prevent its manifests from being added as expected.
   * The manifest group that scans for deletes filters entries to just the snapshot ids that were new since the starting id (`newSnapshots`). When history is missing, that filter would need to be turned off or else the entries will be ignored even if you scan older manifests.
   * When reading the manifests to check for deletes, missing history could have removed those deletes in a manifest rewrite. If I delete file A, it will show up in a delete entry in a snapshot. But once I rewrite the manifest that encoded that delete, the delete is no longer kept. So expiring history can still cause a problem.
   
   Because of that last flaw, I don't think that there is a way to do this safely by searching for deletes. The only way to do this validation without history is to scan the entire table for the required data files.




-- 
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.

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