ulysses-you commented on PR #57221: URL: https://github.com/apache/spark/pull/57221#issuecomment-4967114213
@viirya thanks for the careful review! Addressed all four points in the followup commit: 1. **Bucketed-scan hazard confirmed and it's real.** Under the old order, with `spark.sql.legacy.bucketedTableScan.outputOrdering=true` a query like `SELECT * FROM bucketed_sorted_table SORT BY sort_col` (no interesting-partition operator above) has its sort stripped by `RemoveRedundantSorts` based on the scan's `outputOrdering`, then `DisableUnnecessaryBucketedScan` disables the scan and the ordering is silently lost. Running `RemoveRedundantSorts` after `DisableUnnecessaryBucketedScan` closes it. Added regression test `keep local sort satisfied only by a bucketed scan output ordering` (fails on the old order, passes now) and mentioned it in the PR description. 2. **`RemoveRedundantSortsSuite` coverage.** Note your suggested `sortWithinPartitions + repartition` / `SORT BY under DISTRIBUTE BY` shapes can't reach the new branch: the logical `EliminateSorts` strips a local sort sitting below a repartition before physical planning, so the dangling-sort shape is physical-plan-only. I added a test that applies the rule directly to a physical plan (local-sort removal, global-sort preservation, rule-disabled), plus the bucketed-scan test above -- both run through the AE and non-AE variants. 3. **`QueryExecution.preparations` comment** updated: the stale comment no longer names the moved rule, and there's a one-liner noting `RemoveRedundantSorts` sits after `DisableUnnecessaryBucketedScan` to mirror the AQE order. 4. **Class doc** now explains the `outputOrdering.isEmpty` guard: a custom `ShuffleExchangeLike` that preserves child ordering reports a non-empty `outputOrdering`, so its local sort is not dead and must be kept. -- 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]
