ewoodbury opened a new pull request, #3566: URL: https://github.com/apache/iceberg-rust/pull/3566
## Which issue does this PR close? - Closes #3565. ## What changes are included in this PR? When Java writes a manifest whose partition field name isn't a valid Avro name, it stores the field under a sanitized name. For example, `bucket(16, user.id)` is named `user.id_bucket` and written as `user_x2Eid_bucket`. iceberg-rust matched manifest partition fields to the spec by name, so these values read as null with no error. Scan planning then evaluated partition filters against null, and a filter on the partition's source column skipped every file. The reach is wider than pruning: `DeleteFileIndex` keys equality deletes by `data_file.partition()`, so with every partition read as null an equality delete applies to data files in every partition. The `_partition` column and the inspect tables also read null. This PR matches partition fields by `field-id`, which the spec requires on every Avro field and which Java and PyIceberg use. Once per manifest, the reader renames the partition type's fields to the writer's Avro names, looked up by `field-id` in the writer schema. The existing name-based projection then finds each value. Only the partition record needs this: the other manifest fields have fixed names that serde already handles. This is the follow-up from the #3354 review ([comment](https://github.com/apache/iceberg-rust/pull/3354#discussion_r4204959679)), listed in #3262 as "Match manifest partition fields by `field-id` before name". The write side, where iceberg-rust writes the invalid name as is, is #2535 and isn't changed here. Behavior changes: - Partition values that a writer stored under a different name now read correctly when the field IDs match. - If the writer's partition fields have field IDs, a spec field with no matching ID reads as null, even when a writer field has the same name. Java also matches by ID here. On `main` it read the value with the same name. - If the writer's partition fields have no field IDs, they still match by name, as on `main`. Java reads these as null, because a manifest entry always has field IDs. No writer I know of drops them only on the partition fields, so I kept the current behavior. ## Are these changes tested? Yes: - `test_parse_manifest_written_by_java_with_sanitized_partition_name` reads a V1 and a V2 manifest written by iceberg-java 1.12.0 with `bucket(16, user.id)`, added under `testdata/manifests/` along with the Java program that regenerates them. The partition value reads as 3. On `main` it reads as null. - `test_parse_manifest_matches_partition_fields_by_field_id` covers three writer schemas: fields in spec order with two fields' IDs swapped, a field whose name matches the spec but whose ID doesn't, and fields without IDs. The first two fail on `main`. The third passes on `main` and pins the name fallback. I checked each part of the fix by breaking it and confirming a test fails: ignoring field IDs, falling back to the name per field, dropping the name fallback when there are no IDs, and passing the spec's names on the V1 path only. I also read tables written by iceberg-java 1.12.0 at format versions 1 and 2 with `bucket(16, user-id)`. On `main` every partition reads as null and `user-id = 3` returns no rows. With this PR it returns the one matching row, as Java does. These pass locally: - `cargo test -p iceberg --lib --all-features` - `cargo clippy -p iceberg --all-targets --all-features -- -D warnings` - `cargo fmt --all -- --check` - `RUSTDOCFLAGS="-D warnings" cargo doc -p iceberg --no-deps --all-features` - `cargo machete` - `python3 ./dev/release/licenses.py crates check` ## AI Disclosure Drafted with AI assistance (Grok and Claude). I fully own the fix strategy and its correctness. -- 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]
