mbutrovich opened a new issue, #3262: URL: https://github.com/apache/iceberg-rust/issues/3262
### What's the feature are you trying to implement? PyIceberg wants to read manifests and manifest lists through `pyiceberg-core` instead of its Cython Avro decoder. Today the `pyiceberg-core` path is 4x to 5x slower than Cython end to end. This issue tracks the iceberg-rust work needed to make it faster. The overall effort, including the PyIceberg tasks, is tracked in apache/iceberg-python#TBD. This issue falls under the PyIceberg integration epic #1694 and revisits #1280. The full benchmark writeup from @kevinjqliu, including an in-crate profile of `Manifest::parse_avro` and the script, is in https://github.com/kevinjqliu/iceberg-python/issues/45. #### Where the time goes These numbers come from manifests written by PyIceberg (V2, deflate, 12 columns with full column stats, identity partition). `pyiceberg-core` is built locally from `v0.10.1`, once as released (`opt-level = "z"`) and once with `opt-level = 3`. Each number is the best of 7 runs in ms, single threaded, on Apple Silicon. `v0.10.1` predates #3028, which its PR measured as making `Manifest::parse_avro` about 5% faster. | entries | Cython | `read_manifest_entries` (`"z"`) | end to end (`"z"`) | `read_manifest_entries` (`3`) | end to end (`3`) | |--:|--:|--:|--:|--:|--:| | 1,000 | 8.7 | 35.3 | 43.9 | 22.2 | 30.8 | | 10,000 | 94.1 | 351.0 | 452.3 | 217.4 | 320.4 | | 50,000 | 565.9 | 1750.6 | 2308.7 | 1093.7 | 1628.0 | "End to end" includes building PyIceberg `ManifestEntry` and `DataFile` objects from the binding's getters, which is what the Cython path produces. Two separate costs have to go for `pyiceberg-core` to beat Cython: - The Rust parse costs 21.7 us per entry at `opt-level = 3`, against 10.3 us per entry for the whole Cython path. The linked profile puts about 82% of the parse inside `apache-avro`, in decoding into `apache_avro::Value` and then resolving against the reader schema. Resolution always runs, because the reader schema iceberg-rust generates never equals a PyIceberg or Java writer schema. - Converting the binding's output into PyIceberg objects costs another 10.3 us per entry, as much as Cython. That cost doesn't depend on how fast the Rust parser is. #### Task list - [ ] **Task 1: `apache-avro` 0.22 and a byte-level manifest reader.** Upgrade to 0.22 and replace the `Value`-based reader with one that deserializes entries straight from the writer schema (`SchemaAwareDeserializer`, apache/avro-rs#512), with behavior identical to the current reader. Discussion is in #3063. @kevinjqliu has this working locally. #3224 (open) already stops buffering the whole manifest list as `Value`s before converting it, so it overlaps with this task for manifest lists. - [ ] **Task 2: Build the `pyiceberg-core` wheel with `opt-level = 3`.** The release profile is set in [`bindings/python/pyproject.toml#L57-L62`](https://github.com/apache/iceberg-rust/blob/832a4eba7a732baebbbd7f76bb6fd6225f1db1e9/bindings/python/pyproject.toml#L57-L62), where #1841 chose `"z"` for wheel size. It could also use `opt-level = 2`, or `"z"` everywhere except the hot crates. Building `v0.10.1` with `opt-level = 3` showed three problems. I haven't checked them on `main`. - `iceberg-storage-opendal` fails to compile with "queries overflow the depth limit" while computing the layout of an `opendal_core` async fn body. Raising the crate's recursion limit to 256 fixes it. - With `strip = true`, the resulting `.so` fails to load on macOS with "mis-aligned LINKEDIT string pool". It loads with `strip = false`. - The macOS arm64 wheel grows from 12.6 MB to 21.6 MB. - [ ] **Task 3: Manifest entries and manifest lists as Arrow.** Build an Arrow `RecordBatch` from parsed manifest entries in the `iceberg` crate, alongside the existing Arrow builder for the manifest list in `inspect` ([`inspect/manifests.rs#L147`](https://github.com/apache/iceberg-rust/blob/832a4eba7a732baebbbd7f76bb6fd6225f1db1e9/crates/iceberg/src/inspect/manifests.rs#L147)). Expose it through the binding in place of per-entry `#[pyclass]` objects. The binding already depends on `arrow` with the `pyarrow` feature. #3241 (open) already replaces the binding's `unwrap()` calls with Python errors and fixes #2883. `read_manifest_list` still hard-codes `FormatVersion::V2` ([`bindings/python/src/manifest.rs#L223-L228`](https://github.com/apache/iceberg-rust/blob/832a4eba7a732baebbbd7f76bb6fd6225f1db1e9/bindings/python/src/manifest.rs#L223-L228)), which this task should fix. - [ ] **Task 4: Filter manifest entries in Rust.** Scan planning in PyIceberg filters every entry with partition and metrics evaluators, so returning Arrow alone doesn't help it. iceberg-rust already has `ManifestEvaluator`, `ExpressionEvaluator`, and `InclusiveMetricsEvaluator` ([`expr/visitors`](https://github.com/apache/iceberg-rust/tree/832a4eba7a732baebbbd7f76bb6fd6225f1db1e9/crates/iceberg/src/expr/visitors)). Exposing them means passing a bound predicate across the binding, which is new API surface. The alternative is vectorized filtering in PyIceberg over the Task 3 output. Which one to do is open. - [ ] **Task 5: Release `pyiceberg-core`** with Tasks 1 to 3. PyIceberg pins `pyiceberg-core>=0.10.1,<0.11.0`. These follow-ups to Task 1 were built during that work but held back so that Task 1 changes no behavior. - [ ] Match manifest partition fields by `field-id` before name. The spec makes the partition struct's field IDs the partition field IDs and stores them in each Avro field's `field-id` property. Java sanitizes partition field names that aren't valid Avro names and keeps the original in `iceberg-field-name` ([`TypeToSchema.java#L116-L128`](https://github.com/apache/iceberg/blob/781a30f3c9b5c74dc15ad5e16f7266919f2cbfeb/core/src/main/java/org/apache/iceberg/avro/TypeToSchema.java#L116-L128)). The reader matches by name, so today it drops those values without an error. This fixes the read side of #2536. #2535 covers the matching fix on the write side. - [ ] Accept `equality_ids` written as `array<long>`. PyIceberg wrote `long` until apache/iceberg-python#3842, so existing manifests contain it. - [ ] Return an error for an unknown manifest-list `format-version` instead of reading it as V2. - [ ] Size partition structs to the partition spec on the JSON `DataFile` path, as the Avro path already does. - [ ] Write uuid partition values as `fixed[16]` with logical type `uuid`, which is how the spec maps `uuid` to Avro. This changes the file format, so it needs a compatibility check. It overlaps with #2913 and #2916. - [ ] Regenerate `DEPENDENCIES.rust.tsv` with cargo-deny 0.19.9. ### Willingness to contribute I can contribute to this feature independently -- 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]
