viirya commented on PR #58097: URL: https://github.com/apache/spark/pull/58097#issuecomment-5614533527
A clarification on the intended scope of this PR and a change from my earlier response: **Configuration visibility:** I previously chose to keep `spark.sql.shuffle.localPipelined.enabled` public, with a one-time warning when the configured shuffle manager is incompatible. After reviewing the remaining restrictions, I have reconsidered that decision and marked the flag, along with the channel batch-size and queue-capacity settings, as internal. The feature remains opt-in and disabled by default. The mismatch warning is retained as a diagnostic, but we should treat this as experimental functionality while its supported execution paths are still limited. **The latest changes address two of the reported limitations:** both AQE and non-AQE planning now retain regular shuffles when the estimated whole-group task demand exceeds local capacity, without changing the requested partition count. `toLocalIterator` now uses a separate regular-shuffle plan so its per-partition jobs can reuse materialized shuffle output. Runtime slot admission still applies, and this does not provide general output reuse for every multi-job consumer. **Two broader capabilities remain deferred:** - Concurrent actions sharing the same pipelined exchange are still unsupported. The earlier lifecycle fix protects the owning action when another action is rejected; it does not enable concurrent sharing. - Recovery from a lost regular-shuffle prefix within a mixed pipelined job is not implemented. The SQL rewrite conservatively keeps such plans regular. Although the lower-level implementation can execute a mixed job with an already materialized prefix, that does not establish a complete recovery contract. The PR description still presents materialized-prefix mixed execution too broadly. I will update it to distinguish the lower-level capability from the supported SQL behavior and explicitly document these deferred items. My earlier replies should also have distinguished these fallbacks and safeguards more clearly from full support. The latest changes passed 101 tests and the relevant Scala style checks. Does this narrower, internal-only scope seem reasonable for this PR, with concurrent sharing and mixed-prefix recovery addressed separately? -- 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]
