perf(session): cut datafusion-session compile time ~1.7x - #24326
Merged
Merged
Conversation
Dandandan
force-pushed
the
perf/session-compile-time
branch
from
August 13, 2026 15:07
e73053f to
b811029
Compare
AdamGS
reviewed
Aug 13, 2026
Dandandan
force-pushed
the
perf/session-compile-time
branch
from
August 13, 2026 15:26
b811029 to
d86174e
Compare
datafusion-session compile time ~1.6xdatafusion-session compile time ~1.7x
AdamGS
approved these changes
Aug 13, 2026
comphead
reviewed
Aug 13, 2026
comphead
approved these changes
Aug 13, 2026
Dandandan
added a commit
to Dandandan/arrow-datafusion
that referenced
this pull request
Aug 13, 2026
Third and largest instance of the problem from apache#24325 and apache#24326. 76% of this crate's compile time is the trait solver (`evaluate_obligation`), 75.0s of it proving `Send`/`Sync`. `#[async_trait]` gives each `async fn` a `where 'life0: 'async_trait, ..` clause. rustc only serves auto-trait obligations from its global evaluation cache when the `ParamEnv` is empty, so the `Send`/`Sync` proof for everything the returned future captures is redone per method. In this crate the captured sets include `SessionState`, `&LogicalPlan` and `ListingTableConfig`, each of which reaches a large fraction of the logical-plan type graph. Grouping the goals by the `Self` type in their `ParamEnv` shows how concentrated it is -- 14 impls, top 10 = 78%: 7.73s ParquetReadOptions 5.18s DynamicListTableFactory 7.54s JsonReadOptions 4.56s ListingTableFactory 7.07s DefaultPhysicalPlanner 4.47s TestTableFactory 6.59s CsvReadOptions 4.30s ListingTableConfig 5.68s DataFrameTableProvider 4.28s DefaultQueryPlanner 5.37s (trait default bodies) 4.15s DefaultTableFactory 4.11s SessionState 3.72s ArrowReadOptions For contrast, in the same compile 31,818 goals with an empty `ParamEnv` cost 0.19s in total -- 6us each, against ~1.3ms for the same kind of goal under `async_trait`'s bounds. Each of those methods is now the hand-written desugaring of `async fn`, which only forwards; the coroutine is built in a shim with no where-clauses so its proofs land in the global cache. The `ReadOptions` family collapses to a single proof: all five impls already delegated to the `_get_resolved_schema` default body, which now hands the coroutine to a free `infer_schema_boxed`. Bodies are moved verbatim; ones with no `.await` become plain fns wrapped in `ready(..)`. Note `DefaultPhysicalPlanner::create_initial_plan` already used exactly this shape (`-> BoxFuture<'a, _>` plus `Box::pin(async move ..)`), there for recursion rather than for compile time. Interleaved A/B of `cargo rustc -p datafusion --lib`: base: 74.4s 69.9s fix: 16.6s 15.5s A third pair ran under heavy load from a concurrent build (base 219.8s, fix 32.5s) and is excluded; its ratio was consistent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24326 +/- ##
==========================================
- Coverage 81.14% 81.14% -0.01%
==========================================
Files 1112 1112
Lines 386933 386973 +40
Branches 386933 386973 +40
==========================================
+ Hits 313967 313996 +29
- Misses 54476 54480 +4
- Partials 18490 18497 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Same cause as apache#24325. `#[async_trait]` gives each `async fn` a `where 'life0: 'async_trait, .., Self: 'async_trait` clause, and rustc only serves auto-trait obligations from its global evaluation cache when the `ParamEnv` is empty. So the `Send`/`Sync` proof for everything the returned future captures is redone per method. Here the cost is in trait *declarations* rather than impls: grouping the goals by the `Self` type in their `ParamEnv` shows 4.30s of 4.32s under a generic `Self`, i.e. in `async fn`s with default bodies. Three of those defaults -- `TableProvider::{delete_from, update, merge_into}` -- are one-line stubs that nevertheless build a coroutine capturing `Vec<Expr>` / `Expr`, and proving that coroutine `Send` walks the whole `Expr`/`LogicalPlan` type graph. They are now written as the desugaring of `async fn` returning `ready(..)`, so no coroutine is created and there is nothing expensive to prove. The signatures are exactly what `#[async_trait]` generates (verified against `-Zunpretty=expanded`), so implementors are unaffected. Measured per method, by reverting one at a time (`evaluate_obligation` self time, all-converted baseline 659ms): delete_from 1.29s (+631ms) update 1.30s (+641ms) merge_into 1.29s (+631ms) truncate 697ms (+38ms) insert_into 665ms (~0) Only the methods that actually take `Expr` matter, so `insert_into`, `truncate`, and the two `planner.rs` bodies (which measured ~0 as well) are left as `async fn`. Interleaved A/B of `cargo rustc -p datafusion-session --lib`, 3 pairs: base: 1.582s 1.576s 1.590s fix: 0.933s 0.938s 0.932s `evaluate_obligation` drops 1.28s -> 0.646s. `scan_with_args` is left alone too: its default body needs an owned projection (`scan` takes `Option<&Vec<usize>>` while `ScanArgs::projection` yields `&[usize]`), so the local cannot outlive a hoisted future. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dandandan
force-pushed
the
perf/session-compile-time
branch
from
August 13, 2026 16:59
d86174e to
2ff86da
Compare
Dandandan
added a commit
to Dandandan/arrow-datafusion
that referenced
this pull request
Aug 13, 2026
…~4.4x Fourth and last crate in the family from apache#24325, apache#24326 and apache#24329. 58% of this crate's compile time was the trait solver (`evaluate_obligation`), and all 3.16s of it came from the single `impl TableProvider for ListingTable`. `#[async_trait]` gives each `async fn` a `where 'life0: 'async_trait, ..` clause. rustc only serves auto-trait obligations from its global evaluation cache when the `ParamEnv` is empty, so the `Send`/`Sync` proof for everything the returned future captures is redone per method. All three async methods here reach `Expr` -- `scan` takes `&[Expr]`, `scan_with_args` takes `ScanArgs<'a>` which holds `&[Expr]`, and `insert_into`'s body keeps `self.options` (`Vec<Vec<SortExpr>>`) live across an await -- so each pays for a walk of the whole `Expr`/`LogicalPlan` graph. Each method is now the hand-written desugaring of `async fn` and only forwards; the coroutine is built in a shim with no where-clauses, so its proofs land in the global cache. Bodies are moved verbatim into inherent fns, all three still `async`, so nothing is evaluated any earlier than before. Measured per method, by reverting one at a time (with its helpers) from the all-converted state: all three converted 0.931s obligations 34.9ms revert insert_into 1.866s obligations 1.01s revert scan 1.943s obligations 1.05s revert scan_with_args 2.118s obligations 1.17s none converted (base) 4.201s obligations 3.25s Unlike apache#24326, all three pull their weight: converting all of them leaves no coroutine in the impl at all, so the graph is never walked in a non-empty `ParamEnv`. Interleaved A/B of `cargo rustc -p datafusion-catalog-listing --lib`, 3 pairs: base: 4.257s 4.189s 4.134s fix: 0.984s 0.935s 0.980s `evaluate_obligation` drops 3.25s -> 34.7ms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 13, 2026
ryux1
pushed a commit
to ryux1/datafusion
that referenced
this pull request
Aug 13, 2026
## Which issue does this PR close? Adresses: apache#13814 First of four; see also apache#24326 (`datafusion-session`), apache#24329 (`datafusion` core) and apache#24330 (`datafusion-catalog-listing`). All independent — different crates, so they can merge in any order. ## Rationale for this change `datafusion-catalog` is only 4.9k lines of source, but it takes **42s** of a cold `cargo build -p datafusion` (measured with `cargo build --timings`). `-Zself-profile` says ~90% of the crate's compile time is `evaluate_obligation`, and ~99% of that is proving `Send`/`Sync`: | trait | time | goals | |---|---|---| | `Send` | 4.04s | 8245 | | `Sync` | 3.97s | 8195 | | everything else | 0.03s | 7355 | 51.8s of trait solving: | impl | trait solving | |---|---| | `MemTable` | 13.7s | | `StreamTable` | 9.0s | | `CteWorkTable` | 8.9s | | `StreamWrite` | 6.9s | | `StreamTableFactory` | 4.5s | | `ViewTable` | 4.4s | | `StreamingTable` | 4.4s | ## What changes are included in this PR? For those impls, the future is now constructed in a small shim function that has **no** where-clauses, so its auto-trait obligations are proved in an empty `ParamEnv` and get cached globally. The trait method is left as a hand-written desugaring of what `#[async_trait]` would have generated, and only forwards — it never creates a coroutine of its own, so it does no auto-trait work. Isolated probe confirming the shape is what matters (5 trivial impls of a local `#[async_trait]` trait taking `&[Expr]`, added to this crate): | variant | crate build | cost of the 5 impls | |---|---|---| | no impls (baseline) | 8.31s | — | | `#[async_trait]` + `async fn` | 11.64s | +3.33s | | `async fn` delegating body to a boxed helper | 14.28s | +5.97s | | desugared signature + boxed shim | 8.01s | ~0 | Note the middle row: moving only the *body* out makes things worse. The `async fn` itself has to go, because its arguments are what the future captures. ## Are these changes tested? The change is mechanical and the compiler checks each rewritten signature against the trait declaration. Interleaved A/B of `cargo rustc -p datafusion-catalog --lib`, alternating 3 times so machine drift cancels out: ``` before: 8.08s 7.90s 7.63s after: 1.77s 1.70s 1.69s ``` `evaluate_obligation` drops from **7.31s to 70ms**, and its goal count from 25,811 to 14,934. In a full `cargo build -p datafusion` the crate's unit goes from 42.3s to ~8s; since it sits alone on the critical path, that time comes straight off the build's wall clock. ## Are there any user-facing changes? No. No public signature changes — after macro expansion the trait methods have the same signatures as before. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
ryux1
pushed a commit
to ryux1/datafusion
that referenced
this pull request
Aug 13, 2026
## Which issue does this PR close? Adresses: apache#13814 Third and largest instance of the problem from apache#24325 (`datafusion-catalog`), apache#24326 (`datafusion-session`) and apache#24330 (`datafusion-catalog-listing`). Independent of all of them — different crates, so they can merge in any order. ## Rationale for this change `datafusion` core is the last unit of a cold `cargo build -p datafusion` and compiles alone, so its cost lands directly on the build's wall clock. **76% of its compile time was the trait solver**: `-Zself-profile` reported 75.0s of `evaluate_obligation` out of 98.6s total, essentially all of it proving `Send`/`Sync`. `#[async_trait]` gives each `async fn` a `where 'life0: 'async_trait, ..` clause. rustc only serves auto-trait obligations from its **global** evaluation cache when the `ParamEnv` is empty, so the `Send`/`Sync` proof for everything the returned future captures is redone per method. In this crate the captured sets include `SessionState`, `&LogicalPlan` and `ListingTableConfig`, each of which reaches a large fraction of the logical-plan type graph. Grouping the goals by the `Self` type in their `ParamEnv` shows how concentrated this was — 14 impls, top 10 = 78% of the total: | impl | trait solving | | impl | trait solving | |---|---|---|---|---| | `ParquetReadOptions` | 7.73s | | `DynamicListTableFactory` | 5.18s | | `JsonReadOptions` | 7.54s | | `ListingTableFactory` | 4.56s | | `DefaultPhysicalPlanner` | 7.07s | | `TestTableFactory` | 4.47s | | `CsvReadOptions` | 6.59s | | `ListingTableConfig` | 4.30s | | `DataFrameTableProvider` | 5.68s | | `DefaultQueryPlanner` | 4.28s | | *(trait default bodies)* | 5.37s | | `DefaultTableFactory` | 4.15s | | | | | `SessionState` | 4.11s | | | | | `ArrowReadOptions` | 3.72s | For contrast, in the same compile 31,818 goals with an **empty** `ParamEnv` cost 0.19s in total — 6µs each, against ~1.3ms for the same kind of goal under `async_trait`'s bounds. ## What changes are included in this PR? **First commit.** Each of those methods becomes the hand-written desugaring of `async fn`, which only forwards; the coroutine is built in a shim with no where-clauses, so its auto-trait obligations are proved in an empty `ParamEnv` and land in the global cache. Method bodies are moved verbatim into inherent fns. The `ReadOptions` family (25.6s across four impls, plus the 5.37s default body) collapses to a **single** proof: all five impls already delegated to the `_get_resolved_schema` default body, which now hands the coroutine to a free `infer_schema_boxed`. Because that helper is a plain function with no generics and no where-clauses, its proof is cached once and shared by every impl. **Second commit**, from re-profiling after the first. 11.15s of trait solving remained, in exactly two places: 1. `ListingTableConfigExt::infer` was still an `async fn` capturing `self: ListingTableConfig` (4.37s). It now uses the same shim as `infer_options` beside it. 2. `ReadOptions::_get_resolved_schema` still carried `Self: Sync`, which `#[async_trait]` needed while its body was a coroutine capturing `&self`. After the first commit it is neither, so the bound is dead weight — and it forced every caller to prove its own type `Sync` structurally, through arrow's `DataType`/`Schema`, in a non-empty `ParamEnv` (2.1–2.4s each for Csv/Json/Parquet; `ArrowReadOptions` was already cheap, having fewer fields). Two things worth noting for review: - `DefaultPhysicalPlanner::create_initial_plan` already used exactly this shape (`-> BoxFuture<'a, _>` plus `Box::pin(async move ..)`) — there for recursion rather than for compile time. The idiom is not new to this codebase. - The second commit **relaxes a bound on a public trait method**. Nothing in tree overrides `_get_resolved_schema` (all five impls only implement `get_resolved_schema`) and the underscore prefix marks it as internal, but an external override written with `#[async_trait]` would generate `Self: Sync` and no longer match. Happy to drop that commit if you would rather not touch it. One body became eager: `TestTableFactory::create_inner` has no `.await`, so it is a plain fn wrapped in `ready(..)`. It builds a `TestTableProvider` and has no side effects. Everything that awaits stays lazy — `Box::pin(self.m_inner(..))` polls nothing. ## Are these changes tested? - `cargo test -p datafusion --lib` — 442 passed - `cargo check -p datafusion --all-targets` — clean (covers core's integration tests and benches, heavy users of these APIs) - `cargo clippy -p datafusion --lib` — clean - `cargo doc` with `-D warnings` — clean - `cargo fmt --check` — clean The compiler checks each rewritten signature against its trait declaration, and every body is moved verbatim. `cargo rustc -p datafusion --lib` with `-Ztime-passes`, alternated with the base so machine drift cancels out: | | total | `evaluate_obligation` | |---|---|---| | base | 88.9s | 75.0s | | after first commit | 18.6s | 11.15s | | after second commit | **8.2s** | **234ms** | 234ms over 34,724 goals is 6.7µs each — the same rate as goals that carry an empty `ParamEnv`, i.e. the repeated proving is gone rather than merely reduced. What remains in this crate is LLVM: 7.6s emitting objects and 4.2s in LLVM passes. An earlier interleaved wall-clock A/B of the first commit alone measured 74.4s/69.9s base against 16.6s/15.5s fixed. ## Are there any user-facing changes? No, other than the relaxed `Self: Sync` bound described above. No public signature changes — after macro expansion these methods have the same signatures as before. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
ryux1
pushed a commit
to ryux1/datafusion
that referenced
this pull request
Aug 13, 2026
…~4.4x (apache#24330) ## Which issue does this PR close? Adresses: apache#13814 Fourth and last crate in the family from apache#24325 (`datafusion-catalog`), apache#24326 (`datafusion-session`) and apache#24329 (`datafusion` core). Independent of all three — different crates, so they can merge in any order. ## Rationale for this change `datafusion-catalog-listing` is 3,013 lines of source but spends **20.5s** in the frontend during a cold `cargo build -p datafusion` (`cargo build --timings`), and it sits on the critical path between `datafusion-catalog` and `datafusion` core. `-Zself-profile` puts 58% of the crate's compile time in `evaluate_obligation`, and grouping those goals by the `Self` type in their `ParamEnv` shows all of it in one impl: | `Self` in `ParamEnv` | time | goals | |---|---|---| | `ListingTable` | 3.16s | 6,967 | | *(empty `ParamEnv`)* | 0.04s | 8,443 | Note the second row — the same kind of goals cost ~5µs each with an empty `ParamEnv` against ~450µs here. The cause is the one from the earlier PRs: `#[async_trait]` gives each `async fn` a `where 'life0: 'async_trait, ..` clause, which makes the method's `ParamEnv` non-empty, and rustc only serves auto-trait obligations from its **global** evaluation cache when the `ParamEnv` is empty. So the `Send`/`Sync` proof for everything the future captures is redone per method. All three async methods in this impl reach `Expr`: - `scan` takes `&[Expr]` - `scan_with_args` takes `ScanArgs<'a>`, which holds `&[Expr]` - `insert_into` keeps `self.options` (`Vec<Vec<SortExpr>>`) live across an await so each one pays for a walk of the whole `Expr`/`LogicalPlan` graph. ## What changes are included in this PR? Each method is now the hand-written desugaring of `async fn` and only forwards; the coroutine is built in a shim with no where-clauses, so its proofs land in the global cache. Bodies are moved verbatim into inherent fns and all three stay `async`, so nothing is evaluated any earlier than before — `Box::pin(self.m_inner(..))` polls nothing. Following the review on apache#24326, I measured each method's marginal contribution first, by reverting one at a time (together with its helpers) from the all-converted state: | state | crate build | `evaluate_obligation` | |---|---|---| | all three converted | 0.931s | 34.9ms | | revert `insert_into` | 1.866s | 1.01s | | revert `scan` | 1.943s | 1.05s | | revert `scan_with_args` | 2.118s | 1.17s | | none converted (base) | 4.201s | 3.25s | Unlike apache#24326 — where two of the seven bodies I first converted turned out to gain nothing — all three pull their weight here. Converting all of them leaves no coroutine in the impl at all, so the graph is never walked in a non-empty `ParamEnv`, which is why the total drops by two orders of magnitude rather than by a third. ## Are these changes tested? - `cargo test -p datafusion-catalog-listing` — 18 + 7 passed - `cargo test -p datafusion --lib` — 442 passed - `cargo check -p datafusion --all-targets` — clean (`ListingTable` is used heavily by core's integration tests and benches) - `cargo clippy -p datafusion-catalog-listing --all-targets` — clean - `cargo fmt --check` — clean The compiler checks each rewritten signature against the trait declaration, and every body is moved verbatim. Interleaved A/B of `cargo rustc -p datafusion-catalog-listing --lib`, alternating 3 times so machine drift cancels out: ``` base: 4.257s 4.189s 4.134s fix: 0.984s 0.935s 0.980s ``` `evaluate_obligation` drops from 3.25s to 34.7ms. ## Are there any user-facing changes? No. No public signature changes — after macro expansion these methods have the same signatures as before. ### Follow-up With this, the four crates that made up the serial tail of a cold build are done. The general fix remains available and would cover downstream implementors too: drop `#[async_trait]` from these traits in favour of an explicit `BoxFuture` return with a single lifetime and no where-clauses, so that *every* impl is cheap without hand-desugaring. That is a breaking change to public traits, so it is out of scope here. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
oc7o
pushed a commit
to oc7o/datafusion
that referenced
this pull request
Aug 13, 2026
…e#24338) ## Which issue does this PR close? Adresses: apache#13814 Found while profiling compile times for apache#24325 / apache#24326 / apache#24329 / apache#24330. ## Rationale for this change `datafusion/core/src/bin/` holds three binaries that regenerate the docs under `docs/source/user-guide`: `print_config_docs`, `print_runtime_config_docs` and `print_functions_docs`. Cargo auto-discovers them and they have no `required-features`, so **every `cargo build` links all three** — each one ~174MB, since each links the whole `datafusion` rlib. Nothing in normal development uses them. They are run by `dev/update_config_docs.sh` and `dev/update_function_docs.sh`, and by the CI job that checks the committed docs are up to date. Two places where this shows up: **Cold builds.** The three binaries link *after* every other unit has finished, so they sit on the critical path with nothing to overlap with. `cargo build --timings` shows them occupying the last **3.5s** of a `cargo build -p datafusion` (~8.8s of CPU), after the last library unit completes. **The tightest inner loop** — touch a file in core, rebuild. All three are relinked every time: ``` before: 3.0s 2.4s after: 1.3s 1.1s ``` ## What changes are included in this PR? The three binaries move behind a new non-default `docs_generation` feature, and the two `dev/` scripts pass `--features docs_generation`. Using `required-features` means declaring the `[[bin]]` targets explicitly, since auto-discovered targets cannot carry it. ## Are these changes tested? - `cargo build -p datafusion` no longer produces the three binaries - `cargo build -p datafusion --features docs_generation` does - `./dev/update_config_docs.sh` still regenerates `docs/source/user-guide/configs.md` byte-identically (empty `git diff` afterwards), which is what the CI doc check compares `dev/update_function_docs.sh` uses the same invocation pattern and all three of its call sites were updated; CI exercises both scripts. ## Are there any user-facing changes? The three binaries are no longer built by a default `cargo build`. Anyone who ran them directly needs `--features docs_generation` — same as the `dev/` scripts now do. No library API changes. If you would rather these lived outside the published crate altogether, moving them to a small non-published `dev/` crate would have the same effect on build times; I went with the smaller change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
joroKr21
pushed a commit
to coralogix/arrow-datafusion
that referenced
this pull request
Aug 13, 2026
…ache#24339) ## Which issue does this PR close? Adresses: apache#13814 Found while profiling compile times for apache#24325 / apache#24326 / apache#24329 / apache#24330, which removed the trait-solving cost from the four crates on the critical path and left LLVM as the dominant remaining cost. ## Rationale for this change `dev` is the profile behind every `cargo build` and `cargo test`, so its debug info is generated over and over. `debug = "line-tables-only"` keeps file and line numbers — panics and `RUST_BACKTRACE` output stay just as useful — and drops the variable-level DWARF that only an interactive debugger consumes. Measured per crate, **interleaved** with the baseline so machine drift cancels out. The flag is passed to the crate under test only, so cached dependency artifacts stay valid and nothing else moves between the two measurements: | crate | `debug = 2` | `line-tables-only` | | |---|---|---|---| | `datafusion-physical-plan` | 8.89s | 7.43s | −16% | | `datafusion-functions-aggregate` | 5.86s | 4.63s | −21% | | `datafusion-physical-expr` | 4.57s | 3.59s | −21% | | `datafusion-functions` | 4.64s | 4.04s | −13% | | `datafusion-expr` | 4.54s | 3.57s | −21% | | `datafusion-functions-nested` | 4.37s | 3.06s | −30% | | `datafusion-optimizer` | 3.91s | 3.12s | −20% | | `datafusion-common` | 3.73s | 3.10s | −17% | | `datafusion-sql` | 3.57s | 2.43s | −32% | | `datafusion-datasource-parquet` | 3.27s | 2.44s | −25% | | `datafusion-datasource` | 1.90s | 1.42s | −25% | | `datafusion-physical-optimizer` | 1.22s | 0.99s | −19% | | **sum** | **50.5s** | **39.8s** | **−21%** | The saving is codegen-side, as you would expect: `datafusion-catalog`, which spends its time in the trait solver rather than in LLVM, moves only 7.4s → 7.0s. Artifacts shrink as well — `libdatafusion_physical_plan.rlib` goes from **141MB to 100MB**. ## What changes are included in this PR? One setting on `[profile.dev]`, plus an update to the profile documentation block above it, which currently advertises "full debug info" for `dev`. ## Are these changes tested? ## Are there any user-facing changes? For anyone stepping through DataFusion in a debugger, local variable inspection needs `CARGO_PROFILE_DEV_DEBUG=2 cargo build` (or a local override in `.cargo/config.toml`); the comment in `Cargo.toml` says so. Everything else — panic locations, backtraces, `#[test]` failures — is unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 14, 2026
AdamGS
pushed a commit
to sakshichitnis27/datafusion
that referenced
this pull request
Aug 14, 2026
…pache#24362) ## Which issue does this PR close? - Follow on to apache#24325 - Follow on to apache#24326 - Follow on to apache#24329 - Follow on to apache#24330 ## Rationale for this change @Dandandan came up with a very clever way to improve compile time, see apache#13814 (comment) However, the workaround (to make a special future and implement the async trait manually) is not obvious (at least to me) and I worry it might get removed in the future by accident, as we don't have any checks to prevent the regression Thus I think some comments explaining the pattern and how it would work will help avoid such a problem ## What changes are included in this PR? Add comments explaining why the code is done this way ## Are these changes tested? Comments only ## Are there any user-facing changes? No
pull Bot
pushed a commit
to TCeason/arrow-datafusion
that referenced
this pull request
Aug 14, 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 <noreply@anthropic.com>
imtherealnaska
pushed a commit
to imtherealnaska/datafusion
that referenced
this pull request
Aug 16, 2026
## Which issue does this PR close? Adresses: apache#13814 First of four; see also apache#24326 (`datafusion-session`), apache#24329 (`datafusion` core) and apache#24330 (`datafusion-catalog-listing`). All independent — different crates, so they can merge in any order. ## Rationale for this change `datafusion-catalog` is only 4.9k lines of source, but it takes **42s** of a cold `cargo build -p datafusion` (measured with `cargo build --timings`). `-Zself-profile` says ~90% of the crate's compile time is `evaluate_obligation`, and ~99% of that is proving `Send`/`Sync`: | trait | time | goals | |---|---|---| | `Send` | 4.04s | 8245 | | `Sync` | 3.97s | 8195 | | everything else | 0.03s | 7355 | 51.8s of trait solving: | impl | trait solving | |---|---| | `MemTable` | 13.7s | | `StreamTable` | 9.0s | | `CteWorkTable` | 8.9s | | `StreamWrite` | 6.9s | | `StreamTableFactory` | 4.5s | | `ViewTable` | 4.4s | | `StreamingTable` | 4.4s | ## What changes are included in this PR? For those impls, the future is now constructed in a small shim function that has **no** where-clauses, so its auto-trait obligations are proved in an empty `ParamEnv` and get cached globally. The trait method is left as a hand-written desugaring of what `#[async_trait]` would have generated, and only forwards — it never creates a coroutine of its own, so it does no auto-trait work. Isolated probe confirming the shape is what matters (5 trivial impls of a local `#[async_trait]` trait taking `&[Expr]`, added to this crate): | variant | crate build | cost of the 5 impls | |---|---|---| | no impls (baseline) | 8.31s | — | | `#[async_trait]` + `async fn` | 11.64s | +3.33s | | `async fn` delegating body to a boxed helper | 14.28s | +5.97s | | desugared signature + boxed shim | 8.01s | ~0 | Note the middle row: moving only the *body* out makes things worse. The `async fn` itself has to go, because its arguments are what the future captures. ## Are these changes tested? The change is mechanical and the compiler checks each rewritten signature against the trait declaration. Interleaved A/B of `cargo rustc -p datafusion-catalog --lib`, alternating 3 times so machine drift cancels out: ``` before: 8.08s 7.90s 7.63s after: 1.77s 1.70s 1.69s ``` `evaluate_obligation` drops from **7.31s to 70ms**, and its goal count from 25,811 to 14,934. In a full `cargo build -p datafusion` the crate's unit goes from 42.3s to ~8s; since it sits alone on the critical path, that time comes straight off the build's wall clock. ## Are there any user-facing changes? No. No public signature changes — after macro expansion the trait methods have the same signatures as before. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
imtherealnaska
pushed a commit
to imtherealnaska/datafusion
that referenced
this pull request
Aug 16, 2026
## Which issue does this PR close? Adresses: apache#13814 Third and largest instance of the problem from apache#24325 (`datafusion-catalog`), apache#24326 (`datafusion-session`) and apache#24330 (`datafusion-catalog-listing`). Independent of all of them — different crates, so they can merge in any order. ## Rationale for this change `datafusion` core is the last unit of a cold `cargo build -p datafusion` and compiles alone, so its cost lands directly on the build's wall clock. **76% of its compile time was the trait solver**: `-Zself-profile` reported 75.0s of `evaluate_obligation` out of 98.6s total, essentially all of it proving `Send`/`Sync`. `#[async_trait]` gives each `async fn` a `where 'life0: 'async_trait, ..` clause. rustc only serves auto-trait obligations from its **global** evaluation cache when the `ParamEnv` is empty, so the `Send`/`Sync` proof for everything the returned future captures is redone per method. In this crate the captured sets include `SessionState`, `&LogicalPlan` and `ListingTableConfig`, each of which reaches a large fraction of the logical-plan type graph. Grouping the goals by the `Self` type in their `ParamEnv` shows how concentrated this was — 14 impls, top 10 = 78% of the total: | impl | trait solving | | impl | trait solving | |---|---|---|---|---| | `ParquetReadOptions` | 7.73s | | `DynamicListTableFactory` | 5.18s | | `JsonReadOptions` | 7.54s | | `ListingTableFactory` | 4.56s | | `DefaultPhysicalPlanner` | 7.07s | | `TestTableFactory` | 4.47s | | `CsvReadOptions` | 6.59s | | `ListingTableConfig` | 4.30s | | `DataFrameTableProvider` | 5.68s | | `DefaultQueryPlanner` | 4.28s | | *(trait default bodies)* | 5.37s | | `DefaultTableFactory` | 4.15s | | | | | `SessionState` | 4.11s | | | | | `ArrowReadOptions` | 3.72s | For contrast, in the same compile 31,818 goals with an **empty** `ParamEnv` cost 0.19s in total — 6µs each, against ~1.3ms for the same kind of goal under `async_trait`'s bounds. ## What changes are included in this PR? **First commit.** Each of those methods becomes the hand-written desugaring of `async fn`, which only forwards; the coroutine is built in a shim with no where-clauses, so its auto-trait obligations are proved in an empty `ParamEnv` and land in the global cache. Method bodies are moved verbatim into inherent fns. The `ReadOptions` family (25.6s across four impls, plus the 5.37s default body) collapses to a **single** proof: all five impls already delegated to the `_get_resolved_schema` default body, which now hands the coroutine to a free `infer_schema_boxed`. Because that helper is a plain function with no generics and no where-clauses, its proof is cached once and shared by every impl. **Second commit**, from re-profiling after the first. 11.15s of trait solving remained, in exactly two places: 1. `ListingTableConfigExt::infer` was still an `async fn` capturing `self: ListingTableConfig` (4.37s). It now uses the same shim as `infer_options` beside it. 2. `ReadOptions::_get_resolved_schema` still carried `Self: Sync`, which `#[async_trait]` needed while its body was a coroutine capturing `&self`. After the first commit it is neither, so the bound is dead weight — and it forced every caller to prove its own type `Sync` structurally, through arrow's `DataType`/`Schema`, in a non-empty `ParamEnv` (2.1–2.4s each for Csv/Json/Parquet; `ArrowReadOptions` was already cheap, having fewer fields). Two things worth noting for review: - `DefaultPhysicalPlanner::create_initial_plan` already used exactly this shape (`-> BoxFuture<'a, _>` plus `Box::pin(async move ..)`) — there for recursion rather than for compile time. The idiom is not new to this codebase. - The second commit **relaxes a bound on a public trait method**. Nothing in tree overrides `_get_resolved_schema` (all five impls only implement `get_resolved_schema`) and the underscore prefix marks it as internal, but an external override written with `#[async_trait]` would generate `Self: Sync` and no longer match. Happy to drop that commit if you would rather not touch it. One body became eager: `TestTableFactory::create_inner` has no `.await`, so it is a plain fn wrapped in `ready(..)`. It builds a `TestTableProvider` and has no side effects. Everything that awaits stays lazy — `Box::pin(self.m_inner(..))` polls nothing. ## Are these changes tested? - `cargo test -p datafusion --lib` — 442 passed - `cargo check -p datafusion --all-targets` — clean (covers core's integration tests and benches, heavy users of these APIs) - `cargo clippy -p datafusion --lib` — clean - `cargo doc` with `-D warnings` — clean - `cargo fmt --check` — clean The compiler checks each rewritten signature against its trait declaration, and every body is moved verbatim. `cargo rustc -p datafusion --lib` with `-Ztime-passes`, alternated with the base so machine drift cancels out: | | total | `evaluate_obligation` | |---|---|---| | base | 88.9s | 75.0s | | after first commit | 18.6s | 11.15s | | after second commit | **8.2s** | **234ms** | 234ms over 34,724 goals is 6.7µs each — the same rate as goals that carry an empty `ParamEnv`, i.e. the repeated proving is gone rather than merely reduced. What remains in this crate is LLVM: 7.6s emitting objects and 4.2s in LLVM passes. An earlier interleaved wall-clock A/B of the first commit alone measured 74.4s/69.9s base against 16.6s/15.5s fixed. ## Are there any user-facing changes? No, other than the relaxed `Self: Sync` bound described above. No public signature changes — after macro expansion these methods have the same signatures as before. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
imtherealnaska
pushed a commit
to imtherealnaska/datafusion
that referenced
this pull request
Aug 16, 2026
…~4.4x (apache#24330) ## Which issue does this PR close? Adresses: apache#13814 Fourth and last crate in the family from apache#24325 (`datafusion-catalog`), apache#24326 (`datafusion-session`) and apache#24329 (`datafusion` core). Independent of all three — different crates, so they can merge in any order. ## Rationale for this change `datafusion-catalog-listing` is 3,013 lines of source but spends **20.5s** in the frontend during a cold `cargo build -p datafusion` (`cargo build --timings`), and it sits on the critical path between `datafusion-catalog` and `datafusion` core. `-Zself-profile` puts 58% of the crate's compile time in `evaluate_obligation`, and grouping those goals by the `Self` type in their `ParamEnv` shows all of it in one impl: | `Self` in `ParamEnv` | time | goals | |---|---|---| | `ListingTable` | 3.16s | 6,967 | | *(empty `ParamEnv`)* | 0.04s | 8,443 | Note the second row — the same kind of goals cost ~5µs each with an empty `ParamEnv` against ~450µs here. The cause is the one from the earlier PRs: `#[async_trait]` gives each `async fn` a `where 'life0: 'async_trait, ..` clause, which makes the method's `ParamEnv` non-empty, and rustc only serves auto-trait obligations from its **global** evaluation cache when the `ParamEnv` is empty. So the `Send`/`Sync` proof for everything the future captures is redone per method. All three async methods in this impl reach `Expr`: - `scan` takes `&[Expr]` - `scan_with_args` takes `ScanArgs<'a>`, which holds `&[Expr]` - `insert_into` keeps `self.options` (`Vec<Vec<SortExpr>>`) live across an await so each one pays for a walk of the whole `Expr`/`LogicalPlan` graph. ## What changes are included in this PR? Each method is now the hand-written desugaring of `async fn` and only forwards; the coroutine is built in a shim with no where-clauses, so its proofs land in the global cache. Bodies are moved verbatim into inherent fns and all three stay `async`, so nothing is evaluated any earlier than before — `Box::pin(self.m_inner(..))` polls nothing. Following the review on apache#24326, I measured each method's marginal contribution first, by reverting one at a time (together with its helpers) from the all-converted state: | state | crate build | `evaluate_obligation` | |---|---|---| | all three converted | 0.931s | 34.9ms | | revert `insert_into` | 1.866s | 1.01s | | revert `scan` | 1.943s | 1.05s | | revert `scan_with_args` | 2.118s | 1.17s | | none converted (base) | 4.201s | 3.25s | Unlike apache#24326 — where two of the seven bodies I first converted turned out to gain nothing — all three pull their weight here. Converting all of them leaves no coroutine in the impl at all, so the graph is never walked in a non-empty `ParamEnv`, which is why the total drops by two orders of magnitude rather than by a third. ## Are these changes tested? - `cargo test -p datafusion-catalog-listing` — 18 + 7 passed - `cargo test -p datafusion --lib` — 442 passed - `cargo check -p datafusion --all-targets` — clean (`ListingTable` is used heavily by core's integration tests and benches) - `cargo clippy -p datafusion-catalog-listing --all-targets` — clean - `cargo fmt --check` — clean The compiler checks each rewritten signature against the trait declaration, and every body is moved verbatim. Interleaved A/B of `cargo rustc -p datafusion-catalog-listing --lib`, alternating 3 times so machine drift cancels out: ``` base: 4.257s 4.189s 4.134s fix: 0.984s 0.935s 0.980s ``` `evaluate_obligation` drops from 3.25s to 34.7ms. ## Are there any user-facing changes? No. No public signature changes — after macro expansion these methods have the same signatures as before. ### Follow-up With this, the four crates that made up the serial tail of a cold build are done. The general fix remains available and would cover downstream implementors too: drop `#[async_trait]` from these traits in favour of an explicit `BoxFuture` return with a single lifetime and no where-clauses, so that *every* impl is cheap without hand-desugaring. That is a breaking change to public traits, so it is out of scope here. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
imtherealnaska
pushed a commit
to imtherealnaska/datafusion
that referenced
this pull request
Aug 16, 2026
) ## Which issue does this PR close? Adresses: apache#13814 ## Rationale for this change `datafusion-session` is 1,619 lines of source and spends **18.8s** in the frontend during a cold `cargo build -p datafusion` (`cargo build --timings`) — 11.6ms per line, the worst ratio in the workspace, against 0.09–0.55ms/line for crates like `datafusion-datasource` or `datafusion-physical-plan`. `-Zself-profile` puts ~72% of the crate's compile time in `evaluate_obligation`, and 98% of that in `Send`/`Sync`. ## What changes are included in this PR? `TableProvider::{delete_from, update, merge_into}` are one-line `not_impl_err!` stubs that nevertheless build a coroutine capturing `Vec<Expr>` / `Expr` — proving that coroutine `Send` walks the whole `Expr`/`LogicalPlan` type graph. They are now written as the desugaring of `async fn` returning `ready(..)`, so no coroutine is created and there is nothing expensive to prove. The signatures are exactly what `#[async_trait]` generates — verified against `-Zunpretty=expanded` output of this crate — so implementors are unaffected. I originally converted seven bodies, but measuring each one's marginal contribution (reverting one at a time; `evaluate_obligation` self time, all-converted baseline 659ms) showed only the ones taking `Expr` matter: | method left as `async fn` | trait solving | marginal cost | |---|---|---| | `delete_from` | 1.29s | +631ms | | `update` | 1.30s | +641ms | | `merge_into` | 1.29s | +631ms | | `truncate` | 697ms | +38ms | | `insert_into` | 665ms | ~0 | ## Are these changes tested? Interleaved A/B of `cargo rustc -p datafusion-session --lib` ``` base: 1.582s 1.576s 1.590s fix: 0.933s 0.938s 0.932s ``` `evaluate_obligation` drops from 1.28s to 0.646s. Those are standalone-build numbers. In the feature-unified build this crate's frontend is 18.8s rather than ~1.6s (each obligation is several times dearer there), so the absolute saving on a real build should be larger — I have not measured that directly, since isolating one crate's unit in a full build is hard to do without confounding it with machine drift. ## Are there any user-facing changes? No public signature changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
imtherealnaska
pushed a commit
to imtherealnaska/datafusion
that referenced
this pull request
Aug 16, 2026
…e#24338) ## Which issue does this PR close? Adresses: apache#13814 Found while profiling compile times for apache#24325 / apache#24326 / apache#24329 / apache#24330. ## Rationale for this change `datafusion/core/src/bin/` holds three binaries that regenerate the docs under `docs/source/user-guide`: `print_config_docs`, `print_runtime_config_docs` and `print_functions_docs`. Cargo auto-discovers them and they have no `required-features`, so **every `cargo build` links all three** — each one ~174MB, since each links the whole `datafusion` rlib. Nothing in normal development uses them. They are run by `dev/update_config_docs.sh` and `dev/update_function_docs.sh`, and by the CI job that checks the committed docs are up to date. Two places where this shows up: **Cold builds.** The three binaries link *after* every other unit has finished, so they sit on the critical path with nothing to overlap with. `cargo build --timings` shows them occupying the last **3.5s** of a `cargo build -p datafusion` (~8.8s of CPU), after the last library unit completes. **The tightest inner loop** — touch a file in core, rebuild. All three are relinked every time: ``` before: 3.0s 2.4s after: 1.3s 1.1s ``` ## What changes are included in this PR? The three binaries move behind a new non-default `docs_generation` feature, and the two `dev/` scripts pass `--features docs_generation`. Using `required-features` means declaring the `[[bin]]` targets explicitly, since auto-discovered targets cannot carry it. ## Are these changes tested? - `cargo build -p datafusion` no longer produces the three binaries - `cargo build -p datafusion --features docs_generation` does - `./dev/update_config_docs.sh` still regenerates `docs/source/user-guide/configs.md` byte-identically (empty `git diff` afterwards), which is what the CI doc check compares `dev/update_function_docs.sh` uses the same invocation pattern and all three of its call sites were updated; CI exercises both scripts. ## Are there any user-facing changes? The three binaries are no longer built by a default `cargo build`. Anyone who ran them directly needs `--features docs_generation` — same as the `dev/` scripts now do. No library API changes. If you would rather these lived outside the published crate altogether, moving them to a small non-published `dev/` crate would have the same effect on build times; I went with the smaller change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
imtherealnaska
pushed a commit
to imtherealnaska/datafusion
that referenced
this pull request
Aug 16, 2026
…ache#24339) ## Which issue does this PR close? Adresses: apache#13814 Found while profiling compile times for apache#24325 / apache#24326 / apache#24329 / apache#24330, which removed the trait-solving cost from the four crates on the critical path and left LLVM as the dominant remaining cost. ## Rationale for this change `dev` is the profile behind every `cargo build` and `cargo test`, so its debug info is generated over and over. `debug = "line-tables-only"` keeps file and line numbers — panics and `RUST_BACKTRACE` output stay just as useful — and drops the variable-level DWARF that only an interactive debugger consumes. Measured per crate, **interleaved** with the baseline so machine drift cancels out. The flag is passed to the crate under test only, so cached dependency artifacts stay valid and nothing else moves between the two measurements: | crate | `debug = 2` | `line-tables-only` | | |---|---|---|---| | `datafusion-physical-plan` | 8.89s | 7.43s | −16% | | `datafusion-functions-aggregate` | 5.86s | 4.63s | −21% | | `datafusion-physical-expr` | 4.57s | 3.59s | −21% | | `datafusion-functions` | 4.64s | 4.04s | −13% | | `datafusion-expr` | 4.54s | 3.57s | −21% | | `datafusion-functions-nested` | 4.37s | 3.06s | −30% | | `datafusion-optimizer` | 3.91s | 3.12s | −20% | | `datafusion-common` | 3.73s | 3.10s | −17% | | `datafusion-sql` | 3.57s | 2.43s | −32% | | `datafusion-datasource-parquet` | 3.27s | 2.44s | −25% | | `datafusion-datasource` | 1.90s | 1.42s | −25% | | `datafusion-physical-optimizer` | 1.22s | 0.99s | −19% | | **sum** | **50.5s** | **39.8s** | **−21%** | The saving is codegen-side, as you would expect: `datafusion-catalog`, which spends its time in the trait solver rather than in LLVM, moves only 7.4s → 7.0s. Artifacts shrink as well — `libdatafusion_physical_plan.rlib` goes from **141MB to 100MB**. ## What changes are included in this PR? One setting on `[profile.dev]`, plus an update to the profile documentation block above it, which currently advertises "full debug info" for `dev`. ## Are these changes tested? ## Are there any user-facing changes? For anyone stepping through DataFusion in a debugger, local variable inspection needs `CARGO_PROFILE_DEV_DEBUG=2 cargo build` (or a local override in `.cargo/config.toml`); the comment in `Cargo.toml` says so. Everything else — panic locations, backtraces, `#[test]` failures — is unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
imtherealnaska
pushed a commit
to imtherealnaska/datafusion
that referenced
this pull request
Aug 16, 2026
…pache#24362) ## Which issue does this PR close? - Follow on to apache#24325 - Follow on to apache#24326 - Follow on to apache#24329 - Follow on to apache#24330 ## Rationale for this change @Dandandan came up with a very clever way to improve compile time, see apache#13814 (comment) However, the workaround (to make a special future and implement the async trait manually) is not obvious (at least to me) and I worry it might get removed in the future by accident, as we don't have any checks to prevent the regression Thus I think some comments explaining the pattern and how it would work will help avoid such a problem ## What changes are included in this PR? Add comments explaining why the code is done this way ## Are these changes tested? Comments only ## Are there any user-facing changes? No
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 <noreply@anthropic.com>
Dandandan
added a commit
to emilk/datafusion
that referenced
this pull request
Aug 21, 2026
…nto 9 (apache#24342) ## Which issue does this PR close? None. Found while profiling compile times (apache#24325, apache#24326, apache#24329, apache#24330, apache#24338, apache#24339) — this is the largest remaining item I measured. ## Rationale for this change `datafusion-functions` declares **63 `[[bench]]` targets**. Each one is a separate binary that statically links this crate, arrow and criterion, so building the crate's benchmarks means 63 link steps. Linking, not compiling, is the bulk of it: rebuilding a **single** bench target takes **10.6s**, almost all of it link, while the 63 benchmark files together are only ~10.5k lines of source. This is paid by `cargo bench`, by `cargo build --all-targets`, and by CI's `clippy --all-targets`. ## What changes are included in this PR? The targets are grouped by their `required-features`, which preserves that semantics exactly — a group can only be built when its feature is on, just as each individual bench could before: | target | benches | required-features | |---|---|---| | `string_expressions` | 16 | `["string_expressions"]` | | `math_expressions` | 16 | `["math_expressions"]` | | `unicode_expressions` | 12 | `["unicode_expressions"]` | | `datetime_expressions` | 8 | `["datetime_expressions"]` | | `regex_expressions` | 4 | `["regex_expressions"]` | | `misc` | 4 | none | `crypto`, `encoding` and `dictionary_encoding` stay standalone — they are the only members of their feature groups, so grouping them would buy nothing. Each group lives in its own directory, `benches/<group>/`, with `main.rs` declaring the individual benchmarks as modules and listing their criterion groups in a single `criterion_main!`. Cargo does not auto-discover targets inside those directories, so no `autobenches = false` is needed — which also keeps `cargo machete` working (it stops scanning `benches/` when autodiscovery is disabled). `benches/helper.rs` stays shared at the top level, reached with `#[path = "../helper.rs"]`. **The benchmark code itself is untouched.** There is not a single change to a `bench_function`, `benchmark_group` or `bench_with_input` line anywhere in the diff — git reports every moved file as a 94–99% similarity rename. Per file the change is only: - drop `criterion_main!` — a module cannot define `main` - drop the now-unused `criterion_main` import - drop `extern crate criterion` (a no-op since edition 2018, present in 4 files) - point the four users of `benches/helper.rs` at `crate::helper` ## Are these changes tested? Interleaved with `main` so machine drift cancels out: | `cargo clean -p datafusion-functions` then… | main | this PR | |---|---|---| | `cargo build --benches` | 43.9s / 42.9s | **28.4s / 28.3s** | | `cargo check --benches` | 15.0s / 14.7s | **12.9s / 12.8s** | Every benchmark still runs, and the counts show nothing was dropped: | target | benchmarks (`-- --test`) | |---|---| | `string_expressions` | 252 | | `unicode_expressions` | 251 | | `datetime_expressions` | 103 | | `dictionary_encoding` | 36 | | `regex_expressions` | 27 | | `misc` | 21 | | `encoding` | 9 | | `crypto` | 10 (with `--features crypto_expressions`) | Also clean: `cargo clippy -p datafusion-functions --all-targets`, `cargo fmt --check`, and `cargo machete --with-metadata`. Filtering a single benchmark still works — `cargo bench --bench math_expressions -- power` selects exactly one. `cargo metadata` reports 10 bench targets, against 64 on `main`; both counts include the auto-discovered `helper`, which is unchanged by this PR. ## Are there any user-facing changes? Only for running an individual benchmark. Where you would previously write ``` cargo bench -p datafusion-functions --bench lower ``` you now select the group and filter by name: ``` cargo bench -p datafusion-functions --bench string_expressions -- lower ``` The filter is criterion's own, matching on benchmark id, so it also narrows to a single case within a benchmark. No library changes. If this approach looks right, roughly 100 more bench targets across the other crates could get the same treatment — I did this one crate first so the tradeoff is visible on a real diff before it spreads. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
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?
Adresses: #13814
Rationale for this change
datafusion-sessionis 1,619 lines of source and spends 18.8s in thefrontend during a cold
cargo build -p datafusion(cargo build --timings) —11.6ms per line, the worst ratio in the workspace, against 0.09–0.55ms/line for
crates like
datafusion-datasourceordatafusion-physical-plan.-Zself-profileputs ~72% of the crate's compile time inevaluate_obligation,and 98% of that in
Send/Sync.What changes are included in this PR?
TableProvider::{delete_from, update, merge_into}are one-linenot_impl_err!stubs that nevertheless build a coroutine capturing
Vec<Expr>/Expr—proving that coroutine
Sendwalks the wholeExpr/LogicalPlantype graph.They are now written as the desugaring of
async fnreturningready(..), sono coroutine is created and there is nothing expensive to prove. The signatures
are exactly what
#[async_trait]generates — verified against-Zunpretty=expandedoutput of this crate — so implementors are unaffected.I originally converted seven bodies, but measuring each one's marginal
contribution (reverting one at a time;
evaluate_obligationself time,all-converted baseline 659ms) showed only the ones taking
Exprmatter:async fndelete_fromupdatemerge_intotruncateinsert_intoAre these changes tested?
Interleaved A/B of
cargo rustc -p datafusion-session --libevaluate_obligationdrops from 1.28s to 0.646s.Those are standalone-build numbers. In the feature-unified build this crate's
frontend is 18.8s rather than ~1.6s (each obligation is several times dearer
there), so the absolute saving on a real build should be larger — I have not
measured that directly, since isolating one crate's unit in a full build is hard
to do without confounding it with machine drift.
Are there any user-facing changes?
No public signature changes
🤖 Generated with Claude Code