You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kumarUjjawal (via GitHub)" <gi...@apache.org> on 2026/09/17 17:57:54 UTC
[PR] test: bound merge fan-in in the final hash aggregate spill test [datafusion]
kumarUjjawal opened a new pull request, #25424:
URL: https://github.com/apache/datafusion/pull/25424
## Which issue does this PR close?
- Closes #25423.
## Rationale for this change
Case G of `aggregate_memory_spill.slt` runs four final partitions in a 1 MiB greedy pool. A partition that replays its spills cannot spill again. Its replay table fails to grow when its own merge buffers and the state of the other partitions fill the pool. CI hit this on two merge-queue runs (see the issue).
## What changes are included in this PR?
- `aggregate_memory_spill.slt`: set `datafusion.runtime.max_spill_merge_fan_in = 2` for Case G and reset it at the end, as #25252 did for `ordered_aggregate_spill.slt`. The memory limit stays at 1 MiB. Cases A to F are unchanged.
- `FinalHashAggregateStream::new_with_input` builds the stream over a given input, like `OrderedFinalAggregateStream::new_with_input`. `new` calls it.
- Tests in `hash_stream.rs` spill and replay partition 0 while partitions 1 to 3 keep their state in a shared 1 MiB greedy pool with fan-in 2. Channel inputs return `Pending` after each batch, so the interleaving does not depend on task scheduling. The tests check the grouped results against a reference, the spill metrics, and that the pool returns to the held size after completion, after a drop during replay, and after an input error, and to zero after all streams drop.
## What is the testing strategy for this PR?
Local runs on this branch:
- The SLT file 480 times as 16 concurrent copies, each in its own working directory. Before the change: 12 failures, all in Case G with the `ResourcesExhausted` error from the issue. After the change: 0 failures.
- The new tests 50 times, and all `datafusion-physical-plan` unit tests.
- `cargo test -p datafusion-sqllogictest --test sqllogictests` for all files.
- `cargo fmt --all -- --check` and `cargo clippy --all-targets --all-features -- -D warnings`.
## Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: bound merge fan-in in the final hash aggregate spill test [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #25424:
URL: https://github.com/apache/datafusion/pull/25424#issuecomment-5750304069
Let's see if the test fixes are working
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: bound merge fan-in in the final hash aggregate spill test [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #25424:
URL: https://github.com/apache/datafusion/pull/25424#discussion_r4046043832
##########
datafusion/physical-plan/src/aggregates/hash_stream.rs:
##########
@@ -1607,4 +1623,247 @@ mod tests {
Ok(())
}
+
+ #[derive(Clone, Copy)]
+ enum Finish {
+ Collect,
+ DropDuringReplay,
+ InputError,
+ }
+
+ #[tokio::test]
+ async fn final_hash_spill_replay_with_other_partitions_holding_state() -> Result<()> {
+ for input_batches in [40, 55, 70] {
+ run_shared_pool_case(input_batches, 1024 * 1024, Finish::Collect).await?;
+ }
+ // The same input also produces the reference results without spilling.
+ run_shared_pool_case(70, 10 * 1024 * 1024, Finish::Collect).await
+ }
+
+ #[tokio::test]
+ async fn final_hash_spill_replay_releases_memory_on_drop() -> Result<()> {
+ run_shared_pool_case(55, 1024 * 1024, Finish::DropDuringReplay).await
+ }
+
+ #[tokio::test]
+ async fn final_hash_spill_releases_memory_on_input_error() -> Result<()> {
+ run_shared_pool_case(55, 1024 * 1024, Finish::InputError).await
+ }
+
+ /// Partition 0 spills and replays while partitions 1..3 keep their
+ /// aggregate state in the same greedy pool.
+ async fn run_shared_pool_case(
+ input_batches: i64,
+ limit: usize,
+ finish: Finish,
+ ) -> Result<()> {
+ const PARTITIONS: usize = 4;
+ const HELD_BATCHES: i64 = 20;
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("a", DataType::Int64, false),
+ Field::new("b", DataType::Int64, false),
+ Field::new("v", DataType::Int64, false),
+ Field::new("s", DataType::Utf8View, false),
+ ]));
+ let groups = PhysicalGroupBy::new_single(vec![
+ (col("a", &schema)?, "a".into()),
+ (col("b", &schema)?, "b".into()),
+ ]);
+ let expressions = vec![
+ Arc::new(
+ AggregateExprBuilder::new(sum_udaf(), vec![col("v", &schema)?])
+ .schema(Arc::clone(&schema))
+ .alias("sum")
+ .build()?,
+ ),
+ Arc::new(
+ AggregateExprBuilder::new(min_udaf(), vec![col("s", &schema)?])
+ .schema(Arc::clone(&schema))
+ .alias("min")
+ .build()?,
+ ),
+ ];
+ let empty = TestMemoryExec::try_new_exec(&[vec![]], Arc::clone(&schema), None)?;
+ let partial = AggregateExec::try_new(
+ AggregateMode::Partial,
+ groups.clone(),
+ expressions.clone(),
+ vec![None; 2],
+ empty,
+ Arc::clone(&schema),
+ )?;
+ let partial_schema = partial.schema();
+ let input = TestMemoryExec::try_new_exec(
+ &vec![vec![]; PARTITIONS],
+ Arc::clone(&partial_schema),
+ None,
+ )?;
+ let aggregate = AggregateExec::try_new(
+ AggregateMode::FinalPartitioned,
+ groups.as_final(),
+ expressions,
+ vec![None; 2],
+ input,
+ schema,
+ )?;
+ assert_eq!(aggregate.input_order_mode(), &InputOrderMode::Linear);
+
+ let pool: Arc<dyn MemoryPool> = Arc::new(GreedyMemoryPool::new(limit));
+ let context = Arc::new(
+ TaskContext::default()
+ .with_session_config(SessionConfig::new().with_batch_size(128))
+ .with_runtime(
+ RuntimeEnvBuilder::new()
+ .with_max_spill_merge_fan_in(2)
Review Comment:
Optional clarity suggestion: removing `.with_max_spill_merge_fan_in(2)` here, which otherwise defaults to `0` for unbounded fan-in, still lets all three tests pass. These tests seem focused on shared-pool replay lifecycle and correctness, while Case G in `aggregate_memory_spill.slt` already owns the SQL-level regression coverage for #25423.
Could we either mention that distinction in `run_shared_pool_case`'s doc comment, or remove the fan-in setting if it does not have another intended role? I would avoid turning this unit test into a duplicate stress reproduction since the SLT seems like the better boundary for that behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: bound merge fan-in in the final hash aggregate spill test [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #25424:
URL: https://github.com/apache/datafusion/pull/25424#issuecomment-5749264302
> Looks like https://github.com/apache/datafusion/actions/runs/35490410366/job/106024264216 failed with
Pushed change [dd046a0](https://github.com/apache/datafusion/pull/25424/commits/dd046a0f5423798ff16ffa2900eea3b8095fcebe)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: bound merge fan-in in the final hash aggregate spill test [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25424:
URL: https://github.com/apache/datafusion/pull/25424#issuecomment-5749168686
Looks like https://github.com/apache/datafusion/actions/runs/35490410366/job/106024264216 failed with
```
aggregates::hash_stream::tests::final_hash_spill_releases_memory_on_input_error
aggregates::hash_stream::tests::final_hash_spill_replay_releases_memory_on_drop
aggregates::hash_stream::tests::final_hash_spill_replay_with_other_partitions_holding_state
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: bound merge fan-in in the final hash aggregate spill test [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #25424:
URL: https://github.com/apache/datafusion/pull/25424#issuecomment-5718951495
@kosiew this is similar class of bug as https://github.com/apache/datafusion/pull/25252
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: bound merge fan-in in the final hash aggregate spill test [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #25424:
URL: https://github.com/apache/datafusion/pull/25424#issuecomment-5747661983
Thank you @kosiew for the review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: bound merge fan-in in the final hash aggregate spill test [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25424:
URL: https://github.com/apache/datafusion/pull/25424#issuecomment-5719250506
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25424?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `91.66667%` with `16 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.33%. Comparing base ([`1dea714`](https://app.codecov.io/gh/apache/datafusion/commit/1dea7142016e38af94893d488c796037202acf78?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`86c6e40`](https://app.codecov.io/gh/apache/datafusion/commit/86c6e408b4acb2d296f0b258ffa6ba93099a4425?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25424?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...fusion/physical-plan/src/aggregates/hash\_stream.rs](https://app.codecov.io/gh/apache/datafusion/pull/25424?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Fhash_stream.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL2hhc2hfc3RyZWFtLnJz) | 91.66% | [4 Missing and 12 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25424?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25424 +/- ##
========================================
Coverage 82.33% 82.33%
========================================
Files 1137 1137
Lines 431933 432240 +307
Branches 431933 432240 +307
========================================
+ Hits 355612 355886 +274
- Misses 54827 54839 +12
- Partials 21494 21515 +21
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25424?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: bound merge fan-in in the final hash aggregate spill test [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal merged PR #25424:
URL: https://github.com/apache/datafusion/pull/25424
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org