test: add aggregate fuzz tests for all shapes - #24881
Open
rluvaton wants to merge 18 commits into
Open
Conversation
github-actions
Bot
added
core
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24881 +/- ##
==========================================
- Coverage 81.86% 81.86% -0.01%
==========================================
Files 1130 1130
Lines 419103 419103
Branches 419103 419103
==========================================
- Hits 343094 343079 -15
- Misses 55761 55773 +12
- Partials 20248 20251 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tschwarzinger
pushed a commit
to tschwarzinger/datafusion
that referenced
this pull request
Sep 3, 2026
## Which issue does this PR close? N/A ## Rationale for this change after emit all, the memory is still being held in count distinct causing OOM issues ## What changes are included in this PR? release memory in emit all and added tests ## What is the testing strategy for this PR? integration test ## Are there any user-facing changes? no ---- Founded while running: - apache#24881
…a nested group key When GroupedHashAggregateStream spills and switches to merging the sorted spill files it relies on GroupOrderingFull, which requires group ids to be assigned in first-seen order. It recreated its group values collector to guarantee that only when there was more than one group column, assuming a single column always uses a sequential single-column collector. A single nested column (Struct, Map) has no specialized single-column collector and is handled by GroupValuesColumn through a row-backed column, whose vectorized interning assigns ids out of input order. The ordering then emitted groups that were still in progress and the next batch reopened them as new groups, so the same key came out more than once with its aggregates split between the rows. Always recreate the collector for the merge phase.
rluvaton
force-pushed
the
fuzz-test-aggregation-all-cases
branch
from
September 6, 2026 08:46
3613439 to
1bce718
Compare
# Conflicts: # datafusion/core/tests/memory_limit/mod.rs
rluvaton
commented
Sep 8, 2026
Member
Author
There was a problem hiding this comment.
rluvaton
commented
Sep 8, 2026
Member
Author
There was a problem hiding this comment.
rluvaton
commented
Sep 8, 2026
Member
Author
There was a problem hiding this comment.
rluvaton
commented
Sep 8, 2026
Member
Author
There was a problem hiding this comment.
… into fuzz-test-aggregation-all-cases
rluvaton
marked this pull request as ready for review
September 8, 2026 14:13
pull Bot
pushed a commit
to TCeason/arrow-datafusion
that referenced
this pull request
Sep 9, 2026
…ble) (apache#25075) ## Which issue does this PR close? N/A ## Rationale for this change While adding fuzz tests for aggregate streams I encounter OOMs that partial reduce takes up all the memory and leave nothing to partial Same as: https://github.com/apache/datafusion/blob/a5c809f98dedcf4b22f5d317ea8efe0720834925/datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs#L544-L546 ## What changes are included in this PR? mark reduce hash aggregate stream as spillable ## What is the testing strategy for this PR? This was found as part of: - apache#24881 ## Are there any user-facing changes? No
The per-case timeout of 60s fired on the four-core CI runner for the order-preserving repartition shape at very high cardinality. The cases take about two seconds alone locally and pass under the full concurrent matrix, so this is CI oversubscription rather than a hang.
rluvaton
added a commit
to rluvaton/datafusion
that referenced
this pull request
Sep 10, 2026
…a fallback group key (type not having dedicated impl) (apache#24889) Found by the new aggregate fuzz tests: - apache#24881 Claude: ## Which issue does this PR close? N/A ## Rationale for this change `GROUP BY` on a single nested column (`Struct`, `Map`) returns duplicate groups when the legacy `GroupedHashAggregateStream` spills: the same key comes out as several rows, with the aggregate values split between them. It needs the legacy stream (`datafusion.execution.enable_migration_aggregate = false`), a single nested group key, and enough memory pressure to spill in a `Final` or `Single` stage. Results are silently wrong rather than an error. The migrated streams are not affected: after spilling they hand the merged input to `OrderedFinalAggregateStream`, which builds a fresh group values collector with `GroupOrdering::Full`. After spilling, the legacy stream re-aggregates the merged spill files with `GroupOrderingFull`, which requires group ids in first-seen order along the sorted input. The stream recreates its group values collector for that phase to guarantee the order, but only when there is more than one group column, assuming a single column always uses a sequential single-column collector. A single nested column has no specialized single-column collector and is served by `GroupValuesColumn` through a row-backed column, whose vectorized interning assigns new ids out of input order under hash collisions. In a merged batch of 28 sorted rows the ids came out as 0 to 4, then 9 to 13, then 5 to 8. `GroupOrderingFull` then treated a group that was still arriving as complete and emitted it, and the next batch reopened it as a new group. ## What changes are included in this PR? `GroupedHashAggregateStream` now always recreates the group values collector when it switches to merging spill files, instead of only for multi-column keys. ## What is the testing strategy for this PR? New integration test `memory_limit::nested_key_spill_keeps_groups_unique`: a 200k-row table grouped by a struct of a list and an integer, with null and empty lists, null numbers and null structs mixed in, aggregated with six aggregates including `count(distinct)` under an 8 MB `FairSpillPool` with a 64-row batch size, compared against the same query with unlimited memory. It runs both the legacy stream and the migrated streams. Without the fix the legacy run fails deterministically with 72 rows instead of 71, keys split into two rows whose counts add up to the reference. The migrated run passes with and without the fix and is kept as a regression guard. ## Are there any user-facing changes? No API changes. Queries that hit this path now return correct results. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
The per-case timeout of 60s fired on the four-core CI runner for the order-preserving repartition shape at very high cardinality. The cases take about two seconds alone locally and pass under the full concurrent matrix, so this is CI oversubscription rather than a hang. Bound the in-flight cases by the available parallelism instead of a fixed 16, and raise the timeout so only a real hang fires it.
Adds a Distinct query (GROUP BY k1, k2 with no aggregates) with the single, partial/final and partial/reduce/final chains.
The shapes were a hand-picked list of (chain, query) pairs. Split them into independent axes: an operator chain, the group keys (one per GroupValues implementation) and the aggregate expressions, and generate every combination that can be planned. No-key queries skip chains that hash or sort, TopK chains take one primitive or string key with a single max or no aggregates, and max alone is dropped elsewhere as a subset of the full list.
# Conflicts: # datafusion/physical-plan/src/aggregates/mod.rs
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
2 participants
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?
N/A
Rationale for this change
There are no tests that verify that we produce the same output in all cases of the extracted aggregate streams
and
we are still doing refactor thereso this increase the coverage.This already found at least 4 bugs that were fixed or in progress
What changes are included in this PR?
added fuzz tests on all shapes of plan and cardinality, group by columns, mem, etc
I let claude create all the tests
What is the testing strategy for this PR?
added
Are there any user-facing changes?
no