planner,executor,variable: warn and deprecate analyze v1 usage#66584
planner,executor,variable: warn and deprecate analyze v1 usage#66584ti-chi-bot[bot] merged 7 commits intopingcap:release-8.5from
Conversation
ti-chi-bot
bot
added
do-not-merge/cherry-pick-not-approved
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEmit a deprecation warning whenever ANALYZE runs with Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
pkg/sessionctx/variable/varsutil_test.go (1)
426-436: Guard warning-slice indexing with an explicit length assertion.At Line 426 and Line 435,
GetWarnings()[0]is read directly. Addrequire.Len(...)first for clearer failures.💡 Suggested tweak
err = v.SetSystemVar(TiDBAnalyzeVersion, "1") require.NoError(t, err) +require.Len(t, v.StmtCtx.GetWarnings(), 1) warn := v.StmtCtx.GetWarnings()[0] require.Error(t, warn.Err) require.Equal(t, "[variable:1287]'tidb_analyze_version=1' is deprecated and will be removed in a future release. Please use tidb_analyze_version=2 instead", warn.Err.Error()) @@ err = v.SetSystemVar(TiDBAnalyzeVersion, "4") require.NoError(t, err) // converts to max value +require.Len(t, v.StmtCtx.GetWarnings(), 1) warn = v.StmtCtx.GetWarnings()[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sessionctx/variable/varsutil_test.go` around lines 426 - 436, The test reads v.StmtCtx.GetWarnings()[0] without checking length; add explicit length assertions before each indexing to avoid panics: call require.Len(t, v.StmtCtx.GetWarnings(), 1) (or require.Greater/NotEmpty as appropriate) immediately before the first warn := v.StmtCtx.GetWarnings()[0] and again before the second warn := v.StmtCtx.GetWarnings()[0] after SetSystemVar(TiDBAnalyzeVersion, "4"); keep existing checks (require.Error/Equal) unchanged and ensure TruncateWarnings(0) still verifies zero length with require.Len(..., 0).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/sessionctx/variable/varsutil_test.go`:
- Around line 426-436: The test reads v.StmtCtx.GetWarnings()[0] without
checking length; add explicit length assertions before each indexing to avoid
panics: call require.Len(t, v.StmtCtx.GetWarnings(), 1) (or
require.Greater/NotEmpty as appropriate) immediately before the first warn :=
v.StmtCtx.GetWarnings()[0] and again before the second warn :=
v.StmtCtx.GetWarnings()[0] after SetSystemVar(TiDBAnalyzeVersion, "4"); keep
existing checks (require.Error/Equal) unchanged and ensure TruncateWarnings(0)
still verifies zero length with require.Len(..., 0).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/executor/test/analyzetest/analyze_test.gopkg/planner/core/planbuilder.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/varsutil_test.go
Signed-off-by: 0xPoe <techregister@pm.me>
Signed-off-by: 0xPoe <techregister@pm.me>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #66584 +/- ##
================================================
Coverage ? 54.9992%
================================================
Files ? 1818
Lines ? 652289
Branches ? 0
================================================
Hits ? 358754
Misses ? 267021
Partials ? 26514
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: 0xPoe <techregister@pm.me>
Signed-off-by: 0xPoe <techregister@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/statistics/handle/handletest/lockstats/lock_table_stats_test.go`:
- Around line 216-219: The assertions that check SQL warnings in the tests that
call setupTestEnvironmentWithTableT() must include the deprecation warning 1681
in addition to the skip-analyze warning 1105; locate the tk.MustQuery("show
warnings").Check(...) calls in lock_table_stats_test.go that follow analyze
table invocations (those using setupTestEnvironmentWithTableT()) and update
their expected rows to include both "Warning 1681 ANALYZE with
tidb_analyze_version=1 is deprecated..." and "Warning 1105 skip analyze locked
tables: ..." in the same order as the other test block (matching the Check used
in the block that already includes both warnings).
| tk.MustQuery("show warnings").Check(testkit.Rows( | ||
| "Warning 1681 ANALYZE with tidb_analyze_version=1 is deprecated and will be removed in a future release.", | ||
| "Warning 1105 skip analyze locked tables: test.t1, test.t2", | ||
| )) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all warning assertions in the test file to check for inconsistencies
rg -n "show warnings.*Check" pkg/statistics/handle/handletest/lockstats/lock_table_stats_test.go -A 3Repository: pingcap/tidb
Length of output: 1044
🏁 Script executed:
#!/bin/bash
# Check the setup and analyze calls in the first two test functions
sed -n '45,60p' pkg/statistics/handle/handletest/lockstats/lock_table_stats_test.go
sed -n '95,115p' pkg/statistics/handle/handletest/lockstats/lock_table_stats_test.go
# Also check the test function that has the new warning (to see its setup)
sed -n '168,225p' pkg/statistics/handle/handletest/lockstats/lock_table_stats_test.goRepository: pingcap/tidb
Length of output: 3667
🏁 Script executed:
#!/bin/bash
# Find the setupTestEnvironmentWithTableT function definition
rg -n "func setupTestEnvironmentWithTableT" pkg/statistics/handle/handletest/lockstats/lock_table_stats_test.go -A 30Repository: pingcap/tidb
Length of output: 699
Other tests using tidb_analyze_version=1 should also expect the deprecation warning.
Tests at lines 54-56 and 104-106 both:
- Use
setupTestEnvironmentWithTableT()which sets@@tidb_analyze_version = 1(line 372) - Execute
analyze tablecommands on locked tables - Check the resulting warnings
These tests currently only expect Warning 1105 (skip analyze locked tables), but since they analyze with version 1, they should also expect Warning 1681 (deprecation warning) to match the expectation at lines 216-219. Update these assertions to include the deprecation warning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/statistics/handle/handletest/lockstats/lock_table_stats_test.go` around
lines 216 - 219, The assertions that check SQL warnings in the tests that call
setupTestEnvironmentWithTableT() must include the deprecation warning 1681 in
addition to the skip-analyze warning 1105; locate the tk.MustQuery("show
warnings").Check(...) calls in lock_table_stats_test.go that follow analyze
table invocations (those using setupTestEnvironmentWithTableT()) and update
their expected rows to include both "Warning 1681 ANALYZE with
tidb_analyze_version=1 is deprecated..." and "Warning 1105 skip analyze locked
tables: ..." in the same order as the other test block (matching the Check used
in the block that already includes both warnings).
Signed-off-by: 0xPoe <techregister@pm.me>
ti-chi-bot
bot
added
size/L
|
/retest |
0xPoe
left a comment
0xPoe
left a comment
There was a problem hiding this comment.
🔢 Self-check (PR reviewed by myself and ready for feedback)
-
Code compiles successfully
-
Unit tests added
-
No AI-generated elegant nonsense in PR.
-
Bazel files updated
-
Comments added where necessary
-
PR title and description updated
-
Documentation PR created (or confirmed not needed)
-
PR size is reasonable
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fixdb, terry1purcell, 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 |
ti-chi-bot
bot
added
cherry-pick-approved
|
/retest |
4 participants
What problem does this PR solve?
Issue Number: ref #63579
Problem Summary:
tidb_analyze_version=1deprecation was not clearly surfaced when runningANALYZE.What changed and how does it work?
tidb_analyze_version=1.ANALYZEruns with effective analyze version 1.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit