Benchmark queries are duplicated across three trees with nothing checking that the copies agree
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 11h
- Merged PRs (30d)
- 362
Description
### Is your feature request related to a problem or challenge?
The same benchmark query is stored, as literal SQL text, in up to three trees. Nothing checks that the copies agree, so they drift, and they have.
For ClickBench:
| copy | consumed by |
| --- | --- |
| `benchmarks/queries/clickbench/{queries,extended}/qN.sql` | `dfbench clickbench --queries-path` |
| `benchmarks/sql_benchmarks/clickbench{,_extended}/benchmarks/qNN.benchmark` | `benchmark_runner clickbench_extended` |
| `datafusion/sqllogictest/test_files/clickbench{,_extended}.slt` | CI, against the committed `clickbench_hits_10.parquet` fixture |
`benchmarks/queries/imdb/*.sql` and `benchmarks/sql_benchmarks/imdb/benchmarks/*.benchmark` are the same arrangement, 113 queries on each side.
### This has already drifted
**A query that is not the same query.** Extended q6 differs between two of its copies today. `benchmarks/sql_benchmarks/clickbench_extended/benchmarks/q06.benchmark` wraps the `URL` column in a cast that the other two copies do not have:
```sql
-- benchmarks/queries/clickbench/extended/q6.sql, and the .slt copy
CASE WHEN split_part(split_part("URL", 'resolution=', 2), '&', 1) ~ '^\d+$' ...
-- benchmarks/sql_benchmarks/clickbench_extended/benchmarks/q06.benchmark
CASE WHEN split_part(split_part(CAST("URL" AS STRING), 'resolution=', 2), '&', 1) ~ '^\d+$' ...
```
The cast arrived when that copy was created in #22804 and was never applied to the other two. So "extended q6" has named two different queries for about three months, and the two runners have been reporting timings for different work under one name. Whether the cast is wanted is a separate question; the point is that nothing surfaced the difference.
**Copies that fall behind.** Before #25026, `clickbench_extended.slt` covered q0 through q6 while `benchmarks/queries/clickbench/extended/` held q0 through q13. Seven of fourteen extended queries had no sqllogictest entry, so they were never executed by CI, only by whoever ran the benchmark by hand.
**The instruction for keeping them in sync points at a file that no longer exists.** `clickbench_extended.slt` says:
> If you change any of these queries, please change the corresponding query in `benchmarks/queries/clickbench/extended.sql` and update the README.
`benchmarks/queries/clickbench/extended.sql` was replaced by a directory of per-query files. The note also predates the `sql_benchmarks` tree entirely, so it names one of the three copies and misnames it.
**The project already spends PRs on the sync.** #24583 ("Sql benchmark sync updated queries"), #24778 and #24803 ("SQL benchmark configuration parity updates") exist to reconcile these trees by hand.
**A fourth place that enumerates them.** `datafusion/core/benches/sql_planner.rs` builds its ClickBench planning set from a hardcoded `(0..=7)` over the extended directory. There are 15 extended queries, so Q8 onward have never been planned by that benchmark.
### Describe the solution you'd like
Anything that removes the need for a human to remember. Roughly in order of preference:
1. **One source of truth.** Let the `.benchmark` file point at the `.sql` file instead of inlining the SQL, so `run` reads the query from `benchmarks/queries/...`. The `.benchmark` format already has directives that name external paths (`init`, `load`, `result`), so this fits the grain of the format. The sqllogictest copy is harder, since it carries expected output, but it could be generated from the same source.
2. **A test that fails on divergence.** If the copies must stay separate, add a test that parses all three and asserts the SQL matches after normalizing whitespace. That is a small test and it would have caught q6 on the day it landed. Deliberate divergences can be recorded in an explicit allow list, which also documents that they are deliberate — right now nothing distinguishes an intended difference from an accident.
3. At minimum, **fix the stale pointer** in `clickbench_extended.slt` so it names all the copies that actually exist.
For calibration on how much is duplicated: the ClickBench standard 43 agree exactly today, extended is 1 of 15 divergent, and imdb is 6 of the 79 I could match by name. The imdb differences look deliberate — an `at` alias renamed to `at_` — but nothing in the repository says so, which is the same problem from the other side.
### Describe alternatives you've considered
Leaving it and syncing by hand, which is the status quo and the reason for the three PRs above.
### Additional context
Found while adding a query in #25026, which had to be added to all three trees separately.
Contributor guide
Research direction
Start by comparing the ClickBench files under benchmarks/queries, benchmarks/sql_benchmarks, and datafusion/sqllogictest/test_files, then inspect the dfbench and benchmark_runner entry points plus datafusion/core/benches/sql_planner.rs. Check how existing benchmark-format directives and repository tests handle external files and SQL normalization. Done means the chosen consistency check or source-of-truth approach covers the named trees, catches divergence and missing queries, and removes or updates the stale synchronization instruction.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, sql
- Domain
- data-engineering, performance, testing
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 54/100