Uh oh!
There was an error while loading. Please reload this page.
Fix final hash aggregate output regression by materializing once - #23182
Conversation
hhhizzz
commented
Jun 26, 2026
@2010YOUY01 Do you have time to take a look? |
2010YOUY01
left a comment
There was a problem hiding this comment.
LGTM, thank you! Just some minor suggestions.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bc2a13c to
170d4baCompareDandandan
commented
Jun 26, 2026
run benchmarks |
adriangbot
commented
Jun 26, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing codex/hashagg-terminal-output-materialize-v1-20260625 (170d4ba) to a00f749 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
adriangbot
commented
Jun 26, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing codex/hashagg-terminal-output-materialize-v1-20260625 (170d4ba) to a00f749 (merge-base) diff using: tpcds File an issue against this benchmark runner |
adriangbot
commented
Jun 26, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing codex/hashagg-terminal-output-materialize-v1-20260625 (170d4ba) to a00f749 (merge-base) diff using: tpch File an issue against this benchmark runner |
adriangbot
commented
Jun 26, 2026
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
adriangbot
commented
Jun 26, 2026
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
hhhizzz
commented
Jun 26, 2026
Looks like the bench here is using SF1, let me repro on my server to see if the big regression is only in SF10 or higher. |
2010YOUY01
commented
Jun 26, 2026
I remember 5% ish noise is likely for short-running queries |
hhhizzz
commented
Jun 26, 2026
Yes, my point is that q23 was only about 1.14x faster on SF1, but on my server, running SF10 produced around a 3x speedup. This makes me wonder whether we should require at least SF10+ runs before merging performance-related PRs. |
adriangbot
commented
Jun 26, 2026
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
hhhizzz
commented
Jun 26, 2026
I reran the benchmarks on my machine with SF1, and it looks like there's essentially no difference. It seems TPC-DS SF1 just isn't suitable for evaluating this kind of optimization. In hindsight, it also wouldn't have been able to reveal the impact of your previous changes. That said, I did notice a +42.81x improvement on ClickBench, so the optimization still appears to be effective.😁 |
hhhizzz
commented
Jun 26, 2026
@alamb Can you help take a look when you have time? |
alamb
commented
Jun 26, 2026
TPCH queries don't have many high cardinality groupings so I am not surprosed they don't show much of a difference (there is one with a subquery I think) |
alamb
commented
Jun 26, 2026
clickbench looks much better |
…l-output-materialize-v1-20260625
alamb
left a comment
There was a problem hiding this comment.
Thank you @hhhizzz and @2010YOUY01
| Outputting(AggregateHashTableBuffer), | ||
| /// Materialize all the output results, and then incrementally output in the `OutputtingMaterializedFinal` state. | ||
| /// | ||
| /// Note this is a temporary solution until the `GroupValues` issue is solved: |
There was a problem hiding this comment.
👍 it also keeps the existing behavior (of DataFusion 54)
| let output_schema = Arc::clone(&self.output_schema); | ||
| let batch_size = self.batch_size; | ||
| match &mut self.state { | ||
| match std::mem::replace(&mut self.state, AggregateHashTableState::Done) { |
There was a problem hiding this comment.
I found it confusing here that self.state was note replaced in this function (so it looks like self.state is lost)
i think what actually happens is that emit_next_materialized_batch sets it
I think it would be easier to understand what is happening if we inlined emit_next_materialized_batch here, but maybe just a comment would be good enough.
There was a problem hiding this comment.
I pushed a commit with a comment explanining this
Uh oh!
There was an error while loading. Please reload this page.
Thanks, the problem exist in partial path now actually, using |
hhhizzz
commented
Jun 30, 2026
Thanks, Opened #23249 to track this separately. I described it as an investigation for now because we still need benchmark/profile evidence for the partial path before deciding whether to materialize-once-and-slice there too, or only rename/comment |
Which issue does this PR close?
EmitTo::Firstmaintains unused GroupValues lookup state #23178.EmitTo::Firstmaintains unused GroupValues lookup state #23178Rationale for this change
PR #23055 changed final hash aggregate output to emit groups incrementally with
EmitTo::First(batch_size). For terminal final aggregate output, this can causethe group value state to be repeatedly compacted while output batches are being
produced. On TPC-DS q23 this showed up as a significant regression.
This PR implements the short-term approach discussed in #23178: materialize the
final aggregate output once, then return slices of that materialized
RecordBatchaccording tobatch_size.This avoids changing the
GroupValuesAPI while preserving bounded downstreambatch sizes.
What changes are included in this PR?
OutputtingMaterializedhash aggregate state.MaterializedOutput, a small wrapper around aRecordBatchplus outputoffset.
output state transitions.
Performance
TPC-DS SF10 full 99 queries, 10 rounds:
0.8570510.976652(~2.4% faster)0.313770(~218.7% faster), faster in10/10roundsRegressions over 5% were observed in 10 queries. Most have small absolute
deltas, but the largest slowdowns were:
1.055907, +170.996 ms1.060436, +98.544 ms1.050135, +37.858 ms1.061124, +11.848 ms1.052392, +9.386 ms1.063655, +6.995 ms1.071688, +6.515 ms1.109819, +5.362 ms1.058356, +5.072 ms1.057686, +0.815 msOverall, this recovers the q23 regression strongly and improves full-query
geomean, but q39 and q67 are worth calling out as residual per-query slowdowns.
Testing
cargo fmt --all -- --checkcargo test -p datafusion-physical-plan materializcargo test -p datafusion-physical-plan aggregates::