CTTY opened a new pull request, #3670: URL: https://github.com/apache/iceberg-rust/pull/3670
## Which issue does this PR close? - Closes #3669. ## What changes are included in this PR? `TableScan::plan_files` loads manifests from one task spawned on the io runtime, and `ManifestReader::read` parsed each manifest on that task, so the Avro decoding of every manifest in a snapshot ran one after another while the other workers idled. Planning a 601-manifest table spent about 80% of its time in `Manifest::try_from_avro_bytes` on a single core. - `ManifestReader` gains a `pub(crate) with_runtime(Runtime)`. When set, `read` spawns `Manifest::try_from_avro_bytes` on `runtime.cpu()`, which is what `Runtime::cpu` is documented for; the fetch stays on the calling io task. - `ObjectCache::get_manifest` takes a `&Runtime` and passes it to the reader. `ObjectCache` itself stores no runtime, so `Table::with_runtime` keeps sharing the cache. - `PlanContext` carries the table runtime and passes it through `get_manifest`, so up to `concurrency_limit_manifest_files` parses run in parallel. The transaction code path passes the table runtime the same way. - A panic or cancellation in a spawned parse surfaces as `ErrorKind::Unexpected` through the `JoinHandle`, and `try_for_each_concurrent` stops starting new manifests on the first error, so the plan stream ends with that error exactly as it does for a read error. Design notes, in case they help review: - Spawning the whole per-manifest future on `io()` would also parallelise but keep CPU work on the io runtime; spawning it on `cpu()` would run storage IO on a runtime that an engine using iceberg-rust for planning only may build without an IO driver. Only the parse moves. - `spawn` rather than `spawn_blocking` keeps the parse within the worker count the embedding engine sized `cpu()` with, matching how the entry stages in `plan_files` already spawn their work. No public API changes: `make check-public-api` shows no diff. All new items are `pub(crate)`. ### Measurements `plan_files` median on a 4-worker runtime, TPC-H `lineitem` written by pyiceberg, local Parquet/Avro files: | table | manifests | before | after | |---|---:|---:|---:| | SF10 | 60 | 23.9 ms | 7.0 ms | | SF100 | 601 | 232.9 ms | 65.4 ms | ## Are these changes tested? Existing scan, object-cache and manifest tests cover the happy path (they now pass the table runtime). New unit tests: - `spec::manifest::reader::tests::test_read_parses_on_runtime`: a manifest read with and without a runtime yields identical results. - `scan::tests::test_plan_files_reports_corrupt_manifest_as_error` and `..._missing_manifest_as_error`: a bad manifest fails the plan stream with `ErrorKind::Unexpected` and the manifest path in the message, under a timeout so a swallowed error would show up as a hang rather than pass. - `scan::tests::test_plan_files_reports_cancelled_manifest_parse_as_error`: a parse spawned on a shut-down cpu runtime is cancelled, exercising the `JoinError` path a panicking parse would take. `cargo test -p iceberg --lib`: 2148 passed. `cargo clippy -p iceberg --all-targets --all-features -- -D warnings` and `cargo fmt --check` are clean. ## AI Disclosure This change was drafted with AI assistance (Kiro) and then reviewed, tested and benchmarked by me. Areas worth a closer look: the choice of `cpu().spawn` over `spawn_blocking` for the parse, and the error path when the cpu runtime is shut down mid-plan (covered by the cancellation test above). -- 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]
