vbhanuchander-lang commented on PR #17604: URL: https://github.com/apache/iceberg/pull/17604#issuecomment-5251049708
@uros-b Yes — this duplicates #16645, and that one came first. Closing this in favour of it. @wombatu-kun apologies for the noise. Your PR predates mine by ten weeks and reaches the same conclusion on both defects, including the `apply(base, snapshot)` vs `base.currentSnapshot()` one. On the tests you also went further than I did: asserting manifest *contents* through `validateManifestEntries` with the file-order handling, where I only asserted counts. For the record on how I missed it: I checked issue #15981's body and comments for linked pull requests and found only the closed #15982, so I did not look at the issue's cross-reference timeline where yours appears. That was my error, and the lesson is to search open PRs for the feature before writing any of it. Two small things from my version that yours does not have, which I have left as review comments on #16645 rather than keeping a competing PR open: 1. no test for the genuinely-null snapshot, i.e. an empty table with no snapshots at all — your code handles it correctly, it just is not pinned 2. `validateDeletedManifests(..., -1L)` yields "could not be found in the latest snapshot -1", which is a confusing message for the no-snapshot case Happy to help get yours over the line if useful. -- 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]
