[SPARK-25352][SQL] Perform ordered global limit when limit number is bigger than topKSortFallbackThreshold #22344

viirya · 2018-09-06T04:51:49Z

What changes were proposed in this pull request?

We have optimization on global limit to evenly distribute limit rows across all partitions. This optimization doesn't work for ordered results.

For a query ending with sort + limit, in most cases it is performed by TakeOrderedAndProjectExec.

But if limit number is bigger than SQLConf.TOP_K_SORT_FALLBACK_THRESHOLD, global limit will be used. At this moment, we need to do ordered global limit.

How was this patch tested?

Unit tests.

SparkQA · 2018-09-06T06:45:16Z

Test build #95737 has finished for PR 22344 at commit 8d49c1a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class GlobalLimitExec(limit: Int, child: SparkPlan,

viirya · 2018-09-06T07:01:32Z

retest this please.

…backThreshold.

SparkQA · 2018-09-06T08:46:11Z

Test build #95741 has finished for PR 22344 at commit 8d49c1a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class GlobalLimitExec(limit: Int, child: SparkPlan,

SparkQA · 2018-09-06T11:32:57Z

Test build #95746 has finished for PR 22344 at commit d8e2b7e.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class GlobalLimitExec(limit: Int, child: SparkPlan,

viirya · 2018-09-06T12:48:26Z

cc @hvanhovell @cloud-fan

SparkQA · 2018-09-12T01:40:43Z

Test build #95966 has finished for PR 22344 at commit 534e982.

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

viirya · 2018-09-12T01:50:07Z

retest this please.

SparkQA · 2018-09-12T03:32:04Z

Test build #95972 has finished for PR 22344 at commit 534e982.

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

viirya · 2018-09-12T03:39:02Z

retest this please.

SparkQA · 2018-09-12T07:05:01Z

Test build #95974 has finished for PR 22344 at commit 534e982.

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

viirya · 2018-09-12T07:07:20Z

retest this please...

SparkQA · 2018-09-12T10:57:15Z

Test build #95977 has finished for PR 22344 at commit 534e982.

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

viirya · 2018-09-12T11:19:04Z

ping @cloud-fan seems to me we should consider to include this in 2.4.

cloud-fan · 2018-09-12T14:55:09Z

thanks, merging to master/2.4!

…bigger than topKSortFallbackThreshold ## What changes were proposed in this pull request? We have optimization on global limit to evenly distribute limit rows across all partitions. This optimization doesn't work for ordered results. For a query ending with sort + limit, in most cases it is performed by `TakeOrderedAndProjectExec`. But if limit number is bigger than `SQLConf.TOP_K_SORT_FALLBACK_THRESHOLD`, global limit will be used. At this moment, we need to do ordered global limit. ## How was this patch tested? Unit tests. Closes #22344 from viirya/SPARK-25352. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 2f42239) Signed-off-by: Wenchen Fan <[email protected]>

hvanhovell · 2018-09-12T15:05:45Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

