-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Performance: use IsSingleShard() check in pushDerived instead of just engine.EqualUnique opcode
#18974
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.
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
Performance: use IsSingleShard() check in pushDerived instead of just engine.EqualUnique opcode
#18974
Conversation
Signed-off-by: ChaitanyaD48 <chaitanya.d48@gmail.com>
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
|
|
Can you also add test cases that show the impact of this change? |
Hi @arthurschreiber [
{
"comment": "Derived table pushdown optimization",
"query": "SELECT * FROM (SELECT DISTINCT col FROM user.ref WHERE col = 1 LIMIT 10) as sub",
"plan": {
"Type": "Passthrough",
"QueryType": "SELECT",
"Original": "SELECT * FROM (SELECT DISTINCT col FROM user.ref WHERE col = 1 LIMIT 10) as sub",
"Instructions": {
"OperatorType": "Route",
"Variant": "Reference",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select col from (select col from ref where 1 != 1) as sub where 1 != 1",
"Query": "select col from (select distinct col from ref where col = 1 limit 10) as sub"
},
"TablesUsed": [
"user.ref"
]
}
}
]Properties of the query -
These are the test results in both scenariosPrevious Case (EqualUnique-only):
Refactored Case (using IsSharded):
What do you think? |
|
Hi @arthurschreiber just following up on my previous comment. |
|
@ChaitanyaD48 The test case makes sense to me. 👍 |
…erived-use-isSingleShard Signed-off-by: ChaitanyaD48 <chaitanya.d48@gmail.com>
Signed-off-by: ChaitanyaD48 <chaitanya.d48@gmail.com>
|
Thanks! @arthurschreiber |
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
…erived-use-isSingleShard
…erived-use-isSingleShard
Signed-off-by: ChaitanyaD48 <chaitanya.d48@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #18974 +/- ##
=======================================
Coverage 69.91% 69.91%
=======================================
Files 1611 1611
Lines 216178 216178
=======================================
+ Hits 151134 151142 +8
+ Misses 65044 65036 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mattlord
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ChaitanyaD48 ! ❤️


Description
This PR enhances the
pushDerivedfunction inroute_planning.goto consider all single-shard routing scenarios, not just those using theengine.EqualUniqueopcode.IsSingleShard()method checks if a route is a single shard route by looking at its opcode. The method returnstruefor the following opcodes:Unsharded,DBA,Next,EqualUnique, andReference.Reference -
vitess/go/vt/vtgate/engine/routing.goRelated Issue(s)
Fixes #18952
Checklist
Deployment Notes
AI Disclosure