Repository navigation
fix: Only apply SingleDistinctToGroupBy rewrite when it pays (closes #25932) - #26000
Open
mohitgurav20 wants to merge 33 commits into
Open
mohitgurav20 wants to merge 33 commits into
mohitgurav20 wants to merge 33 commits into
Conversation
mohitgurav20
force-pushed
the
fix/single-distinct-groupby-memory-usage
branch
from
October 4, 2026 16:36
527a544 to
9e8dfe6
Compare
mkleen
suggested changes
Oct 8, 2026
Contributor
There was a problem hiding this comment.
@mohitgurav20 Thanks for working on this.
Can you make sure all sql-logic tests pass, or correct them if invalid.
You can run them locally with:
cargo test --profile=ci --test sqllogictests
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26000 +/- ##
========================================
Coverage 82.93% 82.94%
========================================
Files 1154 1154
Lines 458002 458300 +298
Branches 458002 458300 +298
========================================
+ Hits 379856 380130 +274
+ Misses 55123 55104 -19
- Partials 23023 23066 +43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mohitgurav20
force-pushed
the
fix/single-distinct-groupby-memory-usage
branch
from
October 10, 2026 19:06
63e9873 to
0c76607
Compare
Contributor
Author
|
@mkleen sir i have completed the work and addressed ur suggestion pls feel free to review and would like to work if any changes needed. |
Contributor
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/single-distinct-groupby-memory-usage (0923424) to 334f293 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/single-distinct-groupby-memory-usage (0923424) to 334f293 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/single-distinct-groupby-memory-usage (0923424) to 334f293 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix/single-distinct-groupby-memory-usage (0923424) to 334f293 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix/single-distinct-groupby-memory-usage (0923424) to 334f293 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix/single-distinct-groupby-memory-usage (0923424) to 334f293 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
|
@mohitgurav20 There are serious performance regressions in the clickhouse benchmark. See #26000 (comment) for more details. |
Contributor
Author
|
Thanks for running the benchmark and flagging this @mkleen. I analyzed the benchmark run in detail: Because Without the two-phase rewrite to This confirms that the two-phase rewrite is a critical parallelization optimization for lone distinct queries, even when a specialized To address this cleanly: Does that direction sound good to you? If so, I'll update the rule accordingly so we preserve ClickBench performance. |
…pache#25932) SingleDistinctToGroupBy converts single DISTINCT aggregate expressions into a two-phase GROUP BY plan. This enables parallel deduplication via RepartitionExec hash partitioning, which is critical for lone-distinct queries like COUNT(DISTINCT x). However, when companion non-distinct aggregates are present alongside the distinct aggregate (e.g. SELECT sum(y), count(DISTINCT x)), the companion aggregates move to the inner group-by at a finer grain. When the distinct aggregate already has native GroupsAccumulator support, this inner grouping adds unnecessary memory and CPU overhead without benefiting from adapter offloading. This change: 1. Gates SingleDistinctToGroupBy with rewrite_pays() only when companion non-distinct aggregates are present. 2. Preserves the two-phase rewrite for pure lone-distinct queries so ClickBench and similar workloads maintain parallel deduplication without regression. 3. Consistently handles distinct aggregates with unsupported distinct handling. 4. Updates optimizer plan snapshots and sqllogictests.
mohitgurav20
force-pushed
the
fix/single-distinct-groupby-memory-usage
branch
from
October 17, 2026 07:46
287b932 to
ac76b0d
Compare
Contributor
Author
|
Hi @mkleen, just following up on this when you get a chance. I've rebased cleanly onto latest Could you trigger a benchmark run whenever convenient? Thanks! |
Contributor
|
run benchmarks |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/single-distinct-groupby-memory-usage (bbf4ccb) to 484183d (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/single-distinct-groupby-memory-usage (bbf4ccb) to 484183d (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix/single-distinct-groupby-memory-usage (bbf4ccb) to 484183d (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix/single-distinct-groupby-memory-usage (bbf4ccb) to 484183d (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
Contributor
|
@mohitgurav20 Thanks! Looks good at first glance. I’ll go ahead and start the review. |
…roupby-memory-usage
…roupby-memory-usage
Contributor
Author
|
@mkleen Just a quick ping on this — resolved the previous conflicts and re-synced with main, and CI is green across the board. Ready for review whenever you have a moment. |
mkleen
suggested changes
Nov 1, 2026
Contributor
Author
|
@mkleen sure sir ill be back back with the requirements |
Contributor
|
My suggestion would be to narrow this PR down to just the config option, |
Contributor
|
So the new setting should lower the peak memory of single-DISTINCT aggregates, at the cost of runtime. We want to measure how much memory it actually saves and how much speed it costs. |
Contributor
|
Here's a set of benchmark queries for the flag. They use the table from the #25932, CREATE TABLE t AS
SELECT v % 2000 AS g, -- 2,000 groups
v % 10 AS g_few, -- 10 groups
(v * 48271) % 999983 AS x, -- ~1M distinct ints
CAST((v * 48271) % 999983 AS VARCHAR) AS s, -- same values as strings
CAST((v * 48271) % 999983 AS DOUBLE) AS f, -- same values as floats
CAST(v % 100 AS SMALLINT) AS si, -- small-int, 100 distinct
v % 1000 AS y
FROM (SELECT unnest(generate_series(0, 3999999)) AS v);
SELECT g, count(DISTINCT x) FROM t GROUP BY g;
SELECT g, sum(DISTINCT x) FROM t GROUP BY g;
SELECT g, count(DISTINCT s) FROM t GROUP BY g;
SELECT g, avg(DISTINCT f) FROM t GROUP BY g;
SELECT g_few, count(DISTINCT x) FROM t GROUP BY g_few;
SELECT g_few, sum(DISTINCT x) FROM t GROUP BY g_few;
SELECT g_few, count(DISTINCT s) FROM t GROUP BY g_few;
SELECT g, sum(y), sum(DISTINCT x) FROM t GROUP BY g;
SELECT g, count(*), count(DISTINCT x) FROM t GROUP BY g;
SELECT g, min(y), max(y), count(DISTINCT s) FROM t GROUP BY g;
SELECT count(DISTINCT x) FROM t;
SELECT sum(DISTINCT x) FROM t;
SELECT count(DISTINCT s) FROM t;
SELECT count(DISTINCT si) FROM t; -- small-int case
SELECT count(*), count(DISTINCT x) FROM t; |
…regation_prefer_memory
Contributor
Author
|
@mkleen I've updated the PR following your suggestion. It now only introduces the configuration flag I ran the 15 queries on a 4M row dataset using Key Takeaways
Click to view full benchmark numbers (15 queries, 4M rows)
Overall, having the flag default to Let me know if this looks good to you! |
Contributor
|
Great. So in which scenarios would switching of the rewrite makes sense? When does it significantly reduce memory consumption with only a minor impact on performance? Ok, this is the part i would focus on. |
Contributor
Author
|
From checking the execution plans and benchmark numbers, turning off the rewrite (
On the other hand, the benchmarks also clearly showed why the rewrite should stay enabled by default:
So the flag works best when users have memory-heavy |
Contributor
|
Can we implement the flag in a way, that the memory comsumptions gets better without having any real bad performance impacts and avoid the cases where it gets worse ? |
Contributor
Author
|
Yes, we can definitely do that. From the benchmarks, the only cases that got worse were:
So under the flag, we can only skip the rewrite if:
That way, strings and queries without I can push this change if that sounds good to you! |
…trings and un-grouped distincts
…roupby-memory-usage
…roupby-memory-usage # Conflicts: # docs/source/user-guide/configs.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #25932.
Rationale for this change
In queries with a single distinct aggregation,
SingleDistinctToGroupByrewrites the plan into a two-phaseGROUP BY. As shown in benchmarks on #25932, this rewrite provides a ~2.5x speedup by enabling partition fan-out, but increases peak intermediate memory (~538–599 MiB vs ~306 MiB).At the logical planning stage, statistics are not available to cost this trade-off automatically. This PR introduces an optimizer configuration knob so users can choose between maximum throughput (default) and lower peak memory when the system is under load.
What changes are included in this PR?
datafusion.optimizer.single_distinct_aggregation_prefer_memory: bool(defaultfalse).false): Preserves the existing behavior for lone distinct queries—rewriting into two-phaseGROUP BYto maintain throughput and partition parallelism.true): Gated byrewrite_pays; skips the two-phase rewrite if the distinct aggregate function already supportsGroupsAccumulator, reducing intermediate memory usage.DistinctHandling::Unsupported(such asapprox_median) still rewrite unconditionally to guarantee correct execution.single_distinct_to_groupby.rs, end-to-endEXPLAINand execution coverage insingle_distinct_to_groupby.slt, updatedinformation_schema.slt, and updated config documentation.Are these changes tested?
Yes:
prefer_memory = falseandprefer_memory = true) and unsupported distinct fallbacks.EXPLAIN) and query execution.cargo fmt,cargo clippy -D warnings, and optimizer test suite).Are there any user-facing changes?
Yes:
datafusion.optimizer.single_distinct_aggregation_prefer_memory.