mbutrovich commented on PR #3354: URL: https://github.com/apache/iceberg-rust/pull/3354#issuecomment-6021195550
Thanks for the thorough review @laskoviymishka. I pushed changes for most of the inline comments and replied in each thread, including the ones where I'd like to keep the current behavior. > Before merge I'd want: > - a benchmark (or linked reproducible numbers) backing the perf claim The harness and the PyIceberg script that generates its manifests are in a collapsed section of the description, with the commands to run them. The harness times `Manifest::parse_avro` and prints the best and median of 7 runs in a release build. Pointing its `iceberg` dependency at `main` or at this PR's head commit reproduces the table in the description. I kept it out of the repo because the repo has no `criterion` dependency or `benches/` directory, and @blackmwk asked to keep benchmark harnesses out of the repo for now because of their maintenance cost (https://github.com/apache/iceberg-rust/pull/2558#issuecomment-4873905033). Adding in-repo benchmarks seems like its own discussion. > - confirmation the writer still emits `logicalType: map` in the header it writes [`test_write_manifest_header_marks_int_keyed_maps`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/src/spec/manifest/writer.rs#L886-L902) writes a manifest and finds `"logicalType":"map"` six times in the header's schema JSON, once each for `column_sizes`, `value_counts`, `null_value_counts`, `nan_value_counts`, `lower_bounds`, and `upper_bounds`. The writer builds its Avro schema in code and serializes it without parsing it, so apache/avro-rs#654 doesn't reach it. Details are in the `schema.rs` thread. > - at least one real Java/PyIceberg-written manifest in the fixtures [`pyiceberg-v2-data.avro`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/testdata/manifests/pyiceberg-v2-data.avro) is a manifest written by PyIceberg 0.12.0, and [`test_parse_manifest_written_by_pyiceberg`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/src/spec/manifest/mod.rs#L2016-L2102) compares every field of both entries. The [`testdata/manifests/README.md`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/testdata/manifests/README.md?plain=1) includes the script that generates it. Details are in the `mod.rs` line 99 thread. > Separately, the `file_length` i64 move and the uuid schema change are unrelated to manifest-entry performance and would bisect more cleanly as their own commits. Both come from the version bump rather than the reader change, and only the `file_length` one can be split out. apache-avro 0.22 serializes a `u64` as an 8-byte `fixed` ([`serde/ser.rs#L155-L157`](https://github.com/apache/avro-rs/blob/ec5721cb0c80dcde56c1049a004f1d785abd88cf/avro/src/serde/ser.rs#L155-L157)), where 0.21 serialized it as a `long` ([`ser.rs#L156-L162`](https://github.com/apache/avro-rs/blob/04707999f75278fdea927ee8d2a59de41d8f22a7/avro/src/ser.rs#L156-L162)). With the field left as `u64`, encoding any key metadata that has a file length fails on 0.22 with "Failed to encode key metadata", and [`test_roundtrip_with_length`](https://github.com/apache/iceberg-rust/blob/ecba0b537a1f78ec8125c1a26f6dfab3f95b509e/crates/iceberg/src/encryption/key_metadata.rs#L269-L285), [`test_decode_tolerates_trailing_bytes`](https://github.com/apache/iceberg-rust/blob/ecba0b537a1f78ec8125c1a26f6dfab3f95b509e/crates/iceberg/src/encryption/key_metadata.rs#L354-L383), and [`test_load_manife st_decrypts_when_key_metadata_present`](https://github.com/apache/iceberg-rust/blob/ecba0b537a1f78ec8125c1a26f6dfab3f95b509e/crates/iceberg/src/spec/manifest_list/manifest_file.rs#L284-L306) fail. The type change also works on 0.21. There, the only difference is that a file length above `i64::MAX` returns `DataInvalid` instead of `Unexpected`, because 0.21 already rejects it and already rejects a negative length on decode. Would you like me to move it to a PR that lands on `main` before this one? The uuid changes can't be split out. The only non-test uuid changes rename `Schema::Uuid` to 0.22's `Schema::Uuid(UuidSchema)`, which this PR needs in order to compile. Reading the spec's `fixed(16)` uuid follows from the new reader no longer resolving the writer schema against iceberg-rust's string uuid schema, and [`test_parse_manifest_with_fixed_uuid_partition`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/src/spec/manifest/mod.rs#L1849-L1898) documents that. -- 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]
