drop sorts in cross joins and inner joins #3117
Conversation
jycor
changed the title
sql/analyzer/replace_sort.go
Outdated
| if errRight != nil { | ||
| return nil, transform.SameTree, errRight | ||
| } | ||
| // Nothing replaced, so nothing to do |
There was a problem hiding this comment.
Can we use a more specific comment, like "Neither child was converted to an IndexedTableAccess, so we can't remove the sort node"?
sql/analyzer/replace_sort.go
Outdated
| return nil, transform.SameTree, errRight | ||
| } | ||
| // Nothing replaced, so nothing to do | ||
| if sameLeft && sameRight { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That makes sense; I'll change the variable names
sql/analyzer/replace_sort.go
Outdated
| 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. |
There was a problem hiding this comment.
Why are these comments added here, in the part where we swap the left and right nodes?
There was a problem hiding this comment.
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
This PR applies the sort removal optimizations over cross and inner joins.