kosiew commented on code in PR #25250:
URL: https://github.com/apache/datafusion/pull/25250#discussion_r4013031121
##########
datafusion/physical-plan/src/joins/sort_merge_join/tests.rs:
##########
@@ -2465,6 +2467,87 @@ async fn overallocation_multi_batch_no_spill() ->
Result<()> {
Ok(())
}
+/// The stream spills its buffered side when it cannot grow, so it has to be
+/// registered as a consumer that can spill. A `FairSpillPool` otherwise treats
+/// it as unspillable: it is left out of the fair share the spillable consumers
+/// split, and may take everything they have not yet claimed, starving the
+/// sorts the join usually runs on top of.
+#[tokio::test]
+async fn stream_registers_as_a_spillable_consumer() -> Result<()> {
Review Comment:
Nice, this recording pool directly verifies the registration metadata
changed by this PR. One optional improvement would be to add a stronger
regression test using a constrained `FairSpillPool`, with the SMJ and at least
one other concurrently registered spillable consumer, such as the sorts feeding
it. The test could then verify that the join completes and that the SMJ
actually records a spill.
A large equal-key group by itself would not quite cover the regression,
since the SMJ can spill and complete both before and after this change.
Ideally, the test should fail without `.with_can_spill(true)` so it exercises
the fair-share behavior this fix is addressing.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
