Repository navigation
Run all benchmarks on merge to main branch #15511
Description
Is your feature request related to a problem or challenge?
There has been a number of issues where benchmarks stopped working and no one noticed until someone happened to try and run them. This should be resolved and I suggest we do it in the github extended tests where we run all the benchmarks just to verify they actually run whenever a PR is merged into main.
Describe the solution you'd like
Add running of benchmarks in the datafusion extended tests github job.
Describe alternatives you've considered
No response
Additional context
No response
Activity
alamb commentedon Mar 31, 2025
I would persoanlly suggest if we are going to run the benchmarks we should also be actively tracking them / making sure they are measuring something useful.
Omega359 commentedon Mar 31, 2025
Sure. #5504
alamb commentedon Apr 1, 2025
I see what you are saying now: #15500 (comment)
I now agree it would be good to test these somehow
jayzhan211 commentedon Apr 1, 2025
Instead of running the benchmark, how about adding those benchmark query to tests, I don't think we need to actually "benchmark" the code for each merge.
Shreyaskr1409 commentedon Apr 1, 2025
The issue #5504 would require all benchmarks to run after each merge. I think we could just add benchmarks directly for now. What do you think?
I am willing to work on* this.
Shreyaskr1409 commentedon Apr 1, 2025
take
Omega359 commentedon Apr 1, 2025
We could do that too however not all benchmarks are sql queries.
jayzhan211 commentedon Apr 1, 2025
I don't think we need to run the benchmark on CI, at least it should be optional and disable by default.
I agree that maintaining it is challenging. Adding it to the extended test suite and running it on every merge isn’t a viable solution, as it’s costly and often unnecessary. I don’t think keeping the benchmark functional is essential—it’s more like a script that we can modify as needed, depending on what we want to measure each time
Omega359 commentedon Apr 1, 2025
I disagree. If it's in the code base it should work or it should be marked as 'experimental' imho. No one wants to have to go through and fix a benchmark anytime they happen to want to check something.
berkaysynnada commentedon Apr 3, 2025
Keeping all benchmark coverage with their replicas is challenging (as @Omega359 said not all benchmarks are sql queries) and very prone to go out of sync.
You are also right here about the cost, but what if we can have 2 modes for benchmarks, one for the actual benchmarking purpose, and one with just to validate. If it is in validation mode, it will work with very low loads -- e.g. min sampling count, min batch sizes etc.
cargo test (or extended tests) would run the validation mode, and normal benchmarking goes with the standard mode.
jayzhan211 commentedon Apr 4, 2025
This is what I said so I agree with this
We mirror the benchmark code as the test, so we run the test and make sure it works, but we don't actually run the benchmark.
If it is sql query, we can add it in sqllogictest crate, if it is not we add it to rust test.
vito1317 commentedon Jul 14, 2026
take
Proposal for a first increment, keeping the CI-cost concerns discussed above in mind:
benchmarks-smokejob in the extended tests workflow, gated to merges tomain/ release branches and manual dispatch (not PRs)cargo checkall benchmark bins and benches first — this alone catches the most common "benchmark silently broken" failures cheaplytpch,tpch_mem,sort_tpch,external_aggron TPC-H SF1 (generated with tpchgen-cli), pluscancellation(generates its own data)bench.shfix: the TPC-H answers copy useddocker run -it, which fails without a TTY in CIBenchmarks needing multi-GB downloads (clickbench, h2o, imdb, tpcds) are left out of the first pass; coverage can be extended once the approach looks right. PR incoming.
vito1317 commentedon Jul 14, 2026
take