adriangb opened a new issue, #25031:
URL: https://github.com/apache/datafusion/issues/25031
### 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.
--
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]
