[SPARK-39835][SQL] Fix EliminateSorts remove global sort below the local sort #37250

ulysses-you · 2022-07-22T03:47:06Z

What changes were proposed in this pull request?

Correct the EliminateSorts follows:

If the upper sort is global then we can remove the global or local sort recursively.
If the upper sort is local then we can only remove the local sort recursively.

Why are the changes needed?

If a global sort below locol sort, we should not remove the global sort becuase the output partitioning can be affected.

This issue is going to worse since we pull out the V1 Write sort to logcial side.

Does this PR introduce any user-facing change?

yes, bug fix

How was this patch tested?

add test

ulysses-you · 2022-07-22T04:05:30Z

cc @cloud-fan @allisonwang-db

beliefer · 2022-07-22T07:37:17Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

-  private def recursiveRemoveSort(plan: LogicalPlan): LogicalPlan = {
+  /**
+   * If the upper sort is global then we can remove the global or local sort recursively.
+   * If the upper sort is local then we can only remove the local sort recursively.


If the upper Sort is local, why we can't remove the global Sort recursively ?

I think we can eliminate it too.

Think about what if we remove all the Repartition nodes, will users complain? They will even if Repartition does not change the data, but only change the partitioning. Data partitioning is also a user expectation that we shouldn't break.

The semantics of global + local sort should be range partition + local sort, so we can not remove the global sort which is under local sort as we can not remove range partition directly. BTW, I will add a new rule to optimzie this pattern after fix this pr.

yeah I also agree we should not remove the global Sort, as user might have expectation of overall range partitioning.

cloud-fan · 2022-07-22T15:41:10Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

    }
    plan match {
-      case Sort(_, _, child) => recursiveRemoveSort(child)
+      case Sort(_, _, child) if canRemoveGlobalSort =>


nit:

case Sort(_, global, child) if canRemoveGlobalSort || !global =>

cloud-fan · 2022-07-22T15:42:41Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/EliminateSortsSuite.scala

  }
+
+  test("SPARK-39835: Fix EliminateSorts remove global sort below the local sort") {
+    // local - global


this is a bit hard to read as I don't know which one is child. How about global -> local?

cloud-fan · 2022-07-22T15:43:05Z

cc @sigmod

cloud-fan · 2022-07-25T07:43:16Z

@ulysses-you do you know which branch we start to have this bug?

ulysses-you · 2022-07-25T07:53:08Z

@cloud-fan branch-3.0

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

Lines 1015 to 1023 in ea05c33

    
             case s @ Sort(_, _, child) => s.copy(child = recursiveRemoveSort(child)) 
        
             case j @ Join(originLeft, originRight, _, cond, _) if cond.forall(_.deterministic) => 
        
               j.copy(left = recursiveRemoveSort(originLeft), right = recursiveRemoveSort(originRight)) 
        
             case g @ Aggregate(_, aggs, originChild) if isOrderIrrelevantAggs(aggs) => 
        
               g.copy(child = recursiveRemoveSort(originChild)) 
        
           } 
        
           private def recursiveRemoveSort(plan: LogicalPlan): LogicalPlan = plan match { 
        
             case Sort(_, _, child) => recursiveRemoveSort(child)

I guess there are a lot of conflicts for each branch ..

cloud-fan · 2022-07-25T10:34:35Z

The GA failure is unrelated, thanks, merging to master/3.3!

…cal sort ### What changes were proposed in this pull request? Correct the `EliminateSorts` follows: - If the upper sort is global then we can remove the global or local sort recursively. - If the upper sort is local then we can only remove the local sort recursively. ### Why are the changes needed? If a global sort below locol sort, we should not remove the global sort becuase the output partitioning can be affected. This issue is going to worse since we pull out the V1 Write sort to logcial side. ### Does this PR introduce _any_ user-facing change? yes, bug fix ### How was this patch tested? add test Closes #37250 from ulysses-you/remove-sort. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 5dca26d) Signed-off-by: Wenchen Fan <[email protected]>

cloud-fan · 2022-07-25T10:36:04Z

@ulysses-you can you open backport PRs for 3.2 and 3.1? thanks!

ulysses-you · 2022-07-25T12:45:17Z

@cloud-fan created #37276 and #37275

…cal sort Correct the `EliminateSorts` follows: - If the upper sort is global then we can remove the global or local sort recursively. - If the upper sort is local then we can only remove the local sort recursively. If a global sort below locol sort, we should not remove the global sort becuase the output partitioning can be affected. This issue is going to worse since we pull out the V1 Write sort to logcial side. yes, bug fix add test Closes apache#37250 from ulysses-you/remove-sort. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

this is for backport #37330 into branch-3.3 ### What changes were proposed in this pull request? Optimize Global sort to RepartitionByExpression, for example: ``` Sort local Sort local Sort global => RepartitionByExpression ``` ### Why are the changes needed? If a global sort below a local sort, the only meaningful thing is it's distribution. So this pr optimizes that global sort to RepartitionByExpression to save a local sort. ### Does this PR introduce _any_ user-facing change? we fix a bug in #37250 and that pr backport into branch-3.3. However, that fix may introduce performance regression. This pr itself is only to improve performance but in order to avoid the regression, we also backport this pr. see the details #37330 (comment) ### How was this patch tested? add test Closes #37330 from ulysses-you/optimize-sort. Authored-by: ulysses-you <ulyssesyou18gmail.com> Signed-off-by: Wenchen Fan <wenchendatabricks.com> Closes #37373 from ulysses-you/SPARK-39911-3.3. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…he local sort backport #37250 into branch-3.1 ### What changes were proposed in this pull request? Correct the `EliminateSorts` follows: - If the upper sort is global then we can remove the global or local sort recursively. - If the upper sort is local then we can only remove the local sort recursively. ### Why are the changes needed? If a global sort below locol sort, we should not remove the global sort becuase the output partitioning can be affected. This issue is going to worse since we pull out the V1 Write sort to logcial side. ### Does this PR introduce _any_ user-facing change? yes, bug fix ### How was this patch tested? add test Closes #37276 from ulysses-you/SPARK-39835-3.1. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…he local sort backport #37250 into branch-3.2 ### What changes were proposed in this pull request? Correct the `EliminateSorts` follows: - If the upper sort is global then we can remove the global or local sort recursively. - If the upper sort is local then we can only remove the local sort recursively. ### Why are the changes needed? If a global sort below locol sort, we should not remove the global sort becuase the output partitioning can be affected. This issue is going to worse since we pull out the V1 Write sort to logcial side. ### Does this PR introduce _any_ user-facing change? yes, bug fix ### How was this patch tested? add test Closes #37275 from ulysses-you/SPARK-39835-3.2. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…he local sort backport apache#37250 into branch-3.2 ### What changes were proposed in this pull request? Correct the `EliminateSorts` follows: - If the upper sort is global then we can remove the global or local sort recursively. - If the upper sort is local then we can only remove the local sort recursively. ### Why are the changes needed? If a global sort below locol sort, we should not remove the global sort becuase the output partitioning can be affected. This issue is going to worse since we pull out the V1 Write sort to logcial side. ### Does this PR introduce _any_ user-facing change? yes, bug fix ### How was this patch tested? add test Closes apache#37275 from ulysses-you/SPARK-39835-3.2. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 265bd21)

github-actions bot added the SQL label Jul 22, 2022

Fix EliminateSorts remove global sort below the local sort

1ee454d

ulysses-you force-pushed the remove-sort branch from eea2395 to 1ee454d Compare July 22, 2022 05:47

beliefer reviewed Jul 22, 2022

View reviewed changes

cloud-fan reviewed Jul 22, 2022

View reviewed changes

address comment

09efde8

cloud-fan closed this in 5dca26d Jul 25, 2022

ulysses-you deleted the remove-sort branch July 25, 2022 12:29

ulysses-you mentioned this pull request Jul 25, 2022

[SPARK-39835][SQL][3.2] Fix EliminateSorts remove global sort below the local sort #37275

Closed

ulysses-you mentioned this pull request Jul 25, 2022

[SPARK-39835][SQL][3.1] Fix EliminateSorts remove global sort below the local sort #37276

Closed

cloud-fan mentioned this pull request Aug 2, 2022

[SPARK-39911][SQL] Optimize global Sort to RepartitionByExpression #37330

Closed

ulysses-you mentioned this pull request Aug 2, 2022

[SPARK-39911][SQL][3.3] Optimize global Sort to RepartitionByExpression #37373

Closed

[SPARK-39835][SQL] Fix EliminateSorts remove global sort below the local sort #37250

[SPARK-39835][SQL] Fix EliminateSorts remove global sort below the local sort #37250

Uh oh!

Conversation

ulysses-you commented Jul 22, 2022

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

ulysses-you commented Jul 22, 2022

Uh oh!

beliefer Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

ulysses-you Jul 23, 2022

Choose a reason for hiding this comment

Uh oh!

c21 Jul 24, 2022

Choose a reason for hiding this comment

Uh oh!

beliefer Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jul 22, 2022

Uh oh!

cloud-fan commented Jul 25, 2022

Uh oh!

ulysses-you commented Jul 25, 2022

Uh oh!

cloud-fan commented Jul 25, 2022

Uh oh!

cloud-fan commented Jul 25, 2022

Uh oh!

ulysses-you commented Jul 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

beliefer Jul 22, 2022 •

edited

Loading