-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Explain Statement plan improvement #14928
Conversation
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
|
…hich contains the table Signed-off-by: Harshit Gangal <harshit@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>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
7bf964b
to
888e373
Compare
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #14928 +/- ##
==========================================
+ Coverage 47.38% 47.39% +0.01%
==========================================
Files 1145 1145
Lines 239155 239185 +30
==========================================
+ Hits 113322 113372 +50
+ Misses 117249 117237 -12
+ Partials 8584 8576 -8 ☔ View full report in Codecov by Sentry. |
Docs PR: vitessio/website#1669 |
"comment": "vexplain all dml without any directive should fail", | ||
"query": "vexplain all delete from user", | ||
"plan": "VT09008: vexplain queries/all will actually run queries" |
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.
It is not clear to me if vtexplain all
or just vtexplain
queries, will actually run queries
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.
both of them will. only vexplain plan
does not run it.
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.
Should we add end-to-end tests for this?
Otherwise, it all looks good to me
Description
In order to support
EXPLAIN
on more queries, we need to do a little bit of table name massaging before sending down theEXPLAIN
query to MySQL. Specifically, we need to handle routed tables correctly, since they don't have the same name in Vitess as they do in the underlying MySQL instance.Related Issue(s)
Fixes #14961
Checklist