[SPARK-33442][SQL] Change Combine Limit to Eliminate limit using max row #30368

ulysses-you · 2020-11-13T09:20:23Z

What changes were proposed in this pull request?

Change CombineLimits name to EliminateLimits and add check if Limit child max row <= limit.

Why are the changes needed?

In Add-hoc scene, we always add limit for the query if user have no special limit value, but not all limit is nesessary.

A general negative example is

select count(*) from t limit 100000;

It will be great if we can eliminate limit at Spark side.

Also, we make a benchmark for this case

runBenchmark("Sort and Limit") {
  val N = 100000
  val benchmark = new Benchmark("benchmark sort and limit", N)

  benchmark.addCase("TakeOrderedAndProject", 3) { _ =>
    spark.range(N).toDF("c").repartition(200).sort("c").take(200000)
  }

  benchmark.addCase("Sort And Limit", 3) { _ =>
    withSQLConf("spark.sql.execution.topKSortFallbackThreshold" -> "-1") {
      spark.range(N).toDF("c").repartition(200).sort("c").take(200000)
    }
  }

  benchmark.addCase("Sort", 3) { _ =>
    spark.range(N).toDF("c").repartition(200).sort("c").collect()
  }
  benchmark.run()
}

and the result is

Java HotSpot(TM) 64-Bit Server VM 1.8.0_191-b12 on Mac OS X 10.15.6
Intel(R) Core(TM) i5-5257U CPU @ 2.70GHz
benchmark sort and limit:                 Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
TakeOrderedAndProject                              1833           2259         382          0.1       18327.1       1.0X
Sort And Limit                                     1417           1658         285          0.1       14167.5       1.3X
Sort                                               1324           1484         225          0.1       13238.3       1.4X

It shows that it makes sense to replace TakeOrderedAndProjectExec with Sort + Project.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add test.

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

SparkQA · 2020-11-13T10:55:18Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35659/

SparkQA · 2020-11-13T11:17:51Z

Test build #131054 has finished for PR 30368 at commit 67fd737.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-11-13T11:22:16Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35659/

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

SparkQA · 2020-11-13T14:29:29Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35670/

SparkQA · 2020-11-13T14:55:46Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35670/

SparkQA · 2020-11-13T15:02:06Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35672/

SparkQA · 2020-11-13T15:34:32Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35672/

SparkQA · 2020-11-13T16:41:34Z

Test build #131066 has finished for PR 30368 at commit 0a89334.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-11-13T17:57:34Z

Test build #131068 has finished for PR 30368 at commit 2c6b171.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

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

SparkQA · 2020-11-14T14:40:08Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35695/

SparkQA · 2020-11-14T15:01:51Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35695/

SparkQA · 2020-11-14T17:24:28Z

Test build #131092 has finished for PR 30368 at commit c167a0d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2020-11-14T18:20:15Z

sql/core/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q92/explain.txt

@@ -1,5 +1,5 @@
 == Physical Plan ==
-TakeOrderedAndProject (34)
+* Sort (34)


It changes from TakeOrderedAndProject to Sort seems because Limit after Sort is removed?

It might have additional shuffle for global Sort.

This q92 sql:

SELECT sum(ws_ext_discount_amt) AS `Excess Discount Amount ` FROM web_sales, item, date_dim WHERE i_manufact_id = 350 AND i_item_sk = ws_item_sk AND d_date BETWEEN '2000-01-27' AND (cast('2000-01-27' AS DATE) + INTERVAL 90 days) AND d_date_sk = ws_sold_date_sk AND ws_ext_discount_amt > ( SELECT 1.3 * avg(ws_ext_discount_amt) FROM web_sales, date_dim WHERE ws_item_sk = i_item_sk AND d_date BETWEEN '2000-01-27' AND (cast('2000-01-27' AS DATE) + INTERVAL 90 days) AND d_date_sk = ws_sold_date_sk ) ORDER BY sum(ws_ext_discount_amt) LIMIT 100

yes, Limit after Sort is a special case, we will convert to TakeOrderedAndProject, but it seems not necessary to do both sort and limit if child maxRow == 1. Maybe we can do an another check seems like if sort.child.maxRow <= 1 then remove sort ?

Other thought, we can add this pattern

case sort @ Sort(order, true, child) if sort.maxRow < conf.topKSortFallbackThreshold => TakeOrderedAndProjectExec()

In this way, we can infer the TakeOrderedAndProjectExec from Sort which has not Limit after.

What do you think about this? @maropu @viirya @cloud-fan

Ah, @viirya , nice catch! Yea, how about simply excluding the case in the EliminateLimits rule? @ulysses-you

SparkQA · 2020-11-15T03:41:04Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35707/

SparkQA · 2020-11-15T04:05:20Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35707/

SparkQA · 2020-11-15T07:19:03Z

Test build #131104 has finished for PR 30368 at commit ff1ab01.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-11-16T13:47:20Z

Test build #131160 has finished for PR 30368 at commit 4d45c0a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

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

SparkQA · 2020-11-16T14:00:36Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35763/

SparkQA · 2020-11-16T14:31:24Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35763/

SparkQA · 2020-11-17T01:07:54Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35784/

SparkQA · 2020-11-17T01:37:23Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35784/

