chore(deps): upgrade to DataFusion 55 and arrow 59 - #34
Conversation
| restore-keys: | | ||
| ${{ runner.os }}-cargo-registry- | ||
| - run: cargo check --all-targets | ||
| - run: cargo check --all-targets --all-features |
There was a problem hiding this comment.
nit: switching all three jobs from bare to --all-features gains feature coverage but drops the default build (not blocking).
The crate has no default features and every feature-gated test file is #![cfg(feature = "...")]-gated (e.g. tests/parquet_provider_test.rs:1), so the previous cargo check --all-targets was the only thing verifying that the crate compiles with no features enabled. After this change nothing does — a missing #[cfg(feature = ...)] on an item that references a feature-gated dep would build green here and break for anyone depending on the crate with default features.
Cheap fix is to keep both, e.g. on the check job:
- run: cargo check --all-targets
- run: cargo check --all-targets --all-featuresSeparately, CLAUDE.md:17 still documents the pre-push command as cargo test --features sqlite-provider; worth updating to --all-features so local runs match CI.
There was a problem hiding this comment.
Addressed in 90b0aa5 — you were right that this was a real coverage regression, not just a style point. The crate has no default features and the provider test files are all #![cfg(feature = ...)]-gated, so the bare cargo check --all-targets really was the only thing proving the no-feature build compiles. Restored it alongside the all-features one exactly as you suggested; verified cargo check --all-targets passes locally.
Also updated CLAUDE.md:17 to cargo test --all-features in 187a0e7 so the documented pre-push command matches CI.
| [dependencies] | ||
| tracing = "0.1" | ||
| datafusion = "54" | ||
| datafusion = "55" |
There was a problem hiding this comment.
super nit: README.md:7 advertises **DataFusion:** 53 (not blocking).
It was already stale at 54, and this bump makes it two majors behind. Since this PR is the version bump, it's the natural place to fix it to 55.
There was a problem hiding this comment.
Fixed in 187a0e7 — README now says **DataFusion:** 55. Agreed this is the natural place for it since this PR is the version bump.
There was a problem hiding this comment.
Reviewed the DF 55 / arrow 59 adaptation. The API migrations look faithful:
ObjectStoreReadermatches the semantics of theParquetObjectReaderit replaces — the old reader also hadfile_size: Noneand took theload_via_suffix_and_finishpath with no page-index preload, so footer round-trips and page-index behaviour are unchanged.- All three
ExecutionPlanimpls (USearchExec,SqliteFullScanExec,BatchExec) gotapply_expressions, and the no-op is correct for each: none holds a rootPhysicalExpr, andUSearchExec's pre-scan predicate lives in theFilterExecthatchildren()already exposes. &dyn Session/planning_ctxthreading inplanner.rsis complete — no otherSessionStateorcreate_physical_exprcall sites remain.Cargo.lockresolves single versions of arrow 59.2.0, parquet 59.2.0 and object_store 0.13.2, so no duplicate-crate type mismatches.
Two non-blocking nits inline. Note that CI was still pending when this review ran, so I have not seen build/clippy/test results.
There was a problem hiding this comment.
Prior nits addressed: default-feature cargo check --all-targets restored alongside --all-features, CLAUDE.md pre-push command updated, and README.md DataFusion version bumped to 55.
Checked the DF 55 adaptation:
apply_expressionsis implemented on all threeExecutionPlanimpls in the crate (src/planner.rs:316,src/sqlite_provider.rs:639,src/udtf.rs:240) — none missed. All three own no root physical exprs;USearchExec's pre-scan predicate lives in theFilterExecunderprovider_scan, whichchildren()returns, so the expression walk still reaches it.ObjectStoreReaderfollows the upstream parquet suffix-fetch pattern, andCachedMetaReaderstill short-circuitsget_metadata, so the per-query footer round trips remain eliminated.Cargo.lockresolves a singleobject_store 0.13.2, so the direct0.13pin and DF 55's transitive dependency don't split into two incompatible copies.
No new findings. Approving on code review; CI checks were still pending when this review ran, so this is not a statement that the build/tests passed.
Bumps DataFusion 54→55 and the arrow/parquet family 58→59, which forces the MSRV pin to 1.94.0 and requires adapting to the DF 55 planner/ExecutionPlan API changes (
&dyn Session,PhysicalPlanningContext, the now-requiredapply_expressions) and replacing the deprecatedParquetObjectReaderwith a directAsyncFileReaderoverObjectStore.CI now builds, lints, and tests with
--all-featuresso the feature-gated parquet/sqlite/feather providers are actually covered.