zhengxiang378928908-code commented on PR #18224: URL: https://github.com/apache/iceberg/pull/18224#issuecomment-6354437236
Adding an expired-history case to @JeonDaehong's finding: in a v3/v4 probe on this branch, replacing a DV with `newRewrite()` without `validateFromSnapshot` failed with `Cannot determine history between starting snapshot null and the last known ancestor 2` after the oldest snapshot was expired. The same scenario succeeds with the current `BaseRewriteFiles`. On this branch it also succeeds when I pass the head the rewrite was planned against, with no commit in between. On the callers: `BaseRewriteDataFilesAction`, `RewriteDataFilesCommitManager` and `RewritePositionDeletesCommitManager` pass a starting snapshot. `RemoveDanglingDeletesSparkAction` calls `newRewrite()` without one on this PR's base (on current `main` it goes through `RemoveDanglingDeleteFilesAction`, which sets one), but it only removes delete files, so the new DV check returns early either way. The javadoc already says that without a starting snapshot all ancestors are validated. If this behavior is intended, would it help to add a DV-specific note there (pass the snapshot the rewrite was planned against), and cover the expired-history case in a test? -- 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]
