bench: add a ClickBench extended query for a grouped COUNT(DISTINCT) over a string - #25026
Conversation
…string `COUNT(DISTINCT)` has a specialized `GroupsAccumulator` for the integer types and for no other type. Every other type falls back to `GroupsAccumulatorAdapter`, which holds one boxed `Accumulator`, and therefore one hash table, for each group, so the cost of that fallback scales with the group cardinality. Extended Q2 is the only query in either suite that puts a `COUNT(DISTINCT)` on a non-integer column, and it groups by `BrowserCountry`. Nothing exercises the adapter at a high group cardinality, and nothing covers a lone `COUNT(DISTINCT <string>)` next to a non-distinct `COUNT(*)`: standard Q8, Q10, Q11 and Q13 hold a distinct aggregate that stands alone, Q9 carries an `AVG`, and Q22, the one query that does pair a lone distinct aggregate with a non-distinct count, counts distinct `UserID`, which is an `Int64`. Extended Q14 is that query. It groups by `SearchPhrase` and counts distinct `MobilePhoneModel` beside a `COUNT(*)`. Extended queries are discovered from the directory, so this needs no change to the runner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25026 +/- ##
==========================================
+ Coverage 81.69% 81.85% +0.15%
==========================================
Files 1127 1127
Lines 415471 418811 +3340
Branches 415471 418811 +3340
==========================================
+ Hits 339424 342815 +3391
+ Misses 56108 56027 -81
- Partials 19939 19969 +30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @adriangb
I was thinking since we are also using the clickbench_extended should we add this to there as well?
The file only mirrored q0-q6; q7-q13 were added to benchmarks/queries/clickbench/extended/ without a corresponding sqllogictest entry, and this PR's new q14 would have widened that gap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Good call. I noticed multiple queries were missing, so I added them all. Since this increased the scope I'll let you re-approve before merging. |
Thanks, Ping me when you add them. |
|
Sorry forgot to push! The change is now in 23a2181 |
The extended ClickBench queries exist in two runners. `benchmarks/queries/ clickbench/extended/` feeds `dfbench clickbench --queries-path`, and `benchmarks/sql_benchmarks/clickbench_extended/benchmarks/` feeds `benchmark_runner clickbench_extended`. A query added to only one of them is invisible to the other. Add the `.benchmark` file, structurally identical to q13 apart from the query itself. Suite files are discovered from the directory, so nothing else changes: `benchmark_runner clickbench_extended --list` now reports 15 queries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Which issue does this PR close?
No existing issue. This is benchmark coverage carved out of #24859 so that it can land first: a benchmark query added in the same PR that needs it cannot appear in an A/B run, because the bot compares against the merge base and the merge base does not have the query.
Rationale for this change
COUNT(DISTINCT)has a specializedGroupsAccumulatorfor the integer types and for no other type. Every other type falls back toGroupsAccumulatorAdapter, which holds one boxedAccumulator, and therefore one hash table, for each group. The cost of that fallback is per group, so the group cardinality is what decides how much it costs.Nothing in either suite measures that:
COUNT(DISTINCT)on a non-integer column at all. It groups byBrowserCountry, so it exercises the adapter at a low group cardinality, which is where the adapter is cheapest.SingleDistinctToGroupByalready rewrites them and they never reach the adapter.AVG, which that rule has never accepted.UserID, anInt64, which has a specialized accumulator.So a lone
COUNT(DISTINCT <string>)grouped by a high cardinality key, next to a non-distinctCOUNT(*), is uncovered. That is an ordinary analytics shape, it is the shape #24857 changed the memory profile of, and it is the shape #24859 proposes to rewrite. Neither of those could show its effect on any benchmark in this repository.What changes are included in this PR?
The query:
The extended ClickBench queries live in three places, and a query added to only one of them is invisible to the others. This PR adds it to all three:
benchmarks/queries/clickbench/extended/q14.sql, which feedsdfbench clickbench --queries-path. Queries are discovered from the directory, so the runner needs no change.benchmarks/sql_benchmarks/clickbench_extended/benchmarks/q14.benchmark, which feedsbenchmark_runner clickbench_extended. Structurally identical toq13.benchmarkapart from the query. Suite files are also discovered from the directory;benchmark_runner clickbench_extended --listnow reports 15 queries.datafusion/sqllogictest/test_files/clickbench_extended.slt, which runs the extended queries against the committed ten rowclickbench_hits_10.parquetfixture.That last file had only ever mirrored q0 through q6. q7 through q13 were added to the queries directory without a matching sqllogictest entry, and q14 would have widened that gap, so this backfills q7 through q14 together. That is why the diff is larger than one query.
Having to add one query in three places is itself the problem, and the copies have already drifted: extended q6 carries a cast in its
sql_benchmarkscopy that the other two do not have. I filed #25031 for that; it is out of scope here.There is one pre-existing staleness this PR does not fix.
datafusion/core/benches/sql_planner.rsbuilds its ClickBench planning set from a hardcoded(0..=7)over the extended directory, so extended Q8 through Q13 were already outside it before this PR and Q14 joins them. That is a separate cleanup.What is the testing strategy for this PR?
The sqllogictest entries are the test.
clickbench_extended.sltruns every extended query against the committed ten row fixture and asserts its output, so q7 through q14 are now executed on every CI run rather than only by whoever runs the benchmark suite by hand.cargo test -p datafusion-sqllogictest --test sqllogictests -- clickbenchpasses, 2 of 2 files.cargo test -p datafusion-benchmarkspasses, 192 tests, andbenchmark_runner clickbench_extended --listreports the suite at 15 queries, confirming the new.benchmarkfile parses and is discovered.I do not have a
hits.parquetto hand, so I have not run the query against the full dataset. The plan shape, which is the whole value of the query, was checked separately: on an equivalent local table with the same filter, grouping,COUNT(*)and stringCOUNT(DISTINCT), the aggregate keepsaggr=[[count(Int64(1)), count(DISTINCT ...)]], which is theGroupsAccumulatorAdapterpath this query exists to measure, and it is not rewritten away.What I have not established is the effect size on the real dataset, since that depends on how many distinct
SearchPhrasevalues survive the filter. If a reviewer with the data runs it, that number is worth having on this PR.ci/scripts/doc_prettier_check.shpasses on the README change.Are there any user-facing changes?
No. This adds a benchmark query and documentation only.