test: bound merge fan-in in ordered aggregate spill tests

[kumarUjjawal]

Which issue does this PR close?

Closes #25047.

Rationale for this change

ordered_aggregate_spill.slt can fail when spill-merge buffers leave too little memory for aggregate replay while another partition retains aggregate state in the shared greedy pool. Limit merge fan-in to two to reduce this contention while preserving the 600 KiB memory limit, two partitions, and existing query results.

What changes are included in this PR?

• Set and reset the SQL test's spill merge fan-in.
• Add controlled two-partition tests for grouped results, spilling, and reservation cleanup on completion, input error, and cancellation.
• Check spilled rows without depending on the formatted byte unit, and remove exact spill-count claims from comments.

What is the testing strategy for this PR?

Local validation:

• 320 SQL test executions (16 concurrent copies per run) and 50 repetitions of the focused tests.
• Full extended workspace tests, including all 518 SQL test files.
• Workspace Clippy with all targets and features, formatting, and submission checks.

Are there any user-facing changes?

No. This changes tests only; production execution and defaults are unchanged.

[kumarUjjawal]

@kosiew Please take a look when you get the time if this makes sense

[codecov-commenter]

Codecov Report

❌ Patch coverage is 89.94975% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.92%. Comparing base (9082d6b) to head (030b55e).
⚠️ Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
...ysical-plan/src/aggregates/ordered_final_stream.rs	89.94%	4 Missing and 16 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25252      +/-   ##
==========================================
+ Coverage   81.88%   81.92%   +0.04%     
==========================================
  Files        1133     1134       +1     
  Lines      424522   426074    +1552     
  Branches   424522   426074    +1552     
==========================================
+ Hits       347623   349066    +1443     
- Misses      56285    56312      +27     
- Partials    20614    20696      +82     

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

[kosiew]

@kumarUjjawal, thanks for working on this. The shared-pool regression coverage looks good overall. I just have one non-blocking suggestion to make the drop-during-merge test less dependent on polling behavior.

additional test that passed:

set -euo pipefail

runs="${RUNS:-100}"
test_threads="${SLT_TEST_THREADS:-$(getconf _NPROCESSORS_ONLN)}"

if ! [[ "$runs" =~ ^[1-9][0-9]*$ ]]; then
  echo "RUNS must be a positive integer; got: $runs" >&2
  exit 2
fi

if ! [[ "$test_threads" =~ ^[1-9][0-9]*$ ]]; then
  echo "SLT_TEST_THREADS must be a positive integer; got: $test_threads" >&2
  exit 2
fi

repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
cd "$repo_root"

for ((iteration = 1; iteration <= runs; iteration++)); do
  echo "=== ordered_aggregate_spill.slt: iteration $iteration/$runs (test threads: $test_threads) ==="
  if ! cargo test -p datafusion-sqllogictest --test sqllogictests -- \
    ordered_aggregate_spill.slt --test-threads "$test_threads"; then
    echo "ordered_aggregate_spill.slt failed on iteration $iteration/$runs" >&2
    exit 1
  fi
done

echo "ordered_aggregate_spill.slt passed $runs consecutive runs"

[kosiew]

I don't think we need to require the first replay poll to return Pending here. A buffered spill read could be immediately ready and return a batch while the stream is still correctly in MergingSpills, which would make this assertion unnecessarily sensitive to polling behavior.

Could we remove this assertion and keep the MergingSpills state assertion plus the subsequent drop and reservation check? Those directly cover the cleanup invariant we're interested in, without needing a test-only controlled merge input.

[kumarUjjawal]

done 030b55e

[kosiew]

@kumarUjjawal, thanks for the follow-up. The change addresses the concern about requiring the first replay poll to return Pending. An immediately-ready buffered spill read is now accepted, while the MergingSpills state assertion and the subsequent drop and reservation cleanup checks still cover the intended lifecycle behavior.

I don't see any further issues with this change. Looks good to me.

[kumarUjjawal]

Thank you @kosiew for your time