SparkQA · 2020-11-17T03:33:52Z

Test build #131182 has finished for PR 30368 at commit 2f8f139.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

…3442

SparkQA · 2020-11-17T06:36:41Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35800/

SparkQA · 2020-11-17T07:06:06Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35800/

SparkQA · 2020-11-17T07:12:21Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35802/

SparkQA · 2020-11-17T07:36:43Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35802/

SparkQA · 2020-11-17T08:05:02Z

Test build #131199 has finished for PR 30368 at commit 45ab8b5.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-11-17T08:05:02Z

Test build #131197 has finished for PR 30368 at commit 9fb4039.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

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

cloud-fan

LGTM

SparkQA · 2020-11-17T15:30:18Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35828/

SparkQA · 2020-11-17T15:53:41Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35828/

SparkQA · 2020-11-17T19:15:01Z

Test build #131225 has finished for PR 30368 at commit 2f55ec0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-11-18T14:05:57Z

Hi @ulysses-you , can you put the conclusion of #30368 (comment) in PR description, to mention that we may end up replacing TakeOrderedAndProjectExec with Sort + Project, which is faster?

ulysses-you · 2020-11-19T00:08:05Z

@cloud-fan updated the description.

cloud-fan · 2020-11-19T13:31:02Z

thanks, merging to master!

ulysses-you · 2020-11-20T00:06:06Z

thanks for merging!

init

67fd737

ulysses-you commented Nov 13, 2020

View reviewed changes

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

github-actions bot added the SQL label Nov 13, 2020

maropu reviewed Nov 13, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala Outdated Show resolved Hide resolved

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

ulysses-you added 3 commits November 13, 2020 20:29

fix LimitPushdownSuite

46aea2e

keep global limit

0a89334

use long

2c6b171

fix StreamSuite

e570b8e

viirya reviewed Nov 14, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala Outdated Show resolved Hide resolved

ulysses-you added 2 commits November 14, 2020 21:48

golden file

314cb1e

remove eliminate local limit

c167a0d

github-actions bot added the STRUCTURED STREAMING label Nov 14, 2020

viirya reviewed Nov 14, 2020

View reviewed changes

golden file

ff1ab01

skip sort after limit

6576929

golden file

4d45c0a

cloud-fan reviewed Nov 16, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala Outdated Show resolved Hide resolved

fix

2f8f139

ulysses-you added 3 commits November 17, 2020 13:47

Merge branch 'master' of https://github.com/apache/spark into SPARK-3…

2882a36

…3442

fix test

9fb4039

golden file

45ab8b5

cloud-fan reviewed Nov 17, 2020

View reviewed changes

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

cloud-fan approved these changes Nov 17, 2020

View reviewed changes

add comment

2f55ec0

cloud-fan closed this in 21b1350 Nov 19, 2020

ulysses-you deleted the SPARK-33442 branch March 3, 2021 04:30

[SPARK-33442][SQL] Change Combine Limit to Eliminate limit using max row #30368

[SPARK-33442][SQL] Change Combine Limit to Eliminate limit using max row #30368

Conversation

ulysses-you commented Nov 13, 2020 • edited Loading

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?

SparkQA commented Nov 13, 2020

SparkQA commented Nov 13, 2020

SparkQA commented Nov 13, 2020

SparkQA commented Nov 13, 2020

SparkQA commented Nov 13, 2020

SparkQA commented Nov 13, 2020

SparkQA commented Nov 13, 2020

SparkQA commented Nov 13, 2020

SparkQA commented Nov 13, 2020

SparkQA commented Nov 14, 2020

SparkQA commented Nov 14, 2020

SparkQA commented Nov 14, 2020

viirya Nov 14, 2020

Choose a reason for hiding this comment

ulysses-you Nov 15, 2020

Choose a reason for hiding this comment

ulysses-you Nov 15, 2020 • edited Loading

Choose a reason for hiding this comment

maropu Nov 15, 2020 • edited Loading

Choose a reason for hiding this comment

ulysses-you Nov 16, 2020

Choose a reason for hiding this comment

SparkQA commented Nov 15, 2020

SparkQA commented Nov 15, 2020

SparkQA commented Nov 15, 2020

SparkQA commented Nov 16, 2020

SparkQA commented Nov 16, 2020

SparkQA commented Nov 16, 2020

SparkQA commented Nov 17, 2020

SparkQA commented Nov 17, 2020

SparkQA commented Nov 17, 2020

SparkQA commented Nov 17, 2020

SparkQA commented Nov 17, 2020

SparkQA commented Nov 17, 2020

SparkQA commented Nov 17, 2020

SparkQA commented Nov 17, 2020

SparkQA commented Nov 17, 2020

cloud-fan left a comment

Choose a reason for hiding this comment

SparkQA commented Nov 17, 2020

SparkQA commented Nov 17, 2020

SparkQA commented Nov 17, 2020

cloud-fan commented Nov 18, 2020

ulysses-you commented Nov 19, 2020

cloud-fan commented Nov 19, 2020

ulysses-you commented Nov 20, 2020

ulysses-you commented Nov 13, 2020 •

edited

Loading

ulysses-you Nov 15, 2020 •

edited

Loading

maropu Nov 15, 2020 •

edited

Loading