gmhelmold opened a new pull request, #2813: URL: https://github.com/apache/iceberg-rust/pull/2813
## Which issue does this PR close? - Closes #2812. ## What changes are included in this PR? The `manifests` metadata table double-counted file entries. Every manifest's added/existing/deleted counts were written into **both** the `*_data_files_count` and the `*_delete_files_count` columns, regardless of the manifest's content type — so a data manifest reported phantom delete-file counts (and a delete manifest would report phantom data-file counts). This gates each of the six count columns by manifest content type, matching Iceberg's reference implementation (`ManifestsTable.java`, `manifestFileToRow`): ```java manifest.content() == ManifestContent.DATA ? manifest.addedFilesCount() : 0, // added_data manifest.content() == ManifestContent.DELETES ? manifest.addedFilesCount() : 0, // added_delete ``` A data manifest now populates only the `*_data_files_count` columns (delete columns = 0), and a delete manifest only the `*_delete_files_count` columns (data columns = 0). No other column is content-type dependent in the reference (`partition_summaries` and the rest stay unconditional), so nothing else changes. Files: `crates/iceberg/src/inspect/manifests.rs` (the fix), `crates/iceberg/src/scan/mod.rs` (test fixture). ## Are these changes tested? Yes — unit tests in `inspect::manifests::tests`. - New test `test_manifests_table_data_and_delete_manifest` drives the fix with a new fixture (`setup_manifest_files_with_delete`) that writes one data manifest **and** one v2 deletes manifest (a single added `PositionDeletes` entry) into the same snapshot's manifest list. The golden pins **both** directions: the data-manifest row reports `*_delete_files_count = 0`, and the delete-manifest row reports `*_data_files_count = 0` (with `added_delete_files_count = 1`). The test fails if either half of the gating regresses. - The existing `test_manifests_table` golden was updated to drop the bug it had baked in: a single data manifest previously asserted `added/existing/deleted_delete_files_count = 1,1,1`; it now correctly asserts `0,0,0`. Red→green verified locally: with the six gated sites reverted to the old unconditional writes, both tests fail (the expect_test diff shows the delete-manifest row's `*_data_files_count` and the data-manifest row's `*_delete_files_count` each expected 0 but got 1); with the fix, both pass. `cargo fmt`, `cargo clippy --all-targets --all-features --workspace -- -D warnings`, the full inspect/datafusion-integration/sqllogictest suites all pass. ## User-facing change Bug fix. The `manifests` metadata table now reports correct per-content-type file counts. -- 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]