-            if limit < conf.topKSortFallbackThreshold =>
-          TakeOrderedAndProjectExec(limit, order, projectList, planLater(child)) :: Nil
+        case Limit(IntegerLiteral(limit), s@Sort(order, true, child)) =>
+          if (limit < conf.topKSortFallbackThreshold) {


@viirya sorry to be a little late to the party. This pattern is repeated 4x can you just move it into a helper function?

@hvanhovell OK. I will create a follow-up PR.

Also, please add space in-between s and @.

btw here we really need to document what the strategies are. when there were only two cases it's not a big deal because it'd take a few seconds to understand. but this block is pretty large now that's difficult to understand. see join strategy documentation for example.

…bigger than topKSortFallbackThreshold ## What changes were proposed in this pull request? We have optimization on global limit to evenly distribute limit rows across all partitions. This optimization doesn't work for ordered results. For a query ending with sort + limit, in most cases it is performed by `TakeOrderedAndProjectExec`. But if limit number is bigger than `SQLConf.TOP_K_SORT_FALLBACK_THRESHOLD`, global limit will be used. At this moment, we need to do ordered global limit. ## How was this patch tested? Unit tests. Closes apache#22344 from viirya/SPARK-25352. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

rxin · 2018-09-18T23:11:33Z

sql/core/src/main/scala/org/apache/spark/sql/execution/limit.scala

 */
-case class GlobalLimitExec(limit: Int, child: SparkPlan) extends UnaryExecNode {
+case class GlobalLimitExec(limit: Int, child: SparkPlan,
+                           orderedLimit: Boolean = false) extends UnaryExecNode {


what does orderedLimit mean here?

It means this global limit won't change input data order. This is used on sort + limit case which is usually taken by TakeOrderedAndProjectExec at most of time.

But if limit number is more than SQLConf.TOP_K_SORT_FALLBACK_THRESHOLD, it's not goes for TakeOrderedAndProjectExec.

what do you mean by "it's not goes for TakeOrderedAndProjectExec"?

When limit number is more than SQLConf.TOP_K_SORT_FALLBACK_THRESHOLD, the planner won't choose TakeOrderedAndProjectExec to perform the sort + limit operation.

please document it in code.

Ok, I see. I will document it in the pr.

code needs to be documented. we won't find this pr discussion a year from now by looking at the source code, trying to figure out what it means. also the doc needs to be readable. the current doc for the config flag is unfortunately unparsable.

I see. Let me submit a pr later to address those document. Really appreciate your comment.

thanks @viirya

can you write a design doc or put it in the classdoc of limit on how we handle limits? your sequence of prs are making limits much more complicated (with optimizations) and very difficult to reason about. i think we can make it easier to reason about, if we actually document the execution strategy.

Ok. I will do it too.

rxin · 2018-09-18T23:13:45Z

guys - the whole sequence of prs for this feature are contributing a lot of cryptic code with arcane documentation everywhere. i worry a lot about the maintainability of the code that's coming in. can you submit a pr to improve the readability? otherwise i think we should revert all of them.

please document the algorithm, the change, the config flags. it shouldn't require reading the actual implementation to understand the config flags.

we as committers should also do a better job at gating unreadable code.

viirya · 2018-09-18T23:33:55Z

Thanks @rxin. I will submit a pr to improve the readability.

## What changes were proposed in this pull request? This goes to revert sequential PRs based on some discussion and comments at #16677 (comment). #22344 #22330 #22239 #16677 ## How was this patch tested? Existing tests. Closes #22481 from viirya/revert-SPARK-19355-1. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 89671a2) Signed-off-by: Wenchen Fan <[email protected]>

## What changes were proposed in this pull request? This goes to revert sequential PRs based on some discussion and comments at #16677 (comment). #22344 #22330 #22239 #16677 ## How was this patch tested? Existing tests. Closes #22481 from viirya/revert-SPARK-19355-1. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

WeiWenda · 2019-08-05T05:17:11Z

@viirya I meet the same problem when running "insert owerwrite xx select xx from xx sort by xx limit xx". which return a wrong answer, How does this pr going after revert?

Do ordered global limit when limit number is bigger than topKSortFall…

d8e2b7e

…backThreshold.

viirya force-pushed the SPARK-25352 branch from 8d49c1a to d8e2b7e Compare September 6, 2018 07:43

Merge remote-tracking branch 'upstream/master' into SPARK-25352

534e982

asfgit closed this in 2f42239 Sep 12, 2018

hvanhovell reviewed Sep 12, 2018

View reviewed changes

rxin reviewed Sep 18, 2018

View reviewed changes

This was referenced Sep 19, 2018

Revert [SPARK-19355][SPARK-25352] #22464

Closed

Revert [SPARK-19355][SPARK-25352] #22481

Closed

viirya deleted the SPARK-25352 branch December 27, 2023 18:35

[SPARK-25352][SQL] Perform ordered global limit when limit number is bigger than topKSortFallbackThreshold #22344

[SPARK-25352][SQL] Perform ordered global limit when limit number is bigger than topKSortFallbackThreshold #22344

Uh oh!

Conversation

viirya commented Sep 6, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Sep 6, 2018

Uh oh!

viirya commented Sep 6, 2018

Uh oh!

SparkQA commented Sep 6, 2018

Uh oh!

SparkQA commented Sep 6, 2018

Uh oh!

viirya commented Sep 6, 2018

Uh oh!

SparkQA commented Sep 12, 2018

Uh oh!

viirya commented Sep 12, 2018

Uh oh!

SparkQA commented Sep 12, 2018

Uh oh!

viirya commented Sep 12, 2018

Uh oh!

SparkQA commented Sep 12, 2018

Uh oh!

viirya commented Sep 12, 2018

Uh oh!

SparkQA commented Sep 12, 2018

Uh oh!

viirya commented Sep 12, 2018

Uh oh!

cloud-fan commented Sep 12, 2018

Uh oh!

hvanhovell Sep 12, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxin commented Sep 18, 2018

Uh oh!

viirya commented Sep 18, 2018

Uh oh!

WeiWenda commented Aug 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hvanhovell Sep 12, 2018 •

edited

Loading