Dandandan commented on PR #24376: URL: https://github.com/apache/datafusion/pull/24376#issuecomment-5298163126
Thanks for checking — and you're right that with that command there is nothing to see. I reproduced your result here: **0.36s on both revisions**, three runs each. So it is not a machine difference; we measured different build paths. `touch lib.rs` + rebuild with incremental compilation on (cargo's default for `dev`) leaves the crate's *content* unchanged, so rustc's dep graph comes up green and it replays cached query results — the trait solving this PR removes never runs. The quickest way to see that: a cold compile of the crate spends **620–700ms inside `evaluate_obligation`**, which is more than the entire 0.36s rebuild we both measured, so that rebuild cannot be doing the work. My numbers came from a cold compile of the crate with everything upstream warm: ```bash cargo clean -p datafusion-session CARGO_INCREMENTAL=0 cargo rustc -p datafusion-session --lib ``` which gives 0.890s → 0.273s over 3 interleaved rounds, `evaluate_obligation` 624ms → 10ms. My PR description quoted `cargo rustc -p <crate> --lib` **without** those two lines. That omission is mine and it made the claim look broader than it is; I am fixing the description. I then went looking for a *warm* scenario where the change does help, and did not find one. Touching an upstream crate so `datafusion-session` genuinely rebuilds, incremental cache intact: | `datafusion-session` unit (from `--timings`) | merge-base | this PR | |---|---:|---:| | after touching `datafusion/expr/src/lib.rs`, run 1 | 0.25s | 0.27s | | run 2 | 0.29s | 0.29s | No difference. The incremental cache absorbs this cost whenever it exists. Where it does land is non-incremental compilation — which is what CI does, since `[profile.ci]` sets `incremental = false`, so every CI build pays the full ~0.6s in this crate — plus fresh clones and anything after `cargo clean`. That is a much narrower claim than the PR made. I still think `Option<&[usize]>` is the better signature on its own merits — it is what `clippy::ptr_arg` exists to discourage, and it lets the default `scan_with_args` body drop a `Vec` allocation and stop capturing it in the returned future — but if the compile-time argument is what is carrying a breaking change to `TableProvider::scan`, then it should not be, and I am happy for this to be closed on that basis. -- 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]
