adriangb opened a new pull request, #25027:
URL: https://github.com/apache/datafusion/pull/25027
## Which issue does this PR close?
- N/A — small benchmark cleanup, no issue filed.
## Rationale for this change
`datafusion/core/benches/sql_planner.rs` builds its ClickBench planning
benchmark
set from two hardcoded ranges:
```rust
let clickbench_queries = (0..=42)
.map(|q| /* queries/clickbench/queries/q{q}.sql */)
.chain((0..=7).map(|q| /* queries/clickbench/extended/q{q}.sql */))
```
`benchmarks/queries/clickbench/extended/` has grown since that `(0..=7)` was
written and now contains `q0.sql` through `q13.sql`, so extended queries
q8–q13
were never planned by this benchmark — six queries silently outside coverage.
The ranges are also a maintenance trap in both directions: adding a query
file
requires remembering to bump an unrelated literal, and because the read is
`std::fs::read_to_string(...).unwrap()`, a range that runs past the end of
the
directory panics the whole bench rather than stopping.
The `dfbench clickbench` runner does not have this problem — it discovers
query
files by walking `0..` and stopping at the first missing one
(`get_query_sql` /
`get_query_path` in `benchmarks/src/clickbench.rs`). This PR makes the
planning
benchmark use the same scheme.
## What changes are included in this PR?
- Adds a `read_numbered_queries(dir)` helper to `sql_planner.rs` that reads
`q{N}.sql` starting at `q0.sql` and stops at the first missing file.
- Replaces both hardcoded ranges — standard `(0..=42)` and extended
`(0..=7)` —
with calls to it. New query files are now picked up without a code change.
Two details on error handling, since the previous `.unwrap()` was
load-bearing:
- Only `ErrorKind::NotFound` ends the sequence. Any other IO error still
panics,
now with the offending path in the message.
- An `assert!(!queries.is_empty(), ...)` guards the case where
`benchmarks_path`
resolves somewhere wrong: without it, discovery would silently register
zero
ClickBench benchmarks, which is a worse failure than the old panic.
I also grepped for other places that hardcode a ClickBench extended query
count;
there are none. `benchmarks/bench.sh` passes `--queries-path` to the
`dfbench`
runner, which already discovers.
## What is the testing strategy for this PR?
This is benchmark-only code with no unit tests; verified by running the bench
target locally against a real ClickBench `hits_partitioned` dataset:
- `cargo bench -p datafusion --bench sql_planner -- --list` now enumerates
**57**
`physical_plan_clickbench_q*` benchmarks (43 standard + 14 extended), up
from
51, and does not panic.
- `cargo bench -p datafusion --bench sql_planner -- --quick
'physical_plan_clickbench_q5[2-7]$'`
— the six newly covered extended queries — all plan successfully
(6.4–10.7 ms each, unoptimized build).
- `cargo fmt --all` and `cargo clippy -p datafusion --benches -- -D
warnings` are
clean.
Note that benchmark IDs are positional (`q{index + 1}`), so the extended
queries
keep their existing IDs and the six new ones append as q52–q57; no existing
benchmark is renumbered.
## Are there any user-facing changes?
No. Benchmark harness only — no library code, no public API.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
