Repository navigation
perf: Update TableProvider to take scan projection as Option<&[usize]> - #24376
Merged
Merged
Conversation
`TableProvider::scan` took its projection as `Option<&Vec<usize>>`. Besides
being the signature `clippy::ptr_arg` exists to discourage, it forced
`TableProvider::scan_with_args`'s default body to allocate:
let projection = args.projection().map(|p| p.to_vec());
let plan = self.scan(state, projection.as_ref(), filters, limit).await?;
`ScanArgs::projection()` yields `Option<&[usize]>`, so the `Vec` existed only to
be borrowed. It also could not outlive a hoisted future, which forced the body to
stay an `async fn` -- and that made it the single most expensive thing to compile
in `datafusion-session`: proving its coroutine `Send` walks the whole
`Expr`/`LogicalPlan` type graph, which `#[async_trait]`'s `'life0: 'async_trait`
bounds stop rustc from caching (see apache#13814, apache#24325, apache#24326, apache#24329, apache#24330).
With the slice signature the default body hands the projection straight through
and only captures the already-boxed future it awaits:
let plan = self.scan(state, args.projection(), args.filters().unwrap_or(&[]), args.limit());
Box::pin(async move { Ok(plan.await?.into()) })
Interleaved A/B of `cargo rustc -p <crate> --lib`, 3 rounds each:
datafusion-session 0.890s -> 0.273s (evaluate_obligation 624.3ms -> 10.4ms)
datafusion-catalog 1.046s -> 1.205s
datafusion 4.088s -> 4.125s
`datafusion-session` compiles 3.3x faster and its trait solving drops 98%.
`datafusion-catalog` regresses ~10% and core is flat; both were confirmed with
the measurement order reversed, so the net is roughly -0.45s across the three.
The same shape is updated in `StreamingTableExec::try_new`,
`FilterExec::with_projection` and `batch_filter`, whose projections feed the same
call paths. `datafusion_common::project_schema` now takes
`Option<&T> where T: AsRef<[usize]> + ?Sized` so it accepts both a `Vec` and a
slice; no caller of it needs to change.
Call sites get simpler rather than noisier -- `Some(&vec![2, 1])` becomes
`Some(&[2, 1])`, and three `vec!` allocations in tests disappear.
Co-Authored-By: Claude Opus 5 <[email protected]>
github-actions
Bot
added
documentation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Contributor
Author
|
@alamb for consideration - this is I think the only one that couldn't be converted due to breaking the API |
`datafusion/ffi/src/tests/` is behind the `integration-tests` feature, so `cargo check --workspace --all-targets` never compiled it and the call site was missed. `scan.projection` is an `Option<Vec<usize>>` field, so it needs `as_deref()` rather than `as_ref()`. Checked with `--all-features --all-targets` per crate this time, which is what would have caught it: datafusion, datafusion-catalog, datafusion-catalog-listing, datafusion-session, datafusion-functions-table, datafusion-sqllogictest, datafusion-proto, datafusion-substrait and datafusion-ffi are all clean. Co-Authored-By: Claude Opus 5 <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24376 +/- ##
==========================================
- Coverage 81.19% 81.19% -0.01%
==========================================
Files 1110 1110
Lines 388618 388752 +134
Branches 388618 388752 +134
==========================================
+ Hits 315531 315634 +103
- Misses 54507 54529 +22
- Partials 18580 18589 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alamb
approved these changes
Aug 14, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Makes sense to me -- thanks @Dandandan -- I'll have claude confirm the numbers too on a separate machine
`docs/source/library-user-guide/custom-table-providers.md` is included into `datafusion/core/src/lib.rs` via `include_str!`, so its `TableProvider` examples are compiled as doctests -- five `scan` signatures there still took `Option<&Vec<usize>>`. Also wraps the ffi call site changed in the previous commit, which exceeded the line width once `as_ref()` became `as_deref()`. Verified: the three doctests CI reported (lib.rs lines 1641, 1781, 1929) now pass, `cargo test -p datafusion --doc` is 186 passed / 0 failed, and `cargo fmt --all --check` is clean. Co-Authored-By: Claude Opus 5 <[email protected]>
Remove commented-out code explaining async behavior.
alamb
changed the title
Option<&[usize]>TableProvider to take scan projection as Option<&[usize]>
Contributor
|
FWIW I could not reproduce the reported compilation speedup I had code run the PR's reported command,
Is there some other way you were testing? |
Contributor
Author
Yes, I tested it building the whole workspace and then looking at the contribution of datafusion-session. |
Contributor
Author
|
So the difference is: |
Contributor
I think it is ok too -- I was just trying to quantify eactly how much benefit there was for the API change |
Contributor
Author
Sorry this was Claude spamming a comment (I agree with the point as well though). The difference seems to be that incremental keeps this cached (depending on what has been changed) whereas a normal build (e.g. after a clean or dependency upgrade / change...) doesn't. |
imtherealnaska
pushed a commit
to imtherealnaska/datafusion
that referenced
this pull request
Aug 16, 2026
…ze]>` (apache#24376) ## Which issue does this PR close? - Addresses apache#13814 - Follow-on to apache#24325, apache#24326, apache#24329, apache#24330 ## Rationale for this change `TableProvider::scan` took its projection as `Option<&Vec<usize>>`. Besides being the signature `clippy::ptr_arg` exists to discourage, it forced `TableProvider::scan_with_args`'s default body to allocate: ```rust let projection = args.projection().map(|p| p.to_vec()); let plan = self.scan(state, projection.as_ref(), filters, limit).await?; ``` `ScanArgs::projection()` already yields `Option<&[usize]>`, so that `Vec` existed only to be borrowed. Worse, it cannot outlive a hoisted future, so the body had to stay an `async fn` — and that made it **the single most expensive thing to compile in `datafusion-session`**. Proving its coroutine `Send` walks the whole `Expr`/`LogicalPlan` type graph, and `#[async_trait]`'s `'life0: 'async_trait` bounds stop rustc serving that proof from its global cache. I measured this directly before writing the change: stubbing the body out dropped `datafusion-session`'s `evaluate_obligation` from **617ms to 16.5ms** and the whole crate from 0.86s to 0.235s, while the rest of the crate's trait solving attributed to just 4ms. So this one default body was ~97% of what remained after apache#24326. ## What changes are included in this PR? The projection becomes `Option<&[usize]>`, and the default body hands it straight through, capturing only the already-boxed future it awaits: ```rust let plan = self.scan( state, args.projection(), args.filters().unwrap_or(&[]), args.limit(), ); Box::pin(async move { Ok(plan.await?.into()) }) ``` Also updated, because their projections feed the same call paths: `StreamingTableExec::try_new`, `FilterExec::with_projection`, `batch_filter`. `datafusion_common::project_schema` now takes `Option<&T> where T: AsRef<[usize]> + ?Sized` instead of `Option<&impl AsRef<[usize]>>`, so it accepts a `Vec` *or* a slice — no caller of it needs to change. Call sites get simpler rather than noisier: `Some(&vec![2, 1])` becomes `Some(&[2, 1])`, and three `vec!` allocations in tests disappear (clippy pointed those out on its own). ## Are these changes tested? Yes ## Are there any user-facing changes? Yes — this is a **breaking change** for `TableProvider` implementors, so it has an upgrade-guide entry. Implementations only need the signature updated; two patterns inside a body need adjusting: ```rust // Before // After projection.cloned() projection.map(|p| p.to_vec()) ``` and callers holding an `Option<Vec<usize>>` pass `.as_deref()` rather than `.as_ref()`. In tree that was 47 signatures across 36 files, all mechanical. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
datafusioncore crate) #13814datafusion-catalogcompile time ~6x #24325, perf(session): cutdatafusion-sessioncompile time ~1.7x #24326, perf(core): cutdatafusioncore compile time ~10x #24329, perf(catalog-listing): cutdatafusion-catalog-listingcompile time ~4.4x #24330Rationale for this change
TableProvider::scantook its projection asOption<&Vec<usize>>. Besides beingthe signature
clippy::ptr_argexists to discourage, it forcedTableProvider::scan_with_args's default body to allocate:ScanArgs::projection()already yieldsOption<&[usize]>, so thatVecexistedonly to be borrowed. Worse, it cannot outlive a hoisted future, so the body had to
stay an
async fn— and that made it the single most expensive thing to compilein
datafusion-session. Proving its coroutineSendwalks the wholeExpr/LogicalPlantype graph, and#[async_trait]'s'life0: 'async_traitbounds stop rustc serving that proof from its global cache.
I measured this directly before writing the change: stubbing the body out dropped
datafusion-session'sevaluate_obligationfrom 617ms to 16.5ms and thewhole crate from 0.86s to 0.235s, while the rest of the crate's trait solving
attributed to just 4ms. So this one default body was ~97% of what remained after
#24326.
What changes are included in this PR?
The projection becomes
Option<&[usize]>, and the default body hands it straightthrough, capturing only the already-boxed future it awaits:
Also updated, because their projections feed the same call paths:
StreamingTableExec::try_new,FilterExec::with_projection,batch_filter.datafusion_common::project_schemanow takesOption<&T> where T: AsRef<[usize]> + ?Sizedinstead ofOption<&impl AsRef<[usize]>>, so it accepts aVecor a slice — no caller ofit needs to change.
Call sites get simpler rather than noisier:
Some(&vec![2, 1])becomesSome(&[2, 1]), and threevec!allocations in tests disappear (clippy pointedthose out on its own).
Are these changes tested?
Yes
Are there any user-facing changes?
Yes — this is a breaking change for
TableProviderimplementors, so it has anupgrade-guide entry. Implementations only need the signature updated; two patterns
inside a body need adjusting:
and callers holding an
Option<Vec<usize>>pass.as_deref()rather than.as_ref(). In tree that was 47 signatures across 36 files, all mechanical.🤖 Generated with Claude Code