adriangb opened a new pull request, #25026: URL: https://github.com/apache/datafusion/pull/25026
## 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 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. 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: - Extended Q2 is the only query that puts a `COUNT(DISTINCT)` on a non-integer column at all. It groups by `BrowserCountry`, so it exercises the adapter at a low group cardinality, which is where the adapter is cheapest. - Standard Q8, Q10, Q11 and Q13 hold a distinct aggregate that stands alone, so `SingleDistinctToGroupBy` already rewrites them and they never reach the adapter. - Standard Q9 carries an `AVG`, which that rule has never accepted. - Standard Q22 is the one query that pairs a lone distinct aggregate with a non-distinct count, and it counts distinct `UserID`, an `Int64`, which has a specialized accumulator. So a lone `COUNT(DISTINCT <string>)` grouped by a high cardinality key, next to a non-distinct `COUNT(*)`, 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? One query file and its README entry. ```sql SELECT "SearchPhrase", COUNT(*) AS c, COUNT(DISTINCT "MobilePhoneModel") AS models FROM hits WHERE "SearchPhrase" <> '' GROUP BY "SearchPhrase" ORDER BY c DESC LIMIT 10; ``` Extended queries are discovered from the directory, so this needs no change to the runner: `RunOpt::run` walks `0..=usize::MAX` and stops at the first missing file. There is one pre-existing staleness this PR does not fix. `datafusion/core/benches/sql_planner.rs` builds 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? A benchmark query is not covered by tests. It was checked by running the suite: `dfbench clickbench --queries-path benchmarks/queries/clickbench/extended` picks the new query up as Query 14 and returns 10 rows. The plan shape was also confirmed directly, since the value of the query depends on it. On an equivalent local table the aggregate keeps `aggr=[[count(Int64(1)), count(DISTINCT ...)]]`, that is the `GroupsAccumulatorAdapter` path, which is what this query is meant to measure. `ci/scripts/doc_prettier_check.sh` passes on the README change. ## Are there any user-facing changes? No. This adds a benchmark query and documentation only. -- 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]
