fix: clear memory after emit all in count distinct - #24888
Merged
rluvaton merged 2 commits intoSep 3, 2026
Merged
Conversation
github-actions
Bot
added
core
rluvaton
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24888 +/- ##
=======================================
Coverage 81.63% 81.63%
=======================================
Files 1123 1123
Lines 409537 409541 +4
Branches 409537 409541 +4
=======================================
+ Hits 334308 334329 +21
+ Misses 55594 55572 -22
- Partials 19635 19640 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rluvaton
added a commit
to rluvaton/datafusion
that referenced
this pull request
Sep 3, 2026
…ons (apache#24918) ## Which issue does this PR close? Follow-up to apache#24888, which broke the `cargo test hash collisions` CI job on main. ## Rationale for this change The memory limit test added in apache#24888 fails when built with `force_hash_collisions`. Every key hashes to the same value there, so the hash repartition sends all 64 groups to one final stage. That single table needs 5.3 MB against the test's 4 MB pool, and it has nothing reserved yet, so there is nothing to spill. It fails no matter how well the accumulator releases memory, which is what the test is actually about. I tried a few ways to keep it running under the feature first: - **Bigger limit for the collision build.** Below 5.3 MB it dies on that one state batch; at 6 MB and up nothing spills, so the unfixed accumulator passes too and the test asserts nothing. Nothing in between. - **Single partition, no repartition at all.** Same wall. With 64 groups the whole distinct state lives in 64 rows, so total state and one batch are the same 5.3 MB. Also 82s instead of 0.18s. - **More groups, to spread the state over more batches.** With every key in one hash bucket, interning goes quadratic: 4096 groups did not finish in 400s. - **More rows (800k), to make total state exceed one batch.** Fails even with the fix. They all hit the same thing: under forced collisions the total state and a single batch are the same size, and the pool would have to sit above one and below the other. ## What changes are included in this PR? The test and its helpers move into a module gated on `not(feature = "force_hash_collisions")`. ## What is the testing strategy for this PR? `cargo test -p datafusion --features force_hash_collisions --test core_integration count_distinct_releases` runs 0 tests. Without the feature it still runs and passes. ## Are there any user-facing changes? No. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
3 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
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: