Remove redundant sort columns#21371
Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom Jan 5, 2024
Merged
Conversation
...src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantSortColumns.java
Outdated
Show resolved
Hide resolved
ZacBlanco
reviewed
Nov 15, 2023
...src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantSortColumns.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantTopNColumns.java
Outdated
Show resolved
Hide resolved
9daeaa6 to
4eef112
Compare
4eef112 to
0dfd436
Compare
vivek-bharathan
requested changes
Dec 12, 2023
presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/properties/Key.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/facebook/presto/sql/planner/iterative/properties/LogicalPropertiesImpl.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/com/facebook/presto/spi/plan/LogicalProperties.java
Outdated
Show resolved
Hide resolved
ZacBlanco
reviewed
Dec 12, 2023
...est/java/com/facebook/presto/sql/planner/iterative/rule/TestRedundantSortColumnsRemoval.java
Outdated
Show resolved
Hide resolved
ZacBlanco
reviewed
Dec 12, 2023
...est/java/com/facebook/presto/sql/planner/iterative/rule/TestRedundantSortColumnsRemoval.java
Outdated
Show resolved
Hide resolved
0dfd436 to
3f316b4
Compare
vivek-bharathan
approved these changes
Dec 14, 2023
...src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantSortColumns.java
Outdated
Show resolved
Hide resolved
ZacBlanco
approved these changes
Dec 16, 2023
3f316b4 to
1b670d4
Compare
feilong-liu
reviewed
Dec 20, 2023
Contributor
feilong-liu
left a comment
There was a problem hiding this comment.
[TODO] : Add AbstractTest to show correctness is not impacted
Why is it a TODO but not part of this PR?
...src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantTopNColumns.java
Outdated
Show resolved
Hide resolved
If logical properties from the constraint framework tell us that a unique constraint exists for a prefix of the OrderBy clause we can drop the extra un-needed sort columns from this clause
1b670d4 to
537c038
Compare
feilong-liu
approved these changes
Jan 4, 2024
tdcmeehan
approved these changes
Jan 5, 2024
64 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Use the logical properties from constraint framework to remove redundant sort columns
Motivation and Context
Queries can have GROUP + ORDER BY columns that are redundant. Example from TPCDS Q43 :
Impact
When we run with the constraints framework switched on (
SET SESSION exploit_constraints=true), we see redundant columns removed, see examples below -TPCDS Q43 :
Query has -
Orig Plan (relevant portion) -
Orderings are :
(s_store_name ASC_NULLS_LAST, s_store_id ASC_NULLS_LAST, sum ASC_NULLS_LAST, sum_14 ASC_NULLS_LAST, sum_15 ASC_NULLS_LAST, sum_16 ASC_NULLS_LAST, sum_17 ASC_NULLS_LAST, sum_18 ASC_NULLS_LAST, sum_19 ASC_NULLS_LAST)With this rewrite, we see that the plan changes to :
Orderings are :
(s_store_name ASC_NULLS_LAST, s_store_id ASC_NULLS_LAST)TPCDS Q3 :
Query has :
No redundant columns here, no impact on sorting observed
Testing
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.