mohitgurav20 commented on PR #26000:
URL: https://github.com/apache/datafusion/pull/26000#issuecomment-6562970790
@mkleen I've narrowed this PR down strictly to the
`datafusion.optimizer.single_distinct_aggregation_prefer_memory` config option
(defaulting to `false`, matching `main`). All previous automatic heuristic
gating and plan regressions have been reverted. Aggregates with
`DistinctHandling::Unsupported` (like `approx_median`) are still rewritten
regardless of the flag to maintain correctness.
Here are the benchmark results on the 4,000,000 row table across the 15
queries you shared, running with 8 target partitions and using
`PeakRecordingPool` to capture exact peak memory reservations:
| Category | Query | `prefer_memory=false` (default) Time |
`prefer_memory=false` Peak Mem | `prefer_memory=true` Time |
`prefer_memory=true` Peak Mem | Peak Mem Delta | Speed Ratio |
|---|---|---|---|---|---|---|---|
| GROUP BY, 2000 groups | `count(DISTINCT int)` | 148.8 ms | 186.96 MB |
214.9 ms | 169.32 MB | -9.4% | 1.44x |
| GROUP BY, 2000 groups | `sum(DISTINCT int)` | 169.3 ms | 193.96 MB | 179.9
ms | 76.66 MB | -60.5% | 1.06x |
| GROUP BY, 2000 groups | `count(DISTINCT str)` | 157.3 ms | 226.28 MB |
852.9 ms | 327.48 MB | +44.7% | 5.42x |
| GROUP BY, 2000 groups | `avg(DISTINCT float)` | 155.4 ms | 193.81 MB |
219.1 ms | 76.66 MB | -60.4% | 1.41x |
| GROUP BY, 10 groups | `count(DISTINCT int)` | 171.7 ms | 194.15 MB | 242.1
ms | 158.09 MB | -18.6% | 1.41x |
| GROUP BY, 10 groups | `sum(DISTINCT int)` | 130.6 ms | 193.71 MB | 117.4
ms | 46.17 MB | -76.2% | 0.90x |
| GROUP BY, 10 groups | `count(DISTINCT str)` | 147.1 ms | 226.28 MB | 322.4
ms | 247.07 MB | +9.2% | 2.19x |
| Companion aggregates | `sum(y), sum(DISTINCT x)` | 208.9 ms | 226.28 MB |
183.4 ms | 76.79 MB | -66.1% | 0.88x |
| Companion aggregates | `count(*), count(DISTINCT x)` | 178.8 ms | 169.44
MB | 189.8 ms | 169.44 MB | 0.0% | 1.06x |
| Companion aggregates | `min(y), max(y), count(DISTINCT s)` | 200.0 ms |
266.97 MB | 848.8 ms | 327.73 MB | +22.8% | 4.24x |
| No GROUP BY | `count(DISTINCT int)` | 73.3 ms | 47.71 MB | 117.2 ms |
67.50 MB | +41.5% | 1.60x |
| No GROUP BY | `sum(DISTINCT int)` | 66.7 ms | 48.90 MB | 112.0 ms | 72.00
MB | +47.2% | 1.68x |
| No GROUP BY | `count(DISTINCT str)` | 141.1 ms | 95.87 MB | 475.8 ms |
328.50 MB | +242.7% | 3.37x |
| No GROUP BY | `count(DISTINCT smallint)` | 5.8 ms | 0.57 MB | 2.3 ms |
0.07 MB | -87.7% | 0.40x |
| No GROUP BY | `count(*), count(DISTINCT x)` | 110.4 ms | 72.00 MB | 106.0
ms | 72.00 MB | 0.0% | 0.96x |
### Summary of the Trade-offs
1. **Where `prefer_memory = true` wins (60% to 76% memory savings):**
- For `GROUP BY` queries with numeric aggregates (`sum(DISTINCT int)`,
`avg(DISTINCT float)`, `sum(y), sum(DISTINCT x)`), skipping the two-phase
rewrite drops peak memory by **60% to 76%** (~194 MB down to ~46–76 MB).
- Because native numeric accumulators have low per-group overhead,
runtime remains roughly at parity or even slightly faster (0.88x–1.06x) since
we avoid the repartitioning and intermediate grouping table shuffle.
2. **Where the two-phase rewrite is better (why default `false` makes
sense):**
- **Strings**: For variable-length data (`count(DISTINCT str)`), skipping
the two-phase rewrite forces all distinct strings into the final stage
accumulators without partial reduction, increasing peak memory (+9% to +45%)
and slowing down queries by 2.2x–5.4x.
- **No GROUP BY**: For queries without grouping keys, the rewrite is the
only way DataFusion parallelizes distinct work across multiple threads.
Suppressing it serializes accumulation into a single thread, resulting in a
1.6x–3.4x slowdown on high-cardinality data.
- **Low cardinality (`count(DISTINCT smallint)`):** On small distinct
domains (100 distinct values), single-phase is both **88% lighter** (0.07 MB vs
0.57 MB) and **2.5x faster** (2.3 ms vs 5.8 ms) because the 2-phase overhead is
skipped completely.
Overall, the numbers support keeping `false` as the default on `main` to
preserve general multi-threaded throughput, while giving memory-constrained
workloads an opt-in knob to cut peak memory by up to 76%.
Also fixed a rustdoc intra-doc link issue on `DistinctHandling` that caused
the earlier cargo doc check failure on the fork.
--
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]
