Repository navigation
chore: bump DataFusion to 55.2.0 and bio-formats to its DF 55 branch - #332
Conversation
Move the workspace from DataFusion 53.0.0 to 55.2.0 (arrow/parquet 59.3.2, rust-toolchain and CI on 1.94.0, DataFusion 55.2's MSRV) and pin every datafusion-bio-format-* dependency to biodatageeks/datafusion-bio-formats#248 (d1eb0c3), which carries the same bump rebased on formats v1.15.0, so it includes everything the previous e0379fb pin did. pileup and vep share that one rev so the graph holds one copy of datafusion-bio-format-core and DataFusion. Mechanical fallout of the 54/55 API changes: - `as_any` removed from DataFusion traits: dropped from every impl, the `.as_any()` hop removed at plan/expr downcast sites (including the multi-line chains in the interval planner), unused `Any` imports removed. fastqc's own `QcModule::as_any` is unrelated and kept. - `ExecutionPlan::apply_expressions` required: leaf implementations for the nodes without expressions; `IntervalJoinExec` visits its join keys and filter like DataFusion's own `HashJoinExec`. - `QueryPlanner` / `PhysicalPlanner` take `&dyn Session`, and `create_physical_expr` gains the `PhysicalPlanningContext` argument. - `create_hashes` takes DataFusion's `HashState`; the interval join uses `datafusion::common::hash_utils::RandomState` (ahash stays for the plain hash maps). - `Distribution::HashPartitioned` -> `KeyPartitioned`. - `MemoryPool` now requires `Display` and `name()`: TracingPool and the test-only RejectHashPool. - `MutableArrayData::extend` is deprecated in arrow 59.3: the SIFT page gather uses `try_extend` and propagates the overflow error. - `partition_statistics` returns `Arc<Statistics>` and is deprecated for callers; the interval join estimator and the plugin-cache join selector read statistics through `StatisticsContext`. That last item is the one behavioural change. Under DataFusion 55 the deprecated `partition_statistics` returns unknown statistics for DataFusion's own nodes, so `choose_for_hash_plan` would have reported MissingStatistics and silently forced every plugin-cache tier join onto SortMergeJoin; the adaptive-join tests cover it. Why: DataFusion 53's external sorter frees its merge headroom and then re-acquires it from an empty reservation, which starves under `FairSpillPool` when several partitions sort concurrently (#235). apache/datafusion#20642 fixes that and shipped in 54.0.0. Not touched: `research/lance_encoding_sandbox` is outside the workspace and still pins arrow 58 / datafusion-common 53. Supersedes #236 and dependabot #8. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@claude review |
|
@codex review |
|
Claude finished @mwiewior's task in 20s —— View job Review of PR #332 (DataFusion 55.2.0 bump)I read the diff for the ranges, vep, pileup and Cargo changes. I did not build the workspace or run the tests. I also did not read the fastqc changes line by line. They look like the same mechanical Overall: The changes match the DF 54/55 API migrations the description lists. I found no correctness bugs. A few things are worth checking before merge. Things to check
VerdictLooks good. Once the formats PR is merged and tagged, re-pin the git dependencies, then merge. Please confirm the hash-seed question in item 1 first. |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
- interval join: say why `hash_utils::RandomState::default()` is safe (foldhash `FixedState` with a compile-time constant seed, shared by both join sides), and fold `partition_statistics_inner` back into `partition_statistics` where it lived before the bump, with a note on why the partition argument is ignored. Drop the stray blank line left by the `Any` import removal. - plugin-cache join selector: import `StatisticsContext` / `StatisticsArgs` instead of spelling the full paths inline. - the ten leaf `apply_expressions` impls (fastqc, pileup, ranges, vep): use imported `Arc` / `PhysicalExpr` / `Result` / `TreeNodeRecursion` instead of fully qualified paths. No behaviour change. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thanks for the review. I addressed the points in a6556da (no behaviour change):
Verification on a6556da:
🤖 Generated with Claude Code |
|
@codex review |
|
@claude review |
|
Claude finished @mwiewior's task in 15s —— View job Review of PR #332 (DataFusion 55.2.0 bump), second pass at a6556daI read the diff for the ranges planner and join code, the vep plugin_cache and SIFT changes, and the Cargo changes. I did not build or run tests. I am relying on your reported results (1928 passed, clippy, fmt and doc clean). Verdict: No blocking issues. The changes match the DF 54/55 API migrations the description lists. The points from the earlier review are addressed in a6556da. Checked and fine
Minor and non-blocking
Before merging
|
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
- CHANGELOG: the arrow/parquet version is 59.3.2, not 59.2. - Rename the tests whose bodies moved to the DataFusion 55 caller APIs: fasta `test_with_new_children_wrong_count` / `test_with_new_children_preserves_properties` / `test_required_input_distribution` and vcf `test_source_metadata_threaded_through_with_new_children` now name `replace_children` / `input_distribution_requirements`, and the inline comment follows. - The 25 leaf `apply_expressions` impls use imported `Arc` / `PhysicalExpr` / `TreeNodeRecursion` (and `Result` where the file already imports DataFusion's) instead of fully qualified paths, matching biodatageeks/datafusion-bio-functions#332. No behaviour change. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Follow biodatageeks/datafusion-bio-formats#248 to its review-nit commit (test renames, shorter `apply_expressions` signatures, CHANGELOG arrow version). No API change; vep and pileup stay on one formats rev. Co-Authored-By: Claude Opus 5.5 <[email protected]>
- CHANGELOG: the arrow/parquet version is 59.3.2, not 59.2. - Rename the tests whose bodies moved to the DataFusion 55 caller APIs: fasta `test_with_new_children_wrong_count` / `test_with_new_children_preserves_properties` / `test_required_input_distribution` and vcf `test_source_metadata_threaded_through_with_new_children` now name `replace_children` / `input_distribution_requirements`, and the inline comment follows. - The 25 leaf `apply_expressions` impls use imported `Arc` / `PhysicalExpr` / `TreeNodeRecursion` (and `Result` where the file already imports DataFusion's) instead of fully qualified paths, matching biodatageeks/datafusion-bio-functions#332. No behaviour change. Co-Authored-By: Claude Opus 5.5 <[email protected]>
* chore: bump DataFusion to 55.2.0 (arrow/parquet 59.3, MSRV 1.94) Move the workspace from DataFusion 53.0.0 to 55.2.0. Besides the version pins this is the mechanical fallout of the 54 and 55 API changes: - `as_any` is gone from `ExecutionPlan`, `TableProvider` and `ScalarUDFImpl` (trait upcasting): drop it from every impl (including the msa, structure and foldcomp crates added since the first version of this change), drop the `.as_any()` hop at the plan/provider downcast sites, remove the now unused `std::any::Any` imports. Arrow `Array::as_any` is untouched. - `ExecutionPlan::apply_expressions` is now required: no exec node in this workspace owns physical expressions, so all are leaf implementations that return `Continue`. - `partition_statistics` returns `Arc<Statistics>`; the ensembl-cache scan's estimator is wrapped and its tests go through `StatisticsContext`, the new caller-side API. - `with_new_children` / `required_input_distribution` call sites in tests move to `replace_children` / `input_distribution_requirements`. - New clippy lints from the 1.94 toolchain (bed doc comments, cram unwrap-after-is_some, constant assertion in the structure-codec bench harness). Why: DataFusion 53's external sorter frees its merge headroom and re-acquires it from an empty reservation, which starves under `FairSpillPool` when several partitions sort concurrently (biodatageeks/datafusion-bio-functions#235). apache/datafusion#20642 fixes that and shipped in 54.0.0; 55.2.0 is the current release. Not touched: `sandbox/cooler-spike` is outside the workspace and still pins DataFusion 53 / arrow 58. Co-Authored-By: Claude Opus 5.5 <[email protected]> * refactor: address review nits on the DataFusion 55.2 bump - CHANGELOG: the arrow/parquet version is 59.3.2, not 59.2. - Rename the tests whose bodies moved to the DataFusion 55 caller APIs: fasta `test_with_new_children_wrong_count` / `test_with_new_children_preserves_properties` / `test_required_input_distribution` and vcf `test_source_metadata_threaded_through_with_new_children` now name `replace_children` / `input_distribution_requirements`, and the inline comment follows. - The 25 leaf `apply_expressions` impls use imported `Arc` / `PhysicalExpr` / `TreeNodeRecursion` (and `Result` where the file already imports DataFusion's) instead of fully qualified paths, matching biodatageeks/datafusion-bio-functions#332. No behaviour change. Co-Authored-By: Claude Opus 5.5 <[email protected]> * refactor(structure): use imported Result in apply_expressions Co-Authored-By: Claude Opus 5.5 <[email protected]> --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
Move every datafusion-bio-format-* dependency (vep and pileup) from the pre-release rev to tag v1.16.0, which contains the DataFusion 55.2 bump (biodatageeks/datafusion-bio-formats#248) and the OpenDAL 0.59.5 fix for remote reads that retried forever under tokio 1.54 (#274). The lockfile keeps a single copy of datafusion-bio-format-core, datafusion, arrow and parquet, and drops the OpenDAL git revision. Also import Session and PhysicalPlanningContext in the ranges planners instead of spelling out their paths inline (review nit). Co-Authored-By: Claude Opus 5.5 <[email protected]>
DataFusion 55.2 requires rustc 1.94. #332 moved rust-toolchain.toml and ci.yml to 1.94.0 but left release.yml on 1.91.0, so the release job failed at 'cargo check --all' with 'rustc 1.91.0 is not supported'. Also update the minimum Rust version in the README. Co-authored-by: Claude Opus 5.5 <[email protected]>
* chore: bump DataFusion to 55.2.0 and PyO3 to 0.29 Move from DataFusion 53.0.0 to 55.2.0 together with the bio crates: - datafusion 55.2.0, datafusion-python 55.0.0 (the newest published; it requires datafusion ^55.1.0, so the graph keeps one DataFusion 55.2.0) - pyo3 0.28.3 -> 0.29.3, the line datafusion-python 55 and arrow-pyarrow 59 require; pyo3-log 0.13.5 is the first release accepting pyo3 0.29 - arrow / arrow-schema / arrow-array 59.3.2 - rust-toolchain 1.94.0 (DataFusion 55.2's MSRV) - Python `datafusion>=55.0.0,<56` in pyproject, uv.lock and the in-repo bioconda recipe - datafusion-bio-format-* pinned to biodatageeks/datafusion-bio-formats#248 (d1eb0c3) and datafusion-bio-function-* to biodatageeks/datafusion-bio-functions#332 (1099515) until both are tagged Rust changes are the DataFusion 55 trait fallout only: `as_any` dropped from the fastqc and depth table providers and from SchemaOverrideExec, which also gets a leaf `apply_expressions` (it owns no physical expressions). None of PyO3 0.29's breaking APIs are used here. test_runtime_memory_limit_spills moves from a 2 MB to an 8 MB pool. DataFusion 55's grouped aggregation needs more headroom before it can spill: at 2 MB its hash table fills the pool and the follow-up allocation fails with Resources exhausted (DataFusion 53 spilled 91 times there). From 4 MB up it spills and returns all 1M groups (4 MB: 31 spills, 8 MB: 15), so the test still exercises spilling. Why: DataFusion 53's external sorter starves under FairSpillPool when several partitions sort concurrently; apache/datafusion#20642 fixes it in 54.0.0, and the bio crates already moved for it. Co-Authored-By: Claude Opus 5.5 <[email protected]> * fix: pin the abi3 floor to 3.11 and build one wheel per platform CI on the bump built a `cp314-abi3` Windows wheel, installable only on Python 3.14+. With the bare `abi3` feature, PyO3 0.29 takes the abi3 floor from the build interpreter, and maturin's `--find-interpreter` on the Windows runner found 3.14 first. Verified locally: the same tree built against 3.14 gives `cp314-abi3`, against 3.11 `cp311-abi3`. The `abi3-py311` feature matches `requires-python = ">=3.11"` and gives `cp311-abi3` from any interpreter (checked by building with 3.14 and running the wheel under 3.14). The manylinux image also has 3.14t and PyPy 3.11, which abi3 cannot cover. maturin added version-specific wheels for them, which broke the smoke test's `pip install dist/*.whl` and would have published untested free-threaded and PyPy wheels. Linux builds now target CPython 3.11 only, so every platform ships one abi3 wheel as before the bump. Also from review: - OpenSpec delta (Codex P1): the MODIFIED requirement now carries the full current text, including the Polars exception and its scenario, so archiving does not drop them. The abi3 floor is recorded as an ADDED requirement. - `SchemaOverrideExec::apply_expressions` uses imported names instead of fully qualified paths (Claude review nit). - Re-pin datafusion-bio-formats to 99d7f81 (#248) and datafusion-bio-functions to 08a9e63 (#332), their review-nit commits. Co-Authored-By: Claude Opus 5.5 <[email protected]> * chore: pin bio-formats v1.16.0 and bio-functions v0.25.0 Move every datafusion-bio-format-* dependency from rev 99d7f81 to tag v1.16.0 and every datafusion-bio-function-* dependency from rev 08a9e63 to tag v0.25.0, the DataFusion 55 releases of both. bio-formats v1.16.0 also moves OpenDAL to 0.59.5, so a remote read that fails mid-stream returns an error instead of retrying forever under tokio 1.54 (biodatageeks/datafusion-bio-formats#274). Cargo.lock keeps one copy each of datafusion-bio-format-core, datafusion, arrow and parquet. Mark OpenSpec task 1.11 done. Co-Authored-By: Claude Opus 5.5 <[email protected]> --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
Summary
Moves the workspace from DataFusion 53.0.0 to 55.2.0 (arrow/parquet 59.3.2, rust-toolchain and CI on 1.94.0, which is DataFusion 55.2's MSRV). It also pins every
datafusion-bio-format-*dependency to the formats releasev1.16.0. That release contains the same bump (biodatageeks/datafusion-bio-formats#248) and everything the previouse0379fbpin did. It also has the OpenDAL 0.59.5 fix for remote reads that retried forever under tokio 1.54 (biodatageeks/datafusion-bio-formats#274). pileup and vep share that one tag, so the graph keeps a single copy ofdatafusion-bio-format-coreand DataFusion.Fixes the
FairSpillPoolexternal-sort merge starvation behind #235 (apache/datafusion#20642, shipped in 54.0.0).Supersedes #236 (closed; written against DF 55.0.0 on a 61-commits-old master) and dependabot #8.
What changed
Mechanical fallout of the 54/55 API changes:
as_anyremoved from DataFusion traits. The.as_any()hop is gone at plan/expr downcast sites. fastqc's ownQcModule::as_anyis unrelated and kept.ExecutionPlan::apply_expressionsis required. Nodes without expressions get leaf impls;IntervalJoinExecvisits its join keys and filter, asHashJoinExecdoes.QueryPlanner/PhysicalPlannertake&dyn Session, andcreate_physical_exprtakes the extraPhysicalPlanningContext.create_hashestakesHashState. The interval join usesdatafusion::common::hash_utils::RandomState.Distribution::HashPartitioned→KeyPartitioned.MemoryPoolnow requiresDisplay+name(). This coversTracingPooland the test-onlyRejectHashPool.MutableArrayData::extendis handled: the SIFT page gather usestry_extendand propagates the overflow error.partition_statisticsis deprecated for callers and returns unknown statistics for DataFusion's own nodes. Left as is,choose_for_hash_planwould silently force every plugin-cache tier join onto SortMergeJoin. The interval-join estimator and the plugin-cache join selector now read statistics throughStatisticsContext. The adaptive-join tests cover this.KvLookupExec→VariationLookupExecrename from master; thelookup_providertestTableProvideradded since chore: bump DataFusion to 55.0.0 and bio-formats to its DF 55 branch #236.Not touched:
research/lance_encoding_sandbox, which is outside the workspace and stays on arrow 58 / datafusion-common 53.Verification (local, Rust 1.94.0)
cargo fmt --all -- --check,cargo clippy --all-targets --all-features -- -D warnings, andcargo doc --no-deps --all-featureswithRUSTDOCFLAGS=-D warnings: clean.cargo test --all: 1928 passed, 0 failed, 11 ignored.cargo test -p datafusion-bio-function-vep --features cache-builder --lib cache::build: 59 passed.Cargo.lock: one copy each of datafusion, arrow, parquet,datafusion-bio-format-core.Merge order
The formats side is merged and released as
v1.16.0, and this branch is pinned to that tag. It can merge once CI is green.🤖 Generated with Claude Code