Skip to content

test (aggregation-fuzzer): exercise spilling under a bounded memory pool - #23979

Open
naman-modi wants to merge 3 commits into
apache:mainfrom
naman-modi:aggregation-fuzzer-spill-coverage
Open

test (aggregation-fuzzer): exercise spilling under a bounded memory pool#23979
naman-modi wants to merge 3 commits into
apache:mainfrom
naman-modi:aggregation-fuzzer-spill-coverage

Conversation

@naman-modi

@naman-modi naman-modi commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

The aggregation fuzzer always runs under the default unbounded memory pool, so it never exercises the aggregate spill paths (including the ordered-aggregation spilling from #23657).

What changes are included in this PR?

  • A memory-limit knob in SessionContextGenerator. A context is either unbounded (today's behavior) or bounded by a FairSpillPool that forces a spill.
  • The pool is sized from the aggregate's measured peak, not the dataset size, because aggregate memory scales with distinct groups, not rows. The peak is read from the aggregate's own peak_mem_used metric on the baseline plan, and bounded contexts use a range of fractions (2x, 0.5x, 0.4x, 0.334x) of it.
  • Bounded contexts use a single partition and a small batch size so the spill fits; a run still too tight returns ResourcesExhausted and is skipped and logged, not failed.
  • The unbounded baseline stays the correctness oracle, so every spilled result is checked against it.

Are these changes tested?

Yes, no regression, and added a new test_generated_context_spills asserts a spill actually happens.

Are there any user-facing changes?

No, it's Test-only.

@github-actions github-actions Bot added the core Core DataFusion crate label Jul 29, 2026
@naman-modi naman-modi changed the title test (aggregation-fuzzer): exercise spilling under a bounded memory pool feat test (aggregation-fuzzer): exercise spilling under a bounded memory pool Jul 29, 2026
@naman-modi

naman-modi commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Hey @2010YOUY01 this tries to close #23658, and while implementing the solution, I hit some design questions, which I wanted to discuss with you:

I added a bounded-pool knob to the fuzzer, but through the SQL/SessionContext path I can't produce the plan shape the ordered streams need: sorted input always plans as a single-partition Single-mode aggregate, and multi-partition repartitions away the ordering. So under a memory limit everything falls back to the legacy GroupedHashAggregateStream and spills there. I never reach OrderedPartial/OrderedFinal (mode=Partial/Final + sorted input), which is where your work on #23657 added spilling.

So, currently, this PR exercises the legacy hash-aggregate spill path broadly (accumulators, pressure, sorted/unsorted), but not the ordered streams. what i can think of:

  1. Take this framework coverage as-is (legacy path), and cover ordered spilling separately.
    and/or
  2. Add targeted hand-built AggregateExec tests (Partial/Final + ordered input + bounded pool), closer to your spilling_fuzz_in_memory_constrained_env.rs, to hit the ordered streams directly.

Which do you think about this?

@naman-modi naman-modi changed the title feat test (aggregation-fuzzer): exercise spilling under a bounded memory pool test (aggregation-fuzzer): exercise spilling under a bounded memory pool Jul 29, 2026
@codecov-commenter

codecov-commenter commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.85%. Comparing base (62cfc0c) to head (0e4d1ad).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #23979      +/-   ##
==========================================
- Coverage   80.85%   80.85%   -0.01%     
==========================================
  Files        1096     1096              
  Lines      373911   373911              
  Branches   373911   373911              
==========================================
- Hits       302329   302311      -18     
- Misses      53547    53559      +12     
- Partials    18035    18041       +6     

☔ 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.

@naman-modi

Copy link
Copy Markdown
Contributor Author

Hey @2010YOUY01 / @alamb, any thoughts on this approach for fuzzy memory testing?

@naman-modi
naman-modi force-pushed the aggregation-fuzzer-spill-coverage branch from c643f6d to 0e4d1ad Compare July 30, 2026 15:25
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.

2 participants