viirya commented on PR #57221:
URL: https://github.com/apache/spark/pull/57221#issuecomment-4960656365
The change looks correct to me. I verified the tricky interactions: the
`global = false` restriction keeps the rule away from the `GlobalLimit <-
Shuffle(SinglePartition) <- ... <- Sort(global)` shape that
`InsertSortForLimitAndOffset` relies on; removing a local sort under a
round-robin repartition doesn't affect retry determinism since the shuffle's
internal `sortBeforeRepartition` sort is applied regardless of the child
ordering; and moving `RemoveRedundantSorts` after `OptimizeSkewedJoin` can't
regress skew detection, because the sorts its SMJ pattern needs sit above
shuffle stages whose `outputOrdering` is `Nil`, so the old position could never
remove them anyway.
A few comments:
- Could you add a test for the new removal kind to
`RemoveRedundantSortsSuite`? Something cheap like `sortWithinPartitions` +
`repartition` (or `SORT BY` under `DISTRIBUTE BY`) would exercise the new
pattern through both the AE and non-AE variants of that suite. Right now the
non-AQE side of this change (both the new case and the rule reorder in
`QueryExecution.preparations`) has no test backing it -- the new UT only covers
the AQE skew-join path.
- In `QueryExecution.preparations`, the existing comment
("`RemoveRedundantSorts` and `RemoveRedundantWindowGroupLimits` needs to be
added after `EnsureRequirements`...") now sits above
`RemoveRedundantWindowGroupLimits` while the rule it names has moved two lines
down, and unlike the AQE side there's no comment explaining the new position. A
one-liner noting it must stay after `DisableUnnecessaryBucketedScan` and mirror
the AQE rule order would help.
- On that note, I think the non-AQE reorder is more than a consistency
cleanup. With the old order there seems to be a latent correctness hazard: for
a query like `SELECT * FROM bucketed_sorted_table SORT BY sort_col` with no
interesting-partition operator above, `RemoveRedundantSorts` could remove the
sort based on the bucketed scan's `outputOrdering`, and then
`DisableUnnecessaryBucketedScan` disables the bucketed scan and the ordering is
silently lost. Running `RemoveRedundantSorts` after
`DisableUnnecessaryBucketedScan` closes that hole. Worth confirming, and if
it's real, mentioning it in the PR description and maybe adding a regression
test.
- Tiny nit: the new class doc could mention why the `outputOrdering.isEmpty`
guard is there (a custom `ShuffleExchangeLike` that preserves the child
ordering must keep the sort), so both conditions in the code have a matching
rationale in the doc.
--
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]
