viirya commented on PR #25004: URL: https://github.com/apache/datafusion/pull/25004#issuecomment-5590401560
Thanks — you were right on both counts, and the `Pending` one was a real regression I introduced. **The `Pending` drop.** `Pending` is entered by every eligible execution before the shared load decides whether the left side actually spills, so cancelling on a drop there failed joins that never coordinate at all. I reproduced it with an ample pool: an execution whose left side fits in memory got cancelled by an unrelated partition going away. The drop is now recorded rather than acted on, and the coordinator decides. `pending_drop` and `coordination_started` are written and read under the same lock, so whichever of the two happens second performs the cancellation: - drop, then coordinate → `begin_coordination` picks up the recorded drop; - coordinate, then drop → `record_pending_drop` sees coordination has begun and cancels immediately, since that peer is already waiting on a probe report the departing partition will never make. My first attempt at this only handled the first order — nothing remembered that coordination had begun, so a plan that reached `Active` *before* the drop was still left hanging with the chunk reserved. A reviewer caught that the test I had written exercised the opposite order, which is why it passed. There is now a real-plan test for each direction, and I checked that removing the `coordination_started` read fails the one covering the order I had missed. An execution that resolves to `InMemory` never calls `begin_coordination`, so a recorded drop stays inert there — which is the behaviour you were asking for. **One more thing that came out of it.** `handle_done` pads an empty result with a single empty batch so an all-filtered join keeps its schema. A survivor cancelled before producing any rows took that path, so the stream returned `Some(Ok(<empty>))` *after* its cancellation error instead of ending. `cancelled_terminally` now short-circuits that, so a failure termination ends at `None`. Two of the watcher tests were also calling `cancel()` by hand instead of dropping a peer that had really reached `Active`; they now poll the peer into `Active`, assert it, and drop it, so they cover the actual `Drop` wiring. `nlj_cancel_wakes_stream_parked_on_build_input` keeps an explicit call, because its `left_data` never resolves and no peer there can coordinate — it asserts the drop stays inert first, and the comment says why the seam is there. joins 1174 pass, the memory-limit suite 38, clippy clean. -- 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]
