executor, planner: improve plan replayer dump URL output (#69439)#69554
Open
ti-chi-bot wants to merge 2 commits into
Open
executor, planner: improve plan replayer dump URL output (#69439)#69554ti-chi-bot wants to merge 2 commits into
ti-chi-bot wants to merge 2 commits into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
13 tasks
Member
Author
|
@zeminzhou This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughThis PR adds a named presign expiration constant, reformats ChangesPlan Replayer Presigned URL Output
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/server/handler/optimizor/plan_replayer_test.go (1)
87-114: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the helper consumed the only expected row.
These helpers close
*sql.Rowsright after the first scan. SincePLAN REPLAYER DUMPnow has both one-row and multi-row result shapes, the local-token tests will still pass if that path accidentally starts returning extra rows. Add an EOF check before closing so the helper keeps enforcing the single-row contract.Suggested change
func requirePlanReplayerFileTokenFromRows(t *testing.T, rows *sql.Rows) string { require.True(t, rows.Next(), "unexpected data") var item, filename string require.NoError(t, rows.Scan(&item, &filename)) require.Equal(t, "File token", item) require.NotEmpty(t, filename) + require.False(t, rows.Next(), "unexpected extra row") + require.NoError(t, rows.Err()) require.NoError(t, rows.Close()) return filename } @@ func requireSingleStringFromRows(t *testing.T, rows *sql.Rows) string { require.True(t, rows.Next(), "unexpected data") var value string require.NoError(t, rows.Scan(&value)) + require.False(t, rows.Next(), "unexpected extra row") + require.NoError(t, rows.Err()) require.NoError(t, rows.Close()) return value }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/handler/optimizor/plan_replayer_test.go` around lines 87 - 114, The row-checking helpers in requirePlanReplayerFileTokenFromRows, requirePlanReplayerFileTokenFromResult, and requireSingleStringFromRows only verify the first scanned row and then close, so they do not enforce the expected single-row contract. Update these helpers to assert that no additional rows remain after the first scan by checking for EOF/false on the next row before closing, keeping the tests strict for PLAN REPLAYER DUMP’s local-token path even if extra rows are returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/executor/plan_replayer.go`:
- Around line 24-28: Resolve the leftover merge-conflict markers in the import
block of plan_replayer.go so the file parses again. Remove the <<<<<<< / =======
/ >>>>>>> markers and keep the intended imports from the PlanReplayer-related
code path, specifically the net/url and os additions if they are required by the
surrounding plan replayer logic. Verify the import section is a valid single
block with no conflict artifacts.
In `@pkg/executor/test/planreplayer/BUILD.bazel`:
- Around line 11-15: Resolve the merge conflict in the go_test target for the
planreplayer BUILD rule by removing the conflict markers and keeping a single
shard_count assignment in the relevant build stanza. Use the existing go_test
target in BUILD.bazel to locate the conflict, choose the intended shard_count
value, and ensure the file is valid Bazel syntax with no leftover HEAD or commit
markers.
In `@pkg/executor/test/planreplayer/plan_replayer_test.go`:
- Around line 250-315: Resolve the unresolved merge conflict in
plan_replayer_test.go by removing the conflict markers and keeping the intended
test additions. Ensure both TestExplainExploreReplayer and
TestPlanReplayerDumpPresignedURLOutput are cleanly integrated into the test file
with no leftover HEAD/branch markers so the package compiles and the new plan
replayer tests run.
---
Nitpick comments:
In `@pkg/server/handler/optimizor/plan_replayer_test.go`:
- Around line 87-114: The row-checking helpers in
requirePlanReplayerFileTokenFromRows, requirePlanReplayerFileTokenFromResult,
and requireSingleStringFromRows only verify the first scanned row and then
close, so they do not enforce the expected single-row contract. Update these
helpers to assert that no additional rows remain after the first scan by
checking for EOF/false on the next row before closing, keeping the tests strict
for PLAN REPLAYER DUMP’s local-token path even if extra rows are returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f783a14a-c0d1-4ebe-9c09-a3e18d9a72ba
📒 Files selected for processing (6)
pkg/domain/plan_replayer_dump.gopkg/executor/plan_replayer.gopkg/executor/test/planreplayer/BUILD.bazelpkg/executor/test/planreplayer/plan_replayer_test.gopkg/planner/core/planbuilder.gopkg/server/handler/optimizor/plan_replayer_test.go
Comment on lines
+250
to
+315
| <<<<<<< HEAD | ||
| ======= | ||
| func TestExplainExploreReplayer(t *testing.T) { | ||
| ctx := context.Background() | ||
| tempDir := t.TempDir() | ||
| storage, err := extstore.NewExtStorage(ctx, "file://"+tempDir, "") | ||
| require.NoError(t, err) | ||
| extstore.SetGlobalExtStorageForTest(storage) | ||
| defer func() { | ||
| extstore.SetGlobalExtStorageForTest(nil) | ||
| storage.Close() | ||
| }() | ||
|
|
||
| store := testkit.CreateMockStore(t) | ||
| tk := testkit.NewTestKit(t, store) | ||
| tk.MustExec("use test") | ||
| tk.MustExec("create table t_explain_explore_replayer(a int, b int, key(a))") | ||
| tk.MustExec("insert into t_explain_explore_replayer values (1, 1), (2, 2), (3, 1)") | ||
| tk.MustExec("analyze table t_explain_explore_replayer") | ||
| tk.MustExec("create global binding using select * from test.t_explain_explore_replayer where b=1") | ||
| res := tk.MustQuery("plan replayer dump explain select * from test.t_explain_explore_replayer where b=1") | ||
| fileName := requirePlanReplayerFileToken(t, res.Rows()) | ||
|
|
||
| loadStore := testkit.CreateMockStore(t) | ||
| loadTK := testkit.NewTestKit(t, loadStore) | ||
| replayerPath := filepath.Join(tempDir, replayer.GetPlanReplayerDirName(), fileName) | ||
| replayerPath = strings.ReplaceAll(replayerPath, "'", "''") | ||
| for range 2 { | ||
| rows := loadTK.MustQuery(fmt.Sprintf("explain explore replayer '%s'", replayerPath)).Rows() | ||
| require.NotEmpty(t, rows) | ||
| for _, row := range rows { | ||
| require.NotEmpty(t, row[3]) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestPlanReplayerDumpPresignedURLOutput(t *testing.T) { | ||
| ctx := context.Background() | ||
| tempDir := t.TempDir() | ||
| storage, err := extstore.NewExtStorage(ctx, "file://"+tempDir, "") | ||
| require.NoError(t, err) | ||
| const presignedURL = "https://example.com/replayer.zip?X-Amz-Expires=3600&X-Amz-Signature=test" | ||
| extstore.SetGlobalExtStorageForTest(planReplayerPresignStorage{ | ||
| Storage: storage, | ||
| url: presignedURL, | ||
| }) | ||
| defer func() { | ||
| extstore.SetGlobalExtStorageForTest(nil) | ||
| storage.Close() | ||
| }() | ||
|
|
||
| store := testkit.CreateMockStore(t) | ||
| tk := testkit.NewTestKit(t, store) | ||
| tk.MustExec("use test") | ||
| tk.MustExec("create table t_presign(a int)") | ||
| tk.MustQuery("plan replayer dump explain select * from t_presign").Check(testkit.RowsWithSep("|", | ||
| "Download URL|"+presignedURL, | ||
| "Expires in|1h0m0s", | ||
| "Browser|Open the Download URL directly before it expires", | ||
| "curl|curl -L '"+presignedURL+"' -o plan_replayer.zip", | ||
| "Note|If the URL expires, rerun PLAN REPLAYER DUMP to get a new one", | ||
| )) | ||
| require.Equal(t, presignedURL, tk.Session().GetSessionVars().LastPlanReplayerToken) | ||
| } | ||
|
|
||
| >>>>>>> 693e52cd51b (executor, planner: improve plan replayer dump URL output (#69439)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the merge-conflict block before merging.
Lines 250-315 still contain unresolved conflict markers, so this test file cannot compile and neither of the new test cases will run until the conflict is resolved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/executor/test/planreplayer/plan_replayer_test.go` around lines 250 - 315,
Resolve the unresolved merge conflict in plan_replayer_test.go by removing the
conflict markers and keeping the intended test additions. Ensure both
TestExplainExploreReplayer and TestPlanReplayerDumpPresignedURLOutput are
cleanly integrated into the test file with no leftover HEAD/branch markers so
the package compiles and the new plan replayer tests run.
Contributor
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
Contributor
D3Hunter
reviewed
Jul 1, 2026
D3Hunter
left a comment
D3Hunter
left a comment
Contributor
There was a problem hiding this comment.
why cp to 2603, I thought this is used for starter?
Contributor
for preminue |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This is an automated cherry-pick of #69439
What problem does this PR solve?
Issue Number: close #69438
Problem Summary:
In TiDB Cloud,
PLAN REPLAYER DUMPmay return a presigned download URL, but the old result only showed it asFile_token. Users were not told that the URL can be opened directly, used with curl, or that it expires after 1 hour.What changed and how does it work?
This PR changes the
PLAN REPLAYER DUMPresult to a two-columnItem/Valueoutput.When the dump token is an HTTP(S) presigned URL, TiDB returns:
PLAN REPLAYER DUMPafter expirationWhen the token is not a URL, for example local storage, TiDB still returns the generated file token in the new table format. The
@@tidb_last_plan_replayer_tokensession variable keeps its original raw token value.Check List
Tests
Unit test commands:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
plan replayer dump explainoutput to distinguish raw file tokens from presigned download URLs, showing download, expiration, browser guidance, and acurlcommand when applicable.Bug Fixes
Tests