You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2026/08/14 14:07:20 UTC
[PR] perf: desugar remaining `#[async_trait]` stubs capturing `Expr`/`LogicalPlan` [datafusion]
alamb opened a new pull request, #24370:
URL: https://github.com/apache/datafusion/pull/24370
## Which issue does this PR close?
- Addresses https://github.com/apache/datafusion/issues/13814
- Follow on to #24325, #24326, #24329 and #24330
## Rationale for this change
Applies the same rewrite as the four PRs above to the remaining `#[async_trait]` methods on the serial build path whose futures capture `Expr`/`LogicalPlan`-reaching types (so their `Send`/`Sync` proofs are re-proved per method in a non-empty `ParamEnv`):
- `UnsupportedQueryPlanner::create_physical_plan` (`datafusion-session`) — captures `&LogicalPlan`
- `ExtensionPlanner::plan_table_scan` default body (`datafusion-session`) — captures `&TableScan`, which holds `Vec<Expr>`
- `EmptyTable::scan` (`datafusion-catalog`) — captures `&[Expr]`
- `TestTableProvider::scan` (`datafusion` core; `test_util` compiles into the lib unconditionally) — captures `&[Expr]`
## What changes are included in this PR?
Each method becomes the hand-written desugaring of what `#[async_trait]` generates (verified against `-Zunpretty=expanded` output), with no coroutine capturing the arguments:
- The stubs (`create_physical_plan`, `plan_table_scan`, `TestTableProvider::scan`) return `ready(..)` or a captureless `async` block directly.
- `EmptyTable::scan` has no `.await`, so its body moves verbatim to a plain inherent fn wrapped in `ready(..)` (same as `TestTableFactory::create_inner` in #24329).
- `plan_table_scan` keeps its `Self: Sync + 'async_trait` bound so the public trait signature is unchanged.
Each converted method carries the explanatory comment from #24362.
## Are these changes tested?
Functionally: `cargo fmt --check`, `cargo clippy -p datafusion-catalog -p datafusion-session --all-targets`, `cargo clippy -p datafusion --lib`, `cargo test -p datafusion-catalog -p datafusion-session`, and `cargo test -p datafusion --lib` all pass. The compiler checks each rewritten signature against its trait declaration.
Compile-time impact is **not yet measured** (my machine is too noisy); this PR is a draft until the numbers below are collected.
<details>
<summary>Compile-time measurements to run (on a quiet machine)</summary>
Interleaved A/B of each affected crate's unit, alternating `main` and this branch at least 3 times each so machine drift cancels out (the methodology from #24325–#24330):
```bash
# once per checkout: warm the dependency graph
cargo build -p datafusion
# one round (repeat >=3x, alternating branches):
cargo clean -p datafusion-session -p datafusion-catalog -p datafusion
time cargo rustc -p datafusion-session --lib
time cargo rustc -p datafusion-catalog --lib
time cargo rustc -p datafusion --lib
```
Expected: a drop in `datafusion-session` (which had ~0.65s of `evaluate_obligation` remaining after #24326 in the standalone build, several times more in the feature-unified build) and a smaller drop in `datafusion-catalog` and core. `EmptyTable`/`TestTableProvider` are unproven — they did not appear in the per-impl profiles of #24325/#24329 — so it is worth confirming their marginal contribution before merging, e.g. by reverting one file at a time.
To attribute the change to the trait solver rather than noise:
```bash
# cargo install --git https://github.com/rust-lang/measureme summarize
RUSTFLAGS="-Zself-profile=/tmp/self-profile" cargo +nightly rustc -p datafusion-session --lib
summarize summarize $(ls -t /tmp/self-profile/*.mm_profdata | head -1) | grep -E "evaluate_obligation|Total"
```
</details>
## Are there any user-facing changes?
No. After macro expansion the method signatures are identical to before, including `plan_table_scan`'s `Self: Sync` bound.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: desugar remaining `#[async_trait]` stubs capturing `Expr`/`LogicalPlan` [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24370:
URL: https://github.com/apache/datafusion/pull/24370#issuecomment-5294487399
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24370?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `28.07018%` with `41 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.18%. Comparing base ([`571477c`](https://app.codecov.io/gh/apache/datafusion/commit/571477c4a7a532fcba67d3e272a3791755dedd79?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d868efb`](https://app.codecov.io/gh/apache/datafusion/commit/d868efbb8e357e3f1763cba520820f6fe1671925?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24370?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/session/src/planner.rs](https://app.codecov.io/gh/apache/datafusion/pull/24370?src=pr&el=tree&filepath=datafusion%2Fsession%2Fsrc%2Fplanner.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zZXNzaW9uL3NyYy9wbGFubmVyLnJz) | 0.00% | [28 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24370?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/core/src/test\_util/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/24370?src=pr&el=tree&filepath=datafusion%2Fcore%2Fsrc%2Ftest_util%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb3JlL3NyYy90ZXN0X3V0aWwvbW9kLnJz) | 0.00% | [13 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24370?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24370 +/- ##
==========================================
- Coverage 81.19% 81.18% -0.02%
==========================================
Files 1110 1110
Lines 388616 388677 +61
Branches 388616 388677 +61
==========================================
Hits 315534 315534
- Misses 54500 54555 +55
- Partials 18582 18588 +6
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24370?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: desugar remaining `#[async_trait]` stubs capturing `Expr`/`LogicalPlan` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24370:
URL: https://github.com/apache/datafusion/pull/24370#issuecomment-5294924792
I had codex run some tests on a gcp machine and this PR doesn't improve compialtion speed
Details below:
Compared PR head d868efbb8e against its base 571477c4a7, using 5 interleaved rounds. A full `cargo clean` and warm build were performed for each checkout; affected package artifacts were cleaned before each timed round.
| Crate | Main times (s) | PR times (s) | Main mean (s) | PR mean (s) |
|---|---:|---:|---:|---:|
| `datafusion-session` | 36.56, 1.44, 1.44, 1.43, 1.46 | 36.35, 1.46, 1.46, 1.46, 1.46 | 8.466 | 8.438 |
| `datafusion-catalog` | 36.49, 3.47, 3.48, 3.58, 3.47 | 36.53, 3.50, 3.51, 3.59, 3.52 | 10.098 | 10.130 |
| `datafusion` | 14.51, 14.43, 14.46, 14.41, 14.45 | 14.56, 14.50, 14.44, 14.44, 14.49 | 14.452 | 14.486 |
The first session/catalog measurements are cold-build outliers. Excluding round 1, the PR was approximately 1.2% slower for session, 0.9% slower for catalog, and 0.2% slower for core—within normal timing noise on this machine.
<details>
<summary>Exact benchmark commands</summary>
```bash
# once per checkout
cargo clean
cargo build -p datafusion --quiet
# each round, before the timed commands
cargo clean -p datafusion-session -p datafusion-catalog -p datafusion --quiet
# timed commands
/usr/bin/time -f "RESULT package=datafusion-session seconds=%e" cargo rustc -p datafusion-session --lib --quiet
/usr/bin/time -f "RESULT package=datafusion-catalog seconds=%e" cargo rustc -p datafusion-catalog --lib --quiet
/usr/bin/time -f "RESULT package=datafusion seconds=%e" cargo rustc -p datafusion --lib --quiet
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: desugar remaining `#[async_trait]` stubs capturing `Expr`/`LogicalPlan` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #24370: perf: desugar remaining `#[async_trait]` stubs capturing `Expr`/`LogicalPlan`
URL: https://github.com/apache/datafusion/pull/24370
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org