Skip to content

test: skip count distinct spill memory test under forced hash collisions - #24918

Merged
rluvaton merged 1 commit into
apache:mainfrom
rluvaton:fix-count-distinct-spill-test-under-forced-hash-collisions
Sep 3, 2026
Merged

test: skip count distinct spill memory test under forced hash collisions#24918
rluvaton merged 1 commit into
apache:mainfrom
rluvaton:fix-count-distinct-spill-test-under-forced-hash-collisions

Conversation

@rluvaton

@rluvaton rluvaton commented Sep 3, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Follow-up to #24888, which broke the cargo test hash collisions CI job on main.

Rationale for this change

The memory limit test added in #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

@github-actions github-actions Bot added the core Core DataFusion crate label Sep 3, 2026
@rluvaton
rluvaton force-pushed the fix-count-distinct-spill-test-under-forced-hash-collisions branch 2 times, most recently from 19a64a6 to fdd9d32 Compare September 3, 2026 07:29
With force_hash_collisions every key hashes alike, so the hash repartitioning
sends all groups to a single final stage whose table cannot fit the memory
limit however well memory is released. The limit is sized for the real
distribution across four final stages.
@rluvaton
rluvaton force-pushed the fix-count-distinct-spill-test-under-forced-hash-collisions branch from fdd9d32 to e484973 Compare September 3, 2026 07:30
@rluvaton
rluvaton marked this pull request as draft September 3, 2026 07:38
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.63%. Comparing base (a6f1dce) to head (e484973).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #24918   +/-   ##
=======================================
  Coverage   81.63%   81.63%           
=======================================
  Files        1123     1123           
  Lines      409963   409963           
  Branches   409963   409963           
=======================================
+ Hits       334685   334687    +2     
+ Misses      55605    55598    -7     
- Partials    19673    19678    +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rluvaton
rluvaton marked this pull request as ready for review September 3, 2026 09:28

@Weijun-H Weijun-H left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Non-blocking wording nit: filter_pushdown.rs adapts expectations under the feature rather than skipping the test, so consider removing that comparison from the PR description.

@rluvaton
rluvaton added this pull request to the merge queue Sep 3, 2026
Merged via the queue into apache:main with commit 4a93ade Sep 3, 2026
40 checks passed
@rluvaton
rluvaton deleted the fix-count-distinct-spill-test-under-forced-hash-collisions branch September 3, 2026 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants