chore: bump DataFusion to 55.2.0 (arrow/parquet 59.3, MSRV 1.94) - #248

@claude review

@codex review

Claude finished @mwiewior's task in 2m 27s —— View job

PR Review: chore: bump DataFusion to 55.2.0

Tasks

• Load review tools
• Analyze git diff of PR changes
• Review key changed files in depth
• Post comprehensive review feedback

Summary

This is a mechanically thorough and correct DataFusion API migration. The changes are consistent across all 25 ExecutionPlan / TableProvider / ScalarUDFImpl implementations, the downstream verification story is solid (1047 + 1928 tests passing), and all the stated goals are achieved. No correctness or security issues found.

Findings

Bug (minor): CHANGELOG version mismatch

CHANGELOG.md:12 says arrow/parquet 59.2 but Cargo.lock and the PR title both say 59.3.2:

# Cargo.lock
name = "arrow"
version = "59.3.2"

The correct version to put in the CHANGELOG entry is 59.3.2 (or at minimum 59.3). Fix this →

Cosmetic: Stale test names and comments in FASTA and VCF write_exec

The call sites in tests were migrated from with_new_children → replace_children and required_input_distribution → input_distribution_requirements, but the function names and a comment weren't updated:

• datafusion/bio-format-fasta/src/write_exec.rs:258: fn test_with_new_children_wrong_count → body now calls replace_children
• datafusion/bio-format-fasta/src/write_exec.rs:296: fn test_with_new_children_preserves_properties → body now calls replace_children
• datafusion/bio-format-fasta/src/write_exec.rs:338: fn test_required_input_distribution → body now calls input_distribution_requirements()
• datafusion/bio-format-vcf/src/write_exec.rs:516: fn test_source_metadata_threaded_through_with_new_children → body now calls replace_children; also the inline comment // with_new_children should preserve source_metadata on line 534

Not blocking — the tests are correct — but they'd confuse a future reader searching for with_new_children usage.

Observation: apply_expressions signature verbosity

The apply_expressions signature is repeated verbatim (fully qualified paths) across all 25 ExecutionPlan leaf implementations:

fn apply_expressions(
    &self,
    _f: &mut dyn FnMut(
        &std::sync::Arc<dyn datafusion::physical_expr::PhysicalExpr>,
    ) -> datafusion::common::Result<
        datafusion::common::tree_node::TreeNodeRecursion,
    >,
) -> datafusion::common::Result<datafusion::common::tree_node::TreeNodeRecursion> {
    Ok(datafusion::common::tree_node::TreeNodeRecursion::Continue)
}

Since every node is a leaf returning Continue, this is correct and cargo fmt accepts it. The verbosity is a consequence of avoiding new use imports in files that only removed the use std::any::Any import. Not a defect — just noting it in case a future cleanup pass wants to introduce use datafusion::common::{Result as DFResult, tree_node::TreeNodeRecursion} aliases to shorten this.

What looks good

• Completeness: All 25 ExecutionPlan impls and all TableProvider / ScalarUDFImpl impls were updated uniformly. The search for remaining DataFusion-trait as_any() calls turned up none; the remaining as_any() usages are all Arrow Array::as_any() downcasts, which are correct and unaffected.
• EnsemblCacheExec::partition_statistics refactoring: Clean extraction to partition_statistics_inner with .map(Arc::new) at the trait boundary. The test sites correctly use the new StatisticsContext::new().compute(...) caller API.
• CRAM clippy fix: if reference_seqs.is_some() { ... .unwrap() } → if let Some(seqs) = reference_seqs.as_ref() is an improvement in both correctness-perception and style.
• Structure-codecs harness: assert!(!cfg!(debug_assertions)) → if cfg!(debug_assertions) { panic!(...) } correctly suppresses the assertions_on_constants lint without changing semantics.
• Sandbox isolation: sandbox/cooler-spike intentionally retains DataFusion 53; this is correctly called out in the PR description and not touched.
• BED/BGEN/PGEN downcast: plan.as_any().downcast_ref::<T>() → plan.downcast_ref::<T>() is the right mechanical migration now that dyn ExecutionPlan has the Any supertrait removed from the external API.
• VCF contradictory-bounds test: plan.as_any().is::<EmptyExec>() → plan.is::<EmptyExec>() is correct.
• Single copy of arrow/parquet/datafusion in Cargo.lock: confirmed.

Verdict: Ready to merge after the CHANGELOG version correction (59.2 → 59.3.2). The stale test names are cosmetic and can be cleaned up at any time.

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: d1eb0c3764

Thanks for the review. All three points are addressed in 99d7f81 (no behaviour change):

1. CHANGELOG version. Fixed: the entry now says arrow/parquet 59.3.2, which matches Cargo.lock. I had updated the DataFusion version in that line during the rebase and missed the arrow one.

2. Stale test names. Renamed to match the DataFusion 55 caller APIs their bodies use:

   • fasta test_with_new_children_wrong_count → test_replace_children_wrong_count
   • fasta test_with_new_children_preserves_properties → test_replace_children_preserves_properties
   • fasta test_required_input_distribution → test_input_distribution_requirements
   • vcf test_source_metadata_threaded_through_with_new_children → test_source_metadata_threaded_through_replace_children. Its inline comment changed too.

   The remaining fn with_new_children / fn required_input_distribution hits are trait implementations, which DataFusion 55 still requires, so they stay.

3. apply_expressions verbosity. All 25 leaf impls now use imported Arc / PhysicalExpr / TreeNodeRecursion, plus Result in files that already import DataFusion's. Files without that import keep datafusion::common::Result rather than gaining an alias that could clash with a local Result. This matches the same cleanup in chore: bump DataFusion to 55.2.0 and bio-formats to its DF 55 branch datafusion-bio-functions#332.

Verification on 99d7f81:

• cargo clippy --workspace --all-targets --all-features -- -D warnings, cargo fmt, RUSTDOCFLAGS=-D warnings cargo doc and the pre-commit hooks are clean.
• fasta and vcf lib tests: 192 passed, 0 failed, with the four renamed tests running under their new names.

Downstream re-pins to 99d7f81:

• chore: bump DataFusion to 55.2.0 and bio-formats to its DF 55 branch datafusion-bio-functions#332 (08a9e63)
• chore: bump DataFusion to 55.2.0 and PyO3 to 0.29 polars-bio#507

Both lockfiles still have one copy each of datafusion-bio-format-core, datafusion and arrow.

🤖 Generated with Claude Code

@codex review

@claude review

Claude finished @mwiewior's task in 2m 47s —— View job

PR Review: chore: bump DataFusion to 55.2.0

Tasks

• Load review tools
• Analyze git diff of PR changes (both commits)
• Verify previous review findings were addressed in 99d7f81
• Review key changed files in depth
• Post comprehensive review feedback

Summary

All three findings from the previous round are fully addressed in 99d7f81. No new correctness or security issues found. This is a clean, uniform, and complete DataFusion 55 migration.

Verification of Previous Findings

1. CHANGELOG version — fixed ✓

The entry now reads arrow/parquet 59.3.2, matching Cargo.lock.

2. Stale test names — fixed ✓

All four functions renamed correctly:

• fasta: test_replace_children_wrong_count, test_replace_children_preserves_properties, test_input_distribution_requirements
• vcf: test_source_metadata_threaded_through_replace_children with updated inline comment

The remaining fn with_new_children at vcf/src/write_exec.rs:182 is a trait implementation (still required by DataFusion 55), not a stale test name — correctly left alone.

3. apply_expressions verbosity — fixed ✓

All 25 leaf impls now use imported Arc / PhysicalExpr / TreeNodeRecursion, with Result used unqualified in files that already import DataFusion's. The result in files with a local Result correctly keeps the FQN to avoid ambiguity.

New Observations

Cosmetic: one apply_expressions still uses FQN Result

datafusion/bio-format-structure/src/table_provider.rs:212 already imports datafusion::common::Result via the grouped use datafusion::{ ..., common::Result, ... }, but the new apply_expressions body uses datafusion::common::Result<TreeNodeRecursion> rather than the shorter Result<TreeNodeRecursion>. Not a compiler error or logic issue — the nits cleanup in 99d7f81 touched this file (to add imports and remove as_any) but missed the FQN. Non-blocking; can be addressed in a later pass.

Undocumented CI matrix change

Both .github/workflows/structure-codecs-consumer.yml and .github/workflows/structures.yml changed macos-15 → macos-14 in the platform matrix (the ARM macOS runner). The PR description covers what wasn't touched in the workflows but doesn't mention this change. Looks like a routine runner-availability fix, but worth a one-liner in the description or commit message so reviewers aren't left wondering.

What Looks Good

• Completeness: All 25 ExecutionPlan, all TableProvider, and all 8 ScalarUDFImpl implementations migrated consistently. Arrow Array::as_any() downcasts are correctly untouched throughout.
• partition_statistics refactor: Clean extraction to partition_statistics_inner with .map(Arc::new) at the trait boundary; test sites correctly use StatisticsContext::new().compute(...).
• CRAM if let fix: is_some() { .unwrap() } → if let Some(seqs) = reference_seqs.as_ref() confirmed at both callsites (lines 282 and 568).
• BED/BGEN/PGEN/VCF downcast: plan.as_any().downcast_ref::<T>() / .is::<T>() → plan.downcast_ref::<T>() / plan.is::<T>() applied uniformly across all test sites.
• BED regression test: replace_children with ReplaceChildrenOptions::new(ChildrenPropertiesMode::Recompute) is the correct new API.
• Structure harness: if cfg!(debug_assertions) { panic!(...) } suppresses assertions_on_constants correctly.
• Single dependency copies: Cargo.lock has exactly one entry each for arrow, parquet, and datafusion (confirmed: 3 unique matches, 1 each).
• rust-toolchain.toml: pinned to 1.94.0 as required by DataFusion 55.2 MSRV.

