geoffreyclaude commented on PR #26242: URL: https://github.com/apache/datafusion/pull/26242#issuecomment-6316148214
> Thanks for the fix. The permit-before-spawn design and the ordered reassembly look sound to me. Notes below, most important first. > > * **Coverage of the new path.** `sort_fuzz.rs` and `spilling_fuzz_in_memory_constrained_env.rs` use `#[tokio::test]` (current-thread), so they only run the inline path. The default `sort_in_place_threshold_bytes` is 1 MiB, so SLT inputs mostly collapse into one run and skip the spawned path too. The spawned path is then covered only by the unit and spill tests here. A multi-thread variant of the sort fuzz with a small threshold, asserting the same output as the inline path, would cover run counts, batch sizes and spill together. > * **Benchmark evidence.** The description gives the before numbers for the 32-row reproducer but no after numbers (alive task count, `SELECT 1` probe latency). The bot runs were on `d1a59c1`, which is earlier than the current head `cc90f9e`, and TPC-H sort inputs are not fragmented, so "no change" says little about the changed path. Please post the reproducer numbers on the current head, or add a small-batch case to an existing sort bench. > * **Budget scope.** The `OnceLock` lives on the plan and is shared by every clone, so two concurrent executions of one plan instance share the permits, and a re-run on another runtime keeps the first runtime's worker count until `reset_state`. Probably acceptable, but worth one sentence in the `SortExec` rustdoc if intended. @comphead Thanks for the detailed review! Addressed all three of the above (as well as the inline comments): 1. Added [multi-threaded fuzz coverage](https://github.com/apache/datafusion/commit/c2e0ab34fb4) comparing inline and spawned output in order, while preserving the existing randomized configurations and current-thread tests. 2. Reran the fragmented reproducer on the current head: peak tasks dropped from ~187k to 9, and the slowest `SELECT 1` probe from 2.83–2.89 s to 155–165 ms. The description includes these results. Fresh `sort_tpch` and `sort_tpch10` bot runs also exhibit "no change". 3. [Clarified the rustdoc](https://github.com/apache/datafusion/commit/416c2d69202): partitions, clones and concurrent executions share the budget. Its limit is initialized once; `reset_state` creates a fresh one. -- 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]
