planner: implements support for dumping multiple SQL statements in a single PLAN REPLAYER DUMP EXPLAIN command#66162
Conversation
|
This PR adds support for dumping multiple SQL statements in a single
|
|
Hi @qw4990. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@pantheon-bot please review |
✅ Review CompleteI've completed a comprehensive review of PR #66162 (planner: implements support for dumping multiple SQL statements in a single PLAN REPLAYER DUMP EXPLAIN command). Review Summary
Key FindingsP2: Misleading error for multi-SQL parse failures
👉 View detailed review comments Overall Assessment✅ No blocking issues found. The implementation correctly adds multi-SQL support to PLAN REPLAYER DUMP EXPLAIN with proper backward compatibility. The P2 issue is a usability improvement that can be addressed in a follow-up. |
|
/test mysql-test |
|
@qw4990: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66162 +/- ##
================================================
+ Coverage 77.7712% 79.6517% +1.8804%
================================================
Files 2001 1952 -49
Lines 546334 538542 -7792
================================================
+ Hits 424891 428958 +4067
+ Misses 119783 108102 -11681
+ Partials 1660 1482 -178
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/ok-to-test |
|
/retest |
| for i := 1; i <= numTables; i++ { | ||
| tableName := fmt.Sprintf("t_dump_multi_%d", i) | ||
| require.Contains(t, names, fmt.Sprintf("stats/test.%s.json", tableName)) | ||
| require.Contains(t, names, fmt.Sprintf("schema/test.%s.schema.txt", tableName)) |
There was a problem hiding this comment.
The tests are a bit thin; can we have AI generate some examples with multiple databases?
There was a problem hiding this comment.
Added more databases.
| ctx.WritePlain(")") | ||
| return nil | ||
| } | ||
| if n.Stmt == nil { |
There was a problem hiding this comment.
question: When will n.Stmt is nil
| {"plan replayer dump explain ('SELECT * FROM t1', 'SELECT * FROM t2')", | ||
| "PLAN REPLAYER DUMP EXPLAIN ('SELECT * FROM t1', 'SELECT * FROM t2')"}, | ||
| {"plan replayer dump explain analyze ('SELECT * FROM t1')", | ||
| "PLAN REPLAYER DUMP EXPLAIN ANALYZE ('SELECT * FROM t1')"}, |
There was a problem hiding this comment.
Add edge case that with empty and non empty in the stmt list?
|
/hold |
[LGTM Timeline notifier]Timeline:
|
|
feel free to unhold if you're ready to merge |
|
/unhold |
|
The doc PR: pingcap/docs#22429 |
|
/test build |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test pull-build-next-gen |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test check-dev2 |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test check-dev2 |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, guo-shaoge, hawkingrei, yudongusa The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test pull-integration-realcluster-test-next-gen |
|
@qw4990: The specified target(s) for Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |


What problem does this PR solve?
Issue Number: close #66151
Problem Summary: Implements support for dumping multiple SQL statements in a single
PLAN REPLAYER DUMP EXPLAINcommand, addressing issue #66151.What changed and how does it work?
Parser
StmtList []stringfield toPlanReplayerStmtASTPLAN REPLAYER DUMP EXPLAIN ( 'sql1', 'sql2', ... )Restoremethod to handle multi-SQL formatPlanner/Executor
StmtListfield toPlanReplayerplanStmtListinto AST nodesDump Output
sql/sql0.sql+explain.txtsql/sql0.sql,sql/sql1.sql, ... +explain/explain0.txt,explain/explain1.txt, ...Example
Output zip contains:
/sql/sql0.sql,/sql/sql1.sql,/sql/sql2.sql/explain/explain0.txt,/explain/explain1.txt,/explain/explain2.txtCheck List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.