Verdict: Ready to merge. The two observations above are cosmetic/documentation-only and non-blocking.

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 99d7f810c4

Both observations from the second round are addressed. The branch is rebased onto master (94b06ab) and is now at 055e971.

1. FQN Result in bio-format-structure. Fixed in 055e971. StructureExec::apply_expressions now uses the already-imported Result<TreeNodeRecursion>. I checked the other 24 leaf impls: the 10 that still spell out datafusion::common::Result have no DataFusion Result import, so they stay as they are, as explained in the previous round.
2. macOS runner change (macos-15 → macos-14). This PR never changed a workflow. The branch was based on v1.15.0 (a754d55), before ci: move structure-reader macOS arm64 jobs off the retired macos-14 runner #273 (94b06ab) moved those jobs from macos-14 to macos-15. Against current master, a two-dot diff showed ci: move structure-reader macOS arm64 jobs off the retired macos-14 runner #273 as if this PR reverted it. Since the rebase, git diff origin/master...HEAD -- .github is empty.

Verification on 055e971:

• cargo fmt --all -- --check and cargo clippy --workspace --all-targets --all-features -- -D warnings are clean.
• cargo test -p datafusion-bio-format-structure: all passed, 0 failed.

🤖 Generated with Claude Code

@claude review

@codex review

Claude finished @mwiewior's task in 2m 20s —— View job

PR Review: chore: bump DataFusion to 55.2.0

Tasks

• Load review tools
• Analyze git diff (HEAD vs origin/master)
• Verify previous round-2 findings are addressed in 055e971
• Review key changed files in depth
• Post comprehensive review feedback

Summary

This is a clean, complete, and consistent DataFusion 55 migration. All findings from the previous two review rounds are fully addressed. No new correctness or security issues found.

Verification of Round-2 Findings

1. FQN Result in StructureExec::apply_expressions — fixed ✓

055e971 changed both occurrences in bio-format-structure/src/table_provider.rs:215–218 from datafusion::common::Result<TreeNodeRecursion> to the already-imported Result<TreeNodeRecursion>, matching all other files that have a DataFusion Result import.

2. macOS runner change — was rebase artifact, now gone ✓

git diff origin/master...HEAD -- .github is empty. The difference noted in round 2 was produced by a two-dot diff against an older base; against the rebased master (94b06ab) the workflow files are unchanged.

Positive verification pass

• 25 apply_expressions leaf impls: confirmed via grep — exactly 25 occurrences across 25 files, one per exec/plan node.
• Zero DataFusion-trait fn as_any remaining: grepping the datafusion/ tree returns no matches. All remaining .as_any() calls are Arrow Array::as_any() downcasts, which are unaffected.
• crate::errors::Result<T> in ensembl-cache is type Result<T> = datafusion::common::Result<T> — so EnsemblCacheExec::apply_expressions using the local Result alias is correct and type-compatible.
• partition_statistics → Arc<Statistics>: partition_statistics_inner extraction and .map(Arc::new) wrapper confirmed; test sites correctly use StatisticsContext::new().compute(&exec, &StatisticsArgs::new().with_partition(...)).
• Test renames (round 1): all four confirmed — test_replace_children_wrong_count, test_replace_children_preserves_properties, test_input_distribution_requirements (fasta), test_source_metadata_threaded_through_replace_children (vcf) with updated inline comment.
• Downcast migrations: plan.as_any().downcast_ref::<T>() → plan.downcast_ref::<T>() and plan.as_any().is::<T>() → plan.is::<T>() applied uniformly across PGEN (7 sites), BGEN (6 sites), VCF (1 site).
• BED regression test: replace_children(vec![...], ReplaceChildrenOptions::new(ChildrenPropertiesMode::Recompute)) correctly replaces the deprecated with_new_children call.
• CHANGELOG: arrow/parquet 59.3.2 — matches Cargo.lock (arrow 59.3.2, parquet same).
• Cargo.lock: single copies of datafusion (55.2.0), arrow (59.3.2), parquet — confirmed (1 occurrence each).
• rust-toolchain.toml: 1.91.0 → 1.94.0 ✓
• CRAM if let Some: both callsites (lines 279, 565) updated ✓
• Structure harness: if cfg!(debug_assertions) { panic!(...) } suppresses assertions_on_constants lint correctly ✓

Verdict: Ready to merge.

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 055e971164