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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
