Do not add redundant subquery ordering into plan #30
Conversation
This file contains 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
mertak-synnada
commented
Aug 6, 2024
ozankabak
reviewed
Aug 6, 2024
…ting' into feature/Sort-For-Subqueries # Conflicts: # datafusion/sqllogictest/test_files/window.slt
…ting' into feature/Sort-For-Subqueries
…ting' into feature/Sort-For-Subqueries
remove unnecessary limit plans when used with sort + fetch add test case for Sort and Limit with offset push down limit even if a child with no fetch appears when the child supports push down
Co-authored-by: Mehmet Ozan Kabak <[email protected]>
…ting' into feature/Sort-For-Subqueries # Conflicts: # datafusion/sqllogictest/test_files/aggregates_topk.slt
…eature/Sort-For-Subqueries # Conflicts: # datafusion/core/src/physical_optimizer/enforce_distribution.rs # datafusion/core/src/physical_optimizer/enforce_sorting.rs # datafusion/core/src/physical_optimizer/limit_pushdown.rs # datafusion/core/src/physical_optimizer/sort_pushdown.rs # datafusion/physical-plan/src/coalesce_batches.rs # datafusion/physical-plan/src/limit.rs # datafusion/physical-plan/src/sorts/sort.rs # datafusion/sqllogictest/test_files/order.slt
add pushes_global_limit_into_multiple_fetch_plans test case change limit_pushdown.rs as manual top down operator and simplify algorithm by supporting most parent node remove and other pushdown cases
ozankabak
reviewed
Aug 14, 2024
Comment on lines
265
to
267
| // UnionExec is supporting limit_pushdown because it's only applicable with | ||
| // SortPreservingMergeExec plan. Since SortExec does not support limit_pushdowm | ||
| // it's not possible to lose sorting in UnionExec's children. |
There was a problem hiding this comment.
I don't understand what this is trying to say
There was a problem hiding this comment.
We discussed adding the supports_limit_pushdown method to UnionExec or not. We agreed to do so because LimitPushDown is only applicable to SPM in UnionExec's possible children. That's why I put this comment actually, maybe we can put a better comment I don't know
# Conflicts: # datafusion/sqllogictest/test_files/window.slt
|
I am going to merge this today unless there are any concerns raised. We will then proceed with |
|
Merged upstream. |
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.
Which issue does this PR close?
Rationale for this change
This PR solves an issue with the subquery sortings. If a main query does not support the same sorting with the subquery and the subquery has no limit, then the ordering is redundant (unless used in an Aggregation but it's being handled in a different plan type anyway).
While implementing this behavior we discovered that it's only possible to use
WITH ORDERsyntax withEXTERNALtables. That's why we needed to addLIMIT xxconditions to subqueries so that the system can understand they're valid orderings. The sql-parser issue will be handled in a different PR to the sql-parser project and those tests will be changed back to their original formsOther than that, we discovered there are some redundant LimitExec plans even if the child plan has fetch support. Also, fixed this issue and replaced
GlobalLimitExecplans withskip=0withLocalLimitExecplan type.Also added Limit push down support to the
UnionExecplan type.What changes are included in this PR?
Explained above
Are these changes tested?
Yes
Are there any user-facing changes?
No