Prevent Early Ordering Pushdown to Enable Aggregation Pushdown to MySQL #16278
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
systay
requested review from
harshit-gangal,
frouioui,
GuptaManan100 and
arthurschreiber
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
systay
added
Type: Enhancement
systay
changed the title
systay
removed
NeedsDescriptionUpdate
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16278 +/- ##
==========================================
- Coverage 68.71% 68.71% -0.01%
==========================================
Files 1547 1547
Lines 198290 198286 -4
==========================================
- Hits 136248 136244 -4
Misses 62042 62042 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
systay
commented
Jul 2, 2024
| @@ -369,14 +369,14 @@ func pushAggregationThroughApplyJoin(ctx *plancontext.PlanningContext, rootAggr | |||
|
|
|||
| columns := &applyJoinColumns{} | |||
| output, err := splitAggrColumnsToLeftAndRight(ctx, rootAggr, join, !join.JoinType.IsInner(), columns, lhs, rhs) | |||
| join.JoinColumns = columns | |||
There was a problem hiding this comment.
This bug was not related to the rest of the PR, but it caused tests to fail, so it had to go.
systay
commented
Jul 2, 2024
| } | ||
|
|
||
| col.GroupBy = addToGroupBy |
There was a problem hiding this comment.
This is another bug that was not related to the issue being found, but it caused some plans to be missing group by expressions
GuptaManan100
approved these changes
Jul 2, 2024
| @@ -2108,9 +2108,9 @@ | |||
| "Name": "user", | |||
| "Sharded": true | |||
| }, | |||
| "FieldQuery": "select min(a.id), a.tcol1, weight_string(a.tcol1), weight_string(a.id) from `user` as a where 1 != 1 group by a.tcol1, weight_string(a.tcol1), weight_string(a.id)", | |||
There was a problem hiding this comment.
Not just an optimization, the previous plan also looks wrong. It should have been selecting weight_string(min(a.id)) and not weight_string(a.id)
Comment on lines
-2188
to
2191
| "FieldQuery": "select foo, col, weight_string(foo) from `user` where 1 != 1 group by col, foo, weight_string(foo)", | ||
| "FieldQuery": "select foo, col, weight_string(foo) from `user` where 1 != 1 group by foo, col, weight_string(foo)", | ||
| "OrderBy": "1 ASC, (0|2) ASC", | ||
| "Query": "select foo, col, weight_string(foo) from `user` where id between :vtg1 and :vtg2 group by col, foo, weight_string(foo) order by `user`.col asc, foo asc", | ||
| "Query": "select foo, col, weight_string(foo) from `user` where id between :vtg1 and :vtg2 group by foo, col, weight_string(foo) order by `user`.col asc, foo asc", | ||
| "Table": "`user`" |
There was a problem hiding this comment.
there is no impact on the output by this change.
I added a plan to the bottom of That query produces a very different plan on {
"QueryType": "SELECT",
"Original": "select sum(user.type) from user join user_extra on user.team_id = user_extra.id group by user_extra.col order by user_extra.col",
"Instructions": {
"OperatorType": "Aggregate",
"Variant": "Ordered",
"Aggregates": "sum(0) AS sum(`user`.type)",
"GroupBy": "1",
"ResultColumns": 1,
"Inputs": [
{
"OperatorType": "Sort",
"Variant": "Memory",
"OrderBy": "1 ASC",
"Inputs": [
{
"OperatorType": "Join",
"Variant": "Join",
"JoinColumnIndexes": "L:0,R:0",
"JoinVars": {
"user_team_id": 1
},
"TableName": "`user`_user_extra",
"Inputs": [
{
"OperatorType": "Route",
"Variant": "Scatter",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select `user`.type, `user`.team_id from `user` where 1 != 1",
"Query": "select `user`.type, `user`.team_id from `user`",
"Table": "`user`"
},
{
"OperatorType": "Route",
"Variant": "Scatter",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select user_extra.col from user_extra where 1 != 1",
"Query": "select user_extra.col from user_extra where user_extra.id = :user_team_id",
"Table": "user_extra"
}
]
}
]
}
]
},
"TablesUsed": [
"user.user",
"user.user_extra"
]
}As you can see here, MySQL is not doing any aggregation at all. |
Signed-off-by: Andres Taylor <andres@planetscale.com>
|
| @@ -5324,8 +5324,8 @@ | |||
| "Name": "user", | |||
| "Sharded": true | |||
| }, | |||
| "FieldQuery": "select count(*), :user_id + user_extra.id, weight_string(:user_id + user_extra.id) from user_extra where 1 != 1 group by :user_id + user_extra.id", | |||
| "Query": "select count(*), :user_id + user_extra.id, weight_string(:user_id + user_extra.id) from user_extra group by :user_id + user_extra.id", | |||
| "FieldQuery": "select count(*), :user_id + user_extra.id, weight_string(:user_id + user_extra.id) from user_extra where 1 != 1 group by :user_id + user_extra.id, weight_string(:user_id + user_extra.id)", | |||
There was a problem hiding this comment.
Does the additional grouping by weight string make sense? 🤔 If so, can you explain why?
There was a problem hiding this comment.
no, they don't really make sense
There was a problem hiding this comment.
They are needed for some specific cases, we have CI tests which are failing due to removal of weight_string changes.
frouioui
reviewed
Jul 2, 2024
| { | ||
| "OperatorType": "Projection", | ||
| "Expressions": [ | ||
| "sum(volume) * count(*) as revenue", |
There was a problem hiding this comment.
can you explain why we are not passing volume as an index, like we do for the other columns?
There was a problem hiding this comment.
we are, the printing of the plan is just not showing it
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
This reverts commit f7c0ef7. Turns out we do need them. Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
harshit-gangal
approved these changes
Jul 4, 2024
Signed-off-by: Andres Taylor <andres@planetscale.com>
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
This PR introduces a crucial check in the
pushOrderingUnderAggr method to prevent the premature pushdown of theORDER BYclause during the query planning process. By ensuring that the ordering is not pushed down too early, we allow the aggregation to be effectively pushed down to MySQL, optimizing query execution.Details:
Added a Check for Planning Phase:
Rationale:
Optimization:
Impact:
Related Issue(s)
Issue: #16279
Checklist