Skip to content

drop sorts in cross joins and inner joins #3117

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jul 30, 2025
Merged

drop sorts in cross joins and inner joins #3117

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e40746c
drop sorts in cross joins
Jul 24, 2025
c913ae2
add inner joins too
Jul 28, 2025
6548216
incorporate swaps
Jul 28, 2025
a163778
tests
Jul 28, 2025
eb10185
regen
Jul 29, 2025
c286b24
tests for inner join
Jul 29, 2025
8134064
regen
Jul 29, 2025
38713ba
feedback
Jul 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
258 changes: 258 additions & 0 deletions enginetest/queries/query_plan_script_tests.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

51 changes: 31 additions & 20 deletions sql/analyzer/replace_sort.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,16 +154,16 @@ func replaceIdxSortHelper(ctx *sql.Context, scope *plan.Scope, node sql.Node, so
*plan.Project, *plan.Filter, *plan.Limit, *plan.Offset, *plan.Distinct, *plan.TableAlias:
newChildren[i], same, err = replaceIdxSortHelper(ctx, scope, child, sortNode)
case *plan.JoinNode:
// TODO: is this applicable to other types of joins?
// as long as left child is already sorted and SortFields are a prefix, then it's ok?
if !c.JoinType().IsMerge() {
continue
}
// It's (probably) not possible to have Sort as child of Join without Subquery/SubqueryAlias,
// and in the case where there is a Subq/SQA it's taken care of through finalizeSubqueries
if sortNode == nil {
continue
}
// Merge Joins assume that left and right are sorted
// Cross Joins and Inner Joins are valid for sort removal if left child is sorted
if !c.JoinType().IsMerge() && !c.JoinType().IsCross() && !c.JoinType().IsInner() {
continue
}
newLeft, sameLeft, errLeft := replaceIdxSortHelper(ctx, scope, c.Left(), sortNode)
if errLeft != nil {
return nil, transform.SameTree, errLeft
Expand All @@ -172,6 +172,7 @@ func replaceIdxSortHelper(ctx *sql.Context, scope *plan.Scope, node sql.Node, so
if errRight != nil {
return nil, transform.SameTree, errRight
}
// Nothing replaced, so nothing to do
Copy link
Contributor

@nicktobey nicktobey Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a more specific comment, like "Neither child was converted to an IndexedTableAccess, so we can't remove the sort node"?

if sameLeft && sameRight {
Copy link
Contributor

@nicktobey nicktobey Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an idea that might improve readability. What if we wrote:

leftIsSorted := !sameLeft
rightIsSorted := !sameRight

And then wrote the conditionals in terms of these? It might make it more clear what each of these branches means.

Copy link
Contributor Author

@jycor jycor Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense; I'll change the variable names

continue
}
Expand All @@ -183,24 +184,34 @@ func replaceIdxSortHelper(ctx *sql.Context, scope *plan.Scope, node sql.Node, so
c.IsReversed = isReversed
continue
}
// If only one side has been replaced, we need to check if the other side can be reversed
if (sameLeft != sameRight) && isReversed {
// If descending, then both Indexes must be reversed
if sameLeft {
newLeft, same, err = buildReverseIndexedTable(ctx, newLeft)
} else if sameRight {
newRight, same, err = buildReverseIndexedTable(ctx, newRight)
if c.JoinType().IsCross() || c.JoinType().IsInner() {
// For cross joins and inner joins, if the right child is sorted, we need to swap
if !sameRight {
// Rule eraseProjection will drop any Projections that are now unnecessary.
Copy link
Contributor

@nicktobey nicktobey Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these comments added here, in the part where we swap the left and right nodes?

Copy link
Contributor Author

@jycor jycor Jul 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swapping left and right nodes opens up the opportunity for projections to be dropped and messes up the GetField indexes. These comments are just saying that both those things are handled later on in analysis.

It may also come in handy if we ever change the order of those rules

// Rule fixExecIndexes will fix any existing Projection GetField indexes.
newLeft, newRight = newRight, newLeft
}
if err != nil {
return nil, transform.SameTree, err
} else {
// If only one side has been replaced, we need to check if the other side can be reversed
if (sameLeft != sameRight) && isReversed {
// If descending, then both Indexes must be reversed
if sameLeft {
newLeft, same, err = buildReverseIndexedTable(ctx, newLeft)
} else if sameRight {
newRight, same, err = buildReverseIndexedTable(ctx, newRight)
}
if err != nil {
return nil, transform.SameTree, err
}
// If we could not replace the IndexedTableAccess with a reversed one (due to lack of reversible index)
// same = true, so just continue
if same {
continue
}
c.IsReversed = true
}
// If we could not replace the IndexedTableAccess with a reversed one (due to lack of reversible index)
// same = true, so just continue
if same {
continue
}
c.IsReversed = true
}

newChildren[i], err = c.WithChildren(newLeft, newRight)
if err != nil {
return nil, transform.SameTree, err
Expand Down
Loading