viirya commented on PR #57221: URL: https://github.com/apache/spark/pull/57221#issuecomment-4971441131
Thanks for the update! The bucketed-scan regression test looks right to me -- I checked that `repartition(1)` gives one file per bucket so the scan reports an ordering under the legacy conf, and that `EnableAdaptiveExecutionSuite`'s `forceApply` routes even this exchange-free query through the AQE prep chain, so both rule chains are guarded. The doc and comment updates look good too. Two remaining points: - I don't think the `EliminateSorts` justification holds. That rule only strips sorts below an upper `Sort` / `Join` / order-irrelevant `Aggregate` (repartitions are just pass-through nodes in `recursiveRemoveSort`), and `CollapseRepartition` only removes a **global** sort under `RepartitionByExpression` (plus any sort under `RebalancePartitions`, and nothing under a round-robin `Repartition`). So e.g. `SELECT /*+ REPARTITION(3) */ * FROM (SELECT * FROM t SORT BY key)` should survive to the physical plan as `Shuffle(RoundRobin) <- Sort(local)` and reach the new branch end-to-end. Could you double-check? If it does, a `checkSorts`-style end-to-end test would be stronger than invoking the rule directly (it also exercises the rule's position in the real preparation chains), and the test comment stating the shape "cannot be produced from SQL/DataFrame directly" needs fixing. Keeping the direct-invocation test for the global-sort-preservation case is still nice. - Minor: the new comment in `QueryExecution.preparations` says the rule is placed after `DisableUnnecessaryBucketedScan` "to mirror the AQE rule order", but as your new regression test shows, running after `DisableUnnecessaryBucketedScan` is a correctness requirement (disabling a bucketed scan drops its output ordering, which must be reflected before sort-removal decisions). I'd state that as the reason so a future reorder doesn't look harmless. -- 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]
