-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-23973][SQL] Remove consecutive Sorts #21072
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
6ba4186
ff7d412
ac03bed
1d6ca1e
e7391f3
e2f4d4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -736,12 +736,22 @@ object EliminateSorts extends Rule[LogicalPlan] { | |
| } | ||
|
|
||
| /** | ||
| * Removes Sort operation if the child is already sorted | ||
| * Removes redundant Sort operation. This can happen: | ||
| * 1) if the child is already sorted | ||
| * 2) if the there is another Sort operator separated by 0...n Project/Filter operators | ||
| */ | ||
| object RemoveRedundantSorts extends Rule[LogicalPlan] { | ||
| def apply(plan: LogicalPlan): LogicalPlan = plan transform { | ||
|
||
| case Sort(orders, true, child) if SortOrder.orderingSatisfies(child.outputOrdering, orders) => | ||
| child | ||
| case s @ Sort(_, _, child) => s.copy(child = recursiveRemoveSort(child)) | ||
| } | ||
|
|
||
| def recursiveRemoveSort(plan: LogicalPlan): LogicalPlan = plan match { | ||
| case Project(fields, child) => Project(fields, recursiveRemoveSort(child)) | ||
| case Filter(condition, child) => Filter(condition, recursiveRemoveSort(child)) | ||
|
||
| case Sort(_, _, child) => recursiveRemoveSort(child) | ||
| case _ => plan | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,15 +42,15 @@ class RemoveRedundantSortsSuite extends PlanTest { | |
|
|
||
| test("remove redundant order by") { | ||
| val orderedPlan = testRelation.select('a, 'b).orderBy('a.asc, 'b.desc_nullsFirst) | ||
| val unnecessaryReordered = orderedPlan.select('a).orderBy('a.asc, 'b.desc_nullsFirst) | ||
| val unnecessaryReordered = orderedPlan.limit(2).select('a).orderBy('a.asc, 'b.desc_nullsFirst) | ||
| val optimized = Optimize.execute(unnecessaryReordered.analyze) | ||
| val correctAnswer = orderedPlan.select('a).analyze | ||
| val correctAnswer = orderedPlan.limit(2).select('a).analyze | ||
| comparePlans(Optimize.execute(optimized), correctAnswer) | ||
| } | ||
|
|
||
| test("do not remove sort if the order is different") { | ||
| val orderedPlan = testRelation.select('a, 'b).orderBy('a.asc, 'b.desc_nullsFirst) | ||
| val reorderedDifferently = orderedPlan.select('a).orderBy('a.asc, 'b.desc) | ||
| val reorderedDifferently = orderedPlan.limit(2).select('a).orderBy('a.asc, 'b.desc) | ||
| val optimized = Optimize.execute(reorderedDifferently.analyze) | ||
| val correctAnswer = reorderedDifferently.analyze | ||
| comparePlans(optimized, correctAnswer) | ||
|
|
@@ -98,4 +98,31 @@ class RemoveRedundantSortsSuite extends PlanTest { | |
| val correctAnswer = groupedAndResorted.analyze | ||
| comparePlans(optimized, correctAnswer) | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a test which explicitly confirms that sort.limit.sort is not simplified? I know the above two tests cover that case, but it's good to have one dedicated to testing this important property. |
||
| test("remove two consecutive sorts") { | ||
| val orderedTwice = testRelation.orderBy('a.asc).orderBy('b.desc) | ||
| val optimized = Optimize.execute(orderedTwice.analyze) | ||
| val correctAnswer = testRelation.orderBy('b.desc).analyze | ||
| comparePlans(optimized, correctAnswer) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a test for three consecutive sorts? Two is the base case, three will help us show the inductive case :) |
||
|
|
||
| test("remove sorts separated by Filter/Project operators") { | ||
| val orderedTwiceWithProject = testRelation.orderBy('a.asc).select('b).orderBy('b.desc) | ||
| val optimizedWithProject = Optimize.execute(orderedTwiceWithProject.analyze) | ||
| val correctAnswerWithProject = testRelation.select('b).orderBy('b.desc).analyze | ||
| comparePlans(optimizedWithProject, correctAnswerWithProject) | ||
|
|
||
| val orderedTwiceWithFilter = | ||
| testRelation.orderBy('a.asc).where('b > Literal(0)).orderBy('b.desc) | ||
| val optimizedWithFilter = Optimize.execute(orderedTwiceWithFilter.analyze) | ||
| val correctAnswerWithFilter = testRelation.where('b > Literal(0)).orderBy('b.desc).analyze | ||
| comparePlans(optimizedWithFilter, correctAnswerWithFilter) | ||
|
|
||
| val orderedTwiceWithBoth = | ||
| testRelation.orderBy('a.asc).select('b).where('b > Literal(0)).orderBy('b.desc) | ||
| val optimizedWithBoth = Optimize.execute(orderedTwiceWithBoth.analyze) | ||
| val correctAnswerWithBoth = | ||
| testRelation.select('b).where('b > Literal(0)).orderBy('b.desc).analyze | ||
| comparePlans(optimizedWithBoth, correctAnswerWithBoth) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 'the there'