statistics: replace separate TopN merge with combined TopN+histogram merge for global stats | tidb-test=pr/2734 - #68147
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/XXL
|
Hi @mjonss. 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. |
|
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:
📝 WalkthroughWalkthroughReplaces the two-step partition TopN + histogram global-merge with a single combined MergePartTopNAndHistToGlobal, removes the concurrency-based TopN worker machinery and related APIs, deprecates tidb_merge_partition_stats_concurrency (behaviorless), updates session/sysvar fields and tests, and adjusts BUILD/bazel deps and benchmarks/fuzzing. ChangesGlobal stats merge rewrite
Sysvar/session and test wiring
Build / deps / Bazel
Sequence Diagram(s)sequenceDiagram
participant Analyzer as ANALYZE
participant MergeMgr as MergePartitionStats2GlobalStats
participant CPU as CPU Worker
participant StatsPkg as statistics.MergePartTopNAndHistToGlobal
participant Killer as SQLKiller
Analyzer->>MergeMgr: trigger partition stats merge
MergeMgr->>CPU: schedule CPU-side merge (pass stmtCtx & killer)
CPU->>StatsPkg: call MergePartTopNAndHistToGlobal(topNs, hists, ..., killer, stmtCtx)
StatsPkg->>Killer: killer.HandleSignal() (periodic cancellation checks)
StatsPkg-->>CPU: merged TopN + global Histogram
CPU-->>MergeMgr: aggregated global stats
MergeMgr-->>Analyzer: store global stats (SHOW STATS updated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (4)
pkg/statistics/handle/handletest/analyze/analyze_test.go (1)
236-241: ⚡ Quick winAssert the bucket bounds you describe.
The new comment says the merge preserves separate
[1,3]and[10,11]buckets, but the test only checkslen(rows) == 2and the final cumulative count. A wrong two-bucket layout would still pass; please assert thelower/uppercolumns too.Suggested tightening
rows = tk.MustQuery("show stats_buckets where partition_name = 'global' and is_index=1").Rows() // The combined merge keeps the natural gap between p0's [1-3] cluster // and p1's [10-11] cluster as two separate buckets instead of // collapsing them into a single wider [1-11] bucket. Total row count // is preserved (cumulative=6 at the last bucket). require.Len(t, rows, 2) +require.Equal(t, "1", rows[0][8]) +require.Equal(t, "3", rows[0][9]) +require.Equal(t, "10", rows[1][8]) +require.Equal(t, "11", rows[1][9]) require.Equal(t, "6", rows[1][6])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/statistics/handle/handletest/analyze/analyze_test.go` around lines 236 - 241, The test currently only checks rows length and cumulative count but doesn't verify bucket bounds; update the assertions that reference rows to explicitly check each bucket's lower and upper bound values so they match the expected buckets [1,3] and [10,11]. Locate the test's rows variable in analyze_test.go (the require.Len and require.Equal lines) and add assertions that rows[0] has lower=="1" and upper=="3" and rows[1] has lower=="10" and upper=="11" (using the same column indices the table uses for lower/upper), keeping the existing cumulative check.pkg/statistics/handle/globalstats/global_stats_test.go (1)
1020-1030: ⚡ Quick winDon’t freeze TODO-marked side behavior into the main merge golden set.
This assertion now locks in rows the inline comments already call “useless” or questionable because of
NULLhandling. That makes this test fail on future cleanups of#66236or NULL-entry persistence even if the combined merge itself is still correct. Please keep this golden focused on the invariants this PR actually cares about, and move those edge cases into dedicated regressions if they need coverage.Based on learnings: Applies to **/*_test.go : Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/statistics/handle/globalstats/global_stats_test.go` around lines 1020 - 1030, The test is freezing TODOed/edge-case rows into the golden output in the tk.MustQuery(`show stats_topn where table_name = 't' and partition_name = 'global'`) assertion; remove the rows that are marked with TODO (the lines containing comments about "useless" or NULL handling such as the entries for "a", "idx_ab", "idx_be", "idx_ec", "uidx_cd" and any others annotated as TODO) so the golden only asserts the invariant rows this PR intends to guarantee, and if coverage is required for those edge cases create separate dedicated regression tests that exercise NULL-handling and `#66236` behaviors instead of locking them into this global_stats_test.go golden.pkg/statistics/handle/globalstats/topn_test.go (1)
45-366: ⚡ Quick winExercise the cancellation path, not just the happy path.
All of the new coverage passes a live
SQLKiller{}but only validates success or input-validation errors. Since this PR adds cancellation support toMergePartTopNAndHistToGlobal, please add one test that trips the killer and asserts the merge aborts; otherwise that new path stays untested until a long-runningANALYZEis interrupted in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/statistics/handle/globalstats/topn_test.go` around lines 45 - 366, Add a new unit test (e.g. TestMergePartTopNAndHistToGlobalCancelled) that exercises the cancellation path of MergePartTopNAndHistToGlobal: create the usual topNs/hists fixture, instantiate sqlkiller.SQLKiller, trigger cancellation (either call killer.Kill() before invoking or spawn a goroutine that calls killer.Kill() shortly after starting the merge), call MergePartTopNAndHistToGlobal with that killer, and assert the call returns an error (and does not return a valid hybridTopN/hybridHist), verifying the function properly aborts on cancellation.pkg/statistics/handle/globalstats/topn_bench_test.go (1)
36-69: ⚡ Quick winUse a fixed RNG seed for the benchmark fixtures.
Both fixture builders use the package-level RNG, so every benchmark run gets a different key/count distribution. That adds noise to cross-branch
benchstatcomparisons and can hide small regressions in the merge path.Suggested change
func prepareOverlappingTopNsAndHists(b *testing.B, partitions int, tz *time.Location) ([]*statistics.TopN, []*statistics.Histogram) { sc := stmtctx.NewStmtCtxWithTimeZone(tz) + rng := rand.New(rand.NewSource(1)) // Prepare TopNs. topNs := make([]*statistics.TopN, 0, partitions) for i := range partitions { @@ - topN.AppendTopN(key, uint64(rand.Intn(1000))) + topN.AppendTopN(key, uint64(rng.Intn(1000))) } } topNs = append(topNs, topN) } @@ func prepareSkewedTopNsAndHists(b *testing.B, partitions int, tz *time.Location) ([]*statistics.TopN, []*statistics.Histogram) { sc := stmtctx.NewStmtCtxWithTimeZone(tz) + rng := rand.New(rand.NewSource(1)) const perPart = 500 @@ - topN.AppendTopN(key, uint64(rand.Intn(1000))) + topN.AppendTopN(key, uint64(rng.Intn(1000))) } topNs = append(topNs, topN) }Based on learnings: Applies to **/*_test.go : Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required.
Also applies to: 76-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/statistics/handle/globalstats/topn_bench_test.go` around lines 36 - 69, The benchmark fixture prepareOverlappingTopNsAndHists is using the package-level RNG (rand.Intn) which makes runs nondeterministic; seed a deterministic RNG at the start of the fixture (e.g., call rand.Seed with a fixed constant) or better create a local rng := rand.New(rand.NewSource(<constant>)) and replace rand.Intn calls with rng.Intn so TopN key/count distributions are reproducible; update both loops in prepareOverlappingTopNsAndHists (the rand.Intn usage and any other random usage in the same function) to use the seeded RNG.
🤖 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/globalstats/global_stats_test.go`:
- Around line 1160-1165: The test can read stale global stats because with async
merge enabled ANALYZE returns before the background "merge global stats" job
finishes; after calling dom.StatsHandle().Update(ctx) add a short polling loop
that re-runs the SHOW STATS_TOPN and SHOW STATS_BUCKETS queries (the same
queries assigned to asyncTopN and asyncBuckets) until the results stop changing
or a timeout is reached, then proceed with the assertions—this ensures the async
merge job has completed before comparing results.
In `@pkg/statistics/histogram.go`:
- Around line 1844-1859: The split logic clears bucketLowerSet when appending a
bucket but fails to reseed bucketLower when the split point was taken from
prevUpper, losing the carried group's lower bound; inside the branch where you
compute cutUpper/cutCount/cutRepeat (the block that chooses prevUpper when
prevCumCount is closer to threshold) ensure that if cutUpper is set to
&prevUpper you reinitialize bucketLower to the carried group's lower (use the
group's stored lower value that corresponds to prevRepeat/prevUpper) and set
bucketLowerSet = true before clearing/advancing, so the next bucket's lower
covers the carried rows; this change should be applied around the
globalHist.AppendBucketWithNDV call (and uses symbols cumCount, threshold,
prevUpper, prevCumCount, prevRepeat, bucketLower, bucketLowerSet, lastUpper,
lastRepeat, bucketIdx).
---
Nitpick comments:
In `@pkg/statistics/handle/globalstats/global_stats_test.go`:
- Around line 1020-1030: The test is freezing TODOed/edge-case rows into the
golden output in the tk.MustQuery(`show stats_topn where table_name = 't' and
partition_name = 'global'`) assertion; remove the rows that are marked with TODO
(the lines containing comments about "useless" or NULL handling such as the
entries for "a", "idx_ab", "idx_be", "idx_ec", "uidx_cd" and any others
annotated as TODO) so the golden only asserts the invariant rows this PR intends
to guarantee, and if coverage is required for those edge cases create separate
dedicated regression tests that exercise NULL-handling and `#66236` behaviors
instead of locking them into this global_stats_test.go golden.
In `@pkg/statistics/handle/globalstats/topn_bench_test.go`:
- Around line 36-69: The benchmark fixture prepareOverlappingTopNsAndHists is
using the package-level RNG (rand.Intn) which makes runs nondeterministic; seed
a deterministic RNG at the start of the fixture (e.g., call rand.Seed with a
fixed constant) or better create a local rng :=
rand.New(rand.NewSource(<constant>)) and replace rand.Intn calls with rng.Intn
so TopN key/count distributions are reproducible; update both loops in
prepareOverlappingTopNsAndHists (the rand.Intn usage and any other random usage
in the same function) to use the seeded RNG.
In `@pkg/statistics/handle/globalstats/topn_test.go`:
- Around line 45-366: Add a new unit test (e.g.
TestMergePartTopNAndHistToGlobalCancelled) that exercises the cancellation path
of MergePartTopNAndHistToGlobal: create the usual topNs/hists fixture,
instantiate sqlkiller.SQLKiller, trigger cancellation (either call killer.Kill()
before invoking or spawn a goroutine that calls killer.Kill() shortly after
starting the merge), call MergePartTopNAndHistToGlobal with that killer, and
assert the call returns an error (and does not return a valid
hybridTopN/hybridHist), verifying the function properly aborts on cancellation.
In `@pkg/statistics/handle/handletest/analyze/analyze_test.go`:
- Around line 236-241: The test currently only checks rows length and cumulative
count but doesn't verify bucket bounds; update the assertions that reference
rows to explicitly check each bucket's lower and upper bound values so they
match the expected buckets [1,3] and [10,11]. Locate the test's rows variable in
analyze_test.go (the require.Len and require.Equal lines) and add assertions
that rows[0] has lower=="1" and upper=="3" and rows[1] has lower=="10" and
upper=="11" (using the same column indices the table uses for lower/upper),
keeping the existing cumulative check.
🪄 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: cf3eddfc-b090-450a-8e3f-422658774021
📒 Files selected for processing (26)
pkg/executor/analyze_test.gopkg/executor/set_test.gopkg/executor/test/analyzetest/columns/analyze_columns_with_test.gopkg/importsdk/BUILD.bazelpkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.gopkg/statistics/BUILD.bazelpkg/statistics/cmsketch_util.gopkg/statistics/handle/globalstats/BUILD.bazelpkg/statistics/handle/globalstats/global_stats.gopkg/statistics/handle/globalstats/global_stats_async.gopkg/statistics/handle/globalstats/global_stats_internal_test.gopkg/statistics/handle/globalstats/global_stats_test.gopkg/statistics/handle/globalstats/merge_worker.gopkg/statistics/handle/globalstats/topn.gopkg/statistics/handle/globalstats/topn_bench_test.gopkg/statistics/handle/globalstats/topn_test.gopkg/statistics/handle/handletest/analyze/analyze_test.gopkg/statistics/handle/handletest/handle_test.gopkg/statistics/handle/util/pool.gopkg/statistics/handle/util/util.gopkg/statistics/histogram.gopkg/statistics/histogram_bench_test.gopkg/statistics/histogram_test.gopkg/statistics/scalar.go
💤 Files with no reviewable changes (8)
- pkg/statistics/handle/util/util.go
- pkg/sessionctx/variable/session.go
- pkg/statistics/scalar.go
- pkg/statistics/histogram_bench_test.go
- pkg/statistics/handle/globalstats/topn.go
- pkg/statistics/handle/globalstats/merge_worker.go
- pkg/statistics/histogram_test.go
- pkg/statistics/cmsketch_util.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68147 +/- ##
================================================
- Coverage 76.3237% 73.6739% -2.6498%
================================================
Files 2041 2090 +49
Lines 559257 595939 +36682
================================================
+ Hits 426846 439052 +12206
- Misses 131511 155187 +23676
- Partials 900 1700 +800
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
ti-chi-bot
Bot
added
release-note
|
/retest |
|
@mjonss: 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. |
6957b80 to
6407b92
Compare
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/executor/set_test.go`:
- Around line 1506-1512: Add a global-path assertion for the deprecated
variable: after setting tidb_merge_partition_stats_concurrency and checking
session value and warnings in pkg/executor/set_test.go (the test using
tk.MustExec / tk.MustQuery), also run a SELECT
@@global.tidb_merge_partition_stats_concurrency and assert it returns "1" so the
global getter is forced to 1; place this check alongside the existing session
assertions (both after the initial set to 1 and after the set to 4/warning) to
ensure both session and global getters are validated.
🪄 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: 88dd02a2-cdc3-4a13-9886-d10bd4634af4
📒 Files selected for processing (2)
pkg/executor/analyze_test.gopkg/executor/set_test.go
6407b92 to
67637c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/statistics/handle/globalstats/global_stats_test.go (1)
1161-1166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWait for async global-merge completion before asserting global stats (flake risk).
At Line 1161 the test enables async merge and reads global TopN/buckets immediately after
ANALYZE+StatsHandle().Update(). That update refreshes cache but does not guarantee the async merge job has finished, so this comparison can be nondeterministic.Suggested guard
tk.MustExec("SET @@tidb_enable_async_merge_global_stats = ON") tk.MustExec("ANALYZE TABLE t " + analyzeOpts) +require.Eventually(t, func() bool { + rows := tk.MustQuery("show analyze status where job_info like 'merge global stats%'").Rows() + return len(rows) > 0 && rows[len(rows)-1][7] == "finished" +}, 10*time.Second, 100*time.Millisecond) require.NoError(t, dom.StatsHandle().Update(context.Background(), dom.InfoSchema()))🤖 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/statistics/handle/globalstats/global_stats_test.go` around lines 1161 - 1166, The test reads async global TopN/buckets immediately after ANALYZE and dom.StatsHandle().Update(), which risks flakiness because the background global-merge job may not have completed; modify the test (around the ANALYZE + dom.StatsHandle().Update() sequence) to wait/poll until the async merge finishes before asserting: repeatedly run the same query used for assertions (tk.MustQuery("SHOW STATS_TOPN WHERE table_name = 't' AND partition_name = 'global'") and/or tk.MustQuery("SHOW STATS_BUCKETS ...")) with a short sleep and timeout, breaking when the expected global rows appear (non-empty or match expected counts), then proceed to set asyncTopN/asyncBuckets and assertions; keep polling timeout to avoid infinite loop.
🤖 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/statistics/handle/globalstats/global_stats_test.go`:
- Around line 1187-1188: The test currently ignores errors from
strconv.ParseFloat when building partTopNMap (cnt, _ :=
strconv.ParseFloat(...)), which can mask formatting regressions; update the
assertion setup to capture the error (cnt, err := strconv.ParseFloat(...)) and
fail the test on parse error (e.g., t.Fatalf or require.NoError) with a clear
message including the raw value and key; apply the same change for the similar
occurrence around row[?] at the second site so both parse failures cause an
explicit test failure instead of defaulting to 0.
In `@pkg/statistics/histogram.go`:
- Around line 1880-1889: The lambda is swallowing codec.EncodeKey errors and
treating failures as “not TopN”, causing incorrect merged histogram counts;
change isGlobalTopNVal (or the enclosing call sites that use it) to propagate
EncodeKey errors instead of returning false silently: have isGlobalTopNVal
return (bool, error) or otherwise return the encode error up the stack when
codec.EncodeKey(tz, ...) fails, and update callers (the Pass 2 merge logic that
uses isGlobalTopNVal, sortedRefs, hists, globalTopNMap, encodeBuf) to abort or
handle the error explicitly so encoding failures surface rather than producing
wrong Repeat bucket counts.
---
Duplicate comments:
In `@pkg/statistics/handle/globalstats/global_stats_test.go`:
- Around line 1161-1166: The test reads async global TopN/buckets immediately
after ANALYZE and dom.StatsHandle().Update(), which risks flakiness because the
background global-merge job may not have completed; modify the test (around the
ANALYZE + dom.StatsHandle().Update() sequence) to wait/poll until the async
merge finishes before asserting: repeatedly run the same query used for
assertions (tk.MustQuery("SHOW STATS_TOPN WHERE table_name = 't' AND
partition_name = 'global'") and/or tk.MustQuery("SHOW STATS_BUCKETS ...")) with
a short sleep and timeout, breaking when the expected global rows appear
(non-empty or match expected counts), then proceed to set asyncTopN/asyncBuckets
and assertions; keep polling timeout to avoid infinite loop.
🪄 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: da20abde-12be-4e64-b440-516a639d97e5
📒 Files selected for processing (19)
pkg/sessionctx/variable/session.gopkg/statistics/BUILD.bazelpkg/statistics/cmsketch_util.gopkg/statistics/handle/globalstats/BUILD.bazelpkg/statistics/handle/globalstats/global_stats.gopkg/statistics/handle/globalstats/global_stats_async.gopkg/statistics/handle/globalstats/global_stats_internal_test.gopkg/statistics/handle/globalstats/global_stats_test.gopkg/statistics/handle/globalstats/merge_worker.gopkg/statistics/handle/globalstats/topn.gopkg/statistics/handle/globalstats/topn_bench_test.gopkg/statistics/handle/globalstats/topn_test.gopkg/statistics/handle/handletest/analyze/analyze_test.gopkg/statistics/handle/util/pool.gopkg/statistics/histogram.gopkg/statistics/histogram_bench_test.gopkg/statistics/histogram_fuzz_test.gopkg/statistics/histogram_test.gopkg/statistics/scalar.go
💤 Files with no reviewable changes (6)
- pkg/statistics/cmsketch_util.go
- pkg/statistics/scalar.go
- pkg/sessionctx/variable/session.go
- pkg/statistics/histogram_bench_test.go
- pkg/statistics/handle/globalstats/merge_worker.go
- pkg/statistics/handle/globalstats/topn.go
✅ Files skipped from review due to trivial changes (2)
- pkg/statistics/handle/util/pool.go
- pkg/statistics/handle/globalstats/global_stats.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/statistics/handle/globalstats/BUILD.bazel
- pkg/statistics/handle/globalstats/topn_bench_test.go
67637c6 to
910e3d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/statistics/histogram_fuzz_test.go (1)
139-151: ⚡ Quick winSort partition
TopNbefore merge in fuzz input generationAt Line 147, entries are appended but never sorted. Adding
tn.Sort()before assigningtopNs[p]keeps fuzz inputs closer to production-shaped TopN data and reduces invalid-input noise.Suggested fix
for k := uint8(0); k < topNPerPart; k++ { v := base + int64(rng.Intn(intraPartRange)) key, err := codec.EncodeKey(sc.TimeZone(), nil, types.NewIntDatum(v)) if err != nil { t.Skip("encode key failed") } cnt := uint64(rng.Intn(20) + 1) tn.AppendTopN(key, cnt) totalRows += int64(cnt) } + tn.Sort() topNs[p] = tn🤖 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/statistics/histogram_fuzz_test.go` around lines 139 - 151, The fuzz input generator creates per-part TopN instances with NewTopN and appends entries via tn.AppendTopN but never sorts them, producing unrealistic inputs; after populating each tn and before assigning topNs[p] call tn.Sort() so each partition's TopN is ordered like production (i.e., insert tn.Sort() between the loop that appends via tn.AppendTopN and the assignment topNs[p] = tn).
🤖 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/statistics/histogram_test.go`:
- Around line 734-736: The helper maxOverMin currently returns 0 when lo <= 0
which masks zero-mass or invalid buckets in ratio assertions; change maxOverMin
so that when lo <= 0 it returns math.Inf(1) (or another sentinel > allowed
ratio) instead of 0 so require.Less(..., 2.0) and similar checks will fail on
zero/invalid masses; update the maxOverMin implementation and add the necessary
math import and adjust tests if they depended on the old zero behavior.
---
Nitpick comments:
In `@pkg/statistics/histogram_fuzz_test.go`:
- Around line 139-151: The fuzz input generator creates per-part TopN instances
with NewTopN and appends entries via tn.AppendTopN but never sorts them,
producing unrealistic inputs; after populating each tn and before assigning
topNs[p] call tn.Sort() so each partition's TopN is ordered like production
(i.e., insert tn.Sort() between the loop that appends via tn.AppendTopN and the
assignment topNs[p] = tn).
🪄 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: 7ef6cf7b-d2c4-4a72-b2b5-0d06dc395a2a
📒 Files selected for processing (24)
pkg/executor/analyze_test.gopkg/executor/set_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.gopkg/statistics/BUILD.bazelpkg/statistics/cmsketch_util.gopkg/statistics/handle/globalstats/BUILD.bazelpkg/statistics/handle/globalstats/global_stats.gopkg/statistics/handle/globalstats/global_stats_async.gopkg/statistics/handle/globalstats/global_stats_internal_test.gopkg/statistics/handle/globalstats/global_stats_test.gopkg/statistics/handle/globalstats/merge_worker.gopkg/statistics/handle/globalstats/topn.gopkg/statistics/handle/globalstats/topn_bench_test.gopkg/statistics/handle/globalstats/topn_test.gopkg/statistics/handle/handletest/analyze/analyze_test.gopkg/statistics/handle/util/pool.gopkg/statistics/handle/util/util.gopkg/statistics/histogram.gopkg/statistics/histogram_bench_test.gopkg/statistics/histogram_fuzz_test.gopkg/statistics/histogram_test.gopkg/statistics/scalar.go
💤 Files with no reviewable changes (7)
- pkg/sessionctx/variable/session.go
- pkg/statistics/handle/util/util.go
- pkg/statistics/histogram_bench_test.go
- pkg/statistics/handle/globalstats/merge_worker.go
- pkg/statistics/cmsketch_util.go
- pkg/statistics/scalar.go
- pkg/statistics/handle/globalstats/topn.go
✅ Files skipped from review due to trivial changes (2)
- pkg/statistics/handle/util/pool.go
- pkg/statistics/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/statistics/handle/globalstats/global_stats_internal_test.go
- pkg/statistics/handle/globalstats/topn_test.go
- pkg/executor/set_test.go
- pkg/statistics/handle/globalstats/topn_bench_test.go
- pkg/statistics/handle/globalstats/global_stats.go
- pkg/executor/analyze_test.go
- pkg/statistics/handle/globalstats/BUILD.bazel
- pkg/statistics/histogram.go
- pkg/statistics/handle/globalstats/global_stats_test.go
- pkg/statistics/handle/handletest/analyze/analyze_test.go
910e3d2 to
d4c1a0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/statistics/histogram_test.go (1)
718-736:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
maxOverMincurrently hides zero-mass regressions in ratio checksAt Line 734, returning
0whenlo <= 0allows checks likerequire.Less(..., 2.0)to pass even when bucket mass is zero/invalid, which can mask real merge regressions.Suggested fix
import ( "fmt" + "math" "testing" "time" @@ if lo <= 0 { - return 0 + return math.Inf(1) }🤖 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/statistics/histogram_test.go` around lines 718 - 736, The function maxOverMin currently returns 0 when any bucket mass is <= 0 which hides regressions; change the behavior so that an empty slice still returns 0 but if lo <= 0 (i.e. any zero/negative mass) the function returns +Inf (use math.Inf(1)) so ratio checks (e.g. require.Less(..., 2.0)) will fail instead of silently passing; update maxOverMin and add the required math import.
🧹 Nitpick comments (2)
pkg/sessionctx/vardef/tidb_vars.go (1)
1012-1014: 💤 Low valueUse the canonical
// Deprecated:GoDoc paragraph for tooling compatibility.The current comment says "is deprecated" inline, but Go doc tools (gopls,
go doc) only recognise a symbol as deprecated when there is a paragraph starting with exactly// Deprecated:. Every other deprecated symbol in this file follows that convention (e.g. lines 165, 201, 687, 810). Without it, IDEs won't surface a deprecation warning at call sites.♻️ Proposed fix
- // TiDBMergePartitionStatsConcurrency is deprecated. It is kept for backward compatibility - // but no longer affects behavior. Global stats always use the combined merge algorithm. + // TiDBMergePartitionStatsConcurrency indicates the number of concurrency for merging + // partition stats. + // + // Deprecated: This variable no longer affects behavior; global stats always use the + // combined merge algorithm. Kept only for backward compatibility. TiDBMergePartitionStatsConcurrency = "tidb_merge_partition_stats_concurrency"🤖 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/sessionctx/vardef/tidb_vars.go` around lines 1012 - 1014, The comment for TiDBMergePartitionStatsConcurrency should use the canonical Go deprecation paragraph so tooling recognizes it; replace the current inline deprecation text with a paragraph that begins exactly "// Deprecated: " followed by the note (e.g., "// Deprecated: TiDBMergePartitionStatsConcurrency is kept for backward compatibility but no longer affects behavior. Global stats always use the combined merge algorithm.") so gopls/go doc surfaces deprecation for the TiDBMergePartitionStatsConcurrency constant.pkg/statistics/handle/globalstats/topn_test.go (1)
64-366: ⚡ Quick winAdd focused coverage for the new cancellation and index-mode paths.
The new cases validate the merge math well, but they never hit the two new branches this PR wires through the merge call:
SQLKillerinterruption andisIndex=true. A small regression test for each would give much better protection for the behavior this change is introducing.🤖 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/statistics/handle/globalstats/topn_test.go` around lines 64 - 366, The tests don't exercise the new SQLKiller cancellation and isIndex=true branches in MergePartTopNAndHistToGlobal; add two focused unit tests: one that constructs a small set of topNs/hists, invokes MergePartTopNAndHistToGlobal with a sqlkiller.SQLKiller that has been triggered/cancelled and asserts an error is returned (or merge aborts) to cover the interruption path, and another that calls MergePartTopNAndHistToGlobal with isIndex=true using a minimal histogram/topN fixture and asserts successful return and expected invariants (non-nil hybridTopN/hybridHist, length/asserted counts) so the index-mode path is exercised; place these alongside the other TestMergePartTopNAndHistToGlobal* tests to ensure the new branches are covered.
🤖 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/statistics/handle/globalstats/global_stats_async.go`:
- Around line 193-199: The log field "skipped" currently uses
len(a.skipPartition) which counts (histID, partitionID) pairs not distinct
partitions; update the stats log in the Info call (the
statslogutil.StatsLogger().Info invocation in global_stats_async.go) to either
(a) compute the number of distinct partition IDs from a.skipPartition (build a
set of partitionIDs and log its length) and replace zap.Int("skipped", ...) with
that distinct count, or (b) if you prefer to keep pair-level semantics, rename
the key to make the unit explicit (e.g., zap.Int("skippedPairs",
len(a.skipPartition))). Ensure the change references a.skipPartition and
a.partitionIDs (or a.globalStats.MissingPartitionStats) so the logged metric
accurately reflects distinct skipped partitions or clearly indicates pair
counts.
In `@pkg/statistics/handle/handletest/analyze/analyze_test.go`:
- Around line 236-245: The test's positional assertions on rows (rows[0],
rows[1]) assume ordering from the SHOW STATS_BUCKETS query; make the result
deterministic by adding an explicit ORDER BY to that query (e.g., ORDER BY
bucket_id or the lower-bound column) in the test setup just before the
assertions so rows maps reliably to the p0/p1 buckets; update the query that
runs SHOW STATS_BUCKETS (the statement that populates rows) to include ORDER BY
and leave the subsequent require.Len/require.Equal checks on rows unchanged.
---
Duplicate comments:
In `@pkg/statistics/histogram_test.go`:
- Around line 718-736: The function maxOverMin currently returns 0 when any
bucket mass is <= 0 which hides regressions; change the behavior so that an
empty slice still returns 0 but if lo <= 0 (i.e. any zero/negative mass) the
function returns +Inf (use math.Inf(1)) so ratio checks (e.g. require.Less(...,
2.0)) will fail instead of silently passing; update maxOverMin and add the
required math import.
---
Nitpick comments:
In `@pkg/sessionctx/vardef/tidb_vars.go`:
- Around line 1012-1014: The comment for TiDBMergePartitionStatsConcurrency
should use the canonical Go deprecation paragraph so tooling recognizes it;
replace the current inline deprecation text with a paragraph that begins exactly
"// Deprecated: " followed by the note (e.g., "// Deprecated:
TiDBMergePartitionStatsConcurrency is kept for backward compatibility but no
longer affects behavior. Global stats always use the combined merge algorithm.")
so gopls/go doc surfaces deprecation for the TiDBMergePartitionStatsConcurrency
constant.
In `@pkg/statistics/handle/globalstats/topn_test.go`:
- Around line 64-366: The tests don't exercise the new SQLKiller cancellation
and isIndex=true branches in MergePartTopNAndHistToGlobal; add two focused unit
tests: one that constructs a small set of topNs/hists, invokes
MergePartTopNAndHistToGlobal with a sqlkiller.SQLKiller that has been
triggered/cancelled and asserts an error is returned (or merge aborts) to cover
the interruption path, and another that calls MergePartTopNAndHistToGlobal with
isIndex=true using a minimal histogram/topN fixture and asserts successful
return and expected invariants (non-nil hybridTopN/hybridHist, length/asserted
counts) so the index-mode path is exercised; place these alongside the other
TestMergePartTopNAndHistToGlobal* tests to ensure the new branches are covered.
🪄 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: 25540839-c0a1-45dd-83e1-cf5843a1c3ac
📒 Files selected for processing (24)
pkg/executor/analyze_test.gopkg/executor/set_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.gopkg/statistics/BUILD.bazelpkg/statistics/cmsketch_util.gopkg/statistics/handle/globalstats/BUILD.bazelpkg/statistics/handle/globalstats/global_stats.gopkg/statistics/handle/globalstats/global_stats_async.gopkg/statistics/handle/globalstats/global_stats_internal_test.gopkg/statistics/handle/globalstats/global_stats_test.gopkg/statistics/handle/globalstats/merge_worker.gopkg/statistics/handle/globalstats/topn.gopkg/statistics/handle/globalstats/topn_bench_test.gopkg/statistics/handle/globalstats/topn_test.gopkg/statistics/handle/handletest/analyze/analyze_test.gopkg/statistics/handle/util/pool.gopkg/statistics/handle/util/util.gopkg/statistics/histogram.gopkg/statistics/histogram_bench_test.gopkg/statistics/histogram_fuzz_test.gopkg/statistics/histogram_test.gopkg/statistics/scalar.go
💤 Files with no reviewable changes (7)
- pkg/statistics/histogram_bench_test.go
- pkg/statistics/handle/util/util.go
- pkg/sessionctx/variable/session.go
- pkg/statistics/cmsketch_util.go
- pkg/statistics/handle/globalstats/topn.go
- pkg/statistics/scalar.go
- pkg/statistics/handle/globalstats/merge_worker.go
✅ Files skipped from review due to trivial changes (4)
- pkg/executor/set_test.go
- pkg/statistics/handle/util/pool.go
- pkg/statistics/BUILD.bazel
- pkg/statistics/histogram.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/statistics/handle/globalstats/BUILD.bazel
- pkg/statistics/handle/globalstats/global_stats_internal_test.go
- pkg/statistics/handle/globalstats/topn_bench_test.go
- pkg/statistics/histogram_fuzz_test.go
- pkg/executor/analyze_test.go
- pkg/statistics/handle/globalstats/global_stats_test.go
@Reminiscent Thank you for pointing this out, you are right and this is a regression from this PR. The cause: the pre-PR merge ended with a "Recalculate repeats" pass that back-filled every global bucket's Repeat from the partition histograms' equality estimates at that bucket's upper. I removed it during the development of this PR, and the combined merge has no equivalent, so a bucket upper introduced by a cut keeps Repeat 0. To your direct question: Repeat == 0 here means "not observed", not "exactly zero". No partition histogram records a point frequency for a value that is only a bucket lower, so the merge has no reliable point-frequency estimate for that boundary. My reason for not carrying the pass over is that it estimates the repeat from ranges in every partition, which inflates it by roughly the number of partitions. Measured on TestGlobalStatsMergeCombined's fixture, where column a is a unique auto_increment key so every true repeat is 1: 7 partitions gives 7, 64 gives 63, 256 gives 252. With 8k hash partitions on the partitioning column only one partition can hold a given value, so the other 8k-1 range estimates are all wrong. That said, this PR should be about CPU and memory, not about changing statistics quality, so parity with the pre-PR output is the right goal here even where I think that output is wrong. I will look at restoring the back-fill without a major performance cost. Note TestGlobalStatsMergeCombined currently pins repeat 0 on four global buckets; those expectations will move back once this is addressed. Improving the repeat estimate itself, along the lines you suggest, is a change I would rather make in a follow-up PR where plan changes can be reviewed on their own. I will ping you when I have a proposal here. |
mjonss
added
the
do-not-merge/hold
Introduce a single-pass combined TopN + histogram merge that will replace the old two-pass design (separate TopN merge with histogram-bound extraction, then bucket-by-bucket histogram merge). The new path is gated on no callers in this commit; production callers and the V1 separate-merge orchestration are switched/removed in follow-ups. Structure: the main function is the orchestration of two passes. Pass 1 merges two sorted group streams behind small cursors (topNCursor over the flattened, compacted partition TopN entries; bucketGroupCursor over a k-way merge heap, yielding one same-upper bucket group at a time), with the self-contained phases extracted as flattenSortedTopN, sumPartitionTotals, selectGlobalTopN, and collectVirtualTopN. TopN values that don't make the global TopN become virtual single-value buckets merged into the ref stream, so Pass 2 walks one uniform stream with no special TopN handling. Pass 2 (buildGlobalHistogram) is the right-to-left equi-depth walk; per-ref bookkeeping (on-demand bound access, global-TopN Repeat ownership, overlap-split residue) lives behind globalMergeRefs. - bucketRef stays 4 bytes per entry by reading bound Datums on demand rather than inlining types.Datum (~32 MB vs ~350 MB sortedRefs at 8k partitions × 500 buckets). - Hand-rolled heap (pushEntry/popMin/up/down) avoids the per-pop interface boxing of container/heap (~100 B of allocation per popped ~80 B entry), which adds up at multi-million-pop scale. - TopN merge uses a reusable encode buffer for O(1) lookup into the global TopN map and a bounded min-heap for top-N selection. - SQLKiller is plumbed in as a parameter so the algorithm can poll for user-initiated KILL during long merges. Two boundary rules matter for estimate accuracy, both reported in review: - A ref whose effective upper lands exactly on the merged bucket's lower bound keeps its Repeat. TiDB histograms represent a point mass only at a bucket's upper, since EqualRowCount matches the value against Bucket.Repeat of the bucket whose upper it is. Attributing that Repeat to the bucket where the value is the lower would bury the rows as interior mass and leave the neighbouring bucket reporting Repeat 0 for its own upper, estimating the value at 0. The overlap scan therefore stops at a ref at or before the boundary and leaves it whole for the bucket built to its left. - selectGlobalTopN prunes count==1 candidates only when the merge walk offered more distinct values than numTopN. The candidate heap is bounded at numTopN, so testing the returned slice length reduces to "the heap is full" and cannot tell "exactly numTopN candidates, all of which fit" from "more candidates than slots, some evicted". Only the latter makes singletons noise, so the walk counts candidates and passes that count in. Without it, two distinct values with numTopN=2 produced an empty global TopN while each partition kept its singleton. Library Bazel target picks up //pkg/util/sqlkiller for the new parameter type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Switch the blocking and async global stats merge entrypoints from the two-pass V1 algorithm (mergeGlobalStatsTopN -> MergePartitionHist2Global Hist) to MergePartTopNAndHistToGlobal added in the previous code commit. The async path now captures the caller's SQLKiller before entering util.CallWithSCtx so KILL / connection close from the user query is honored during the merge: CallWithSCtx hands the algorithm a pooled stats sctx whose lifecycle is independent of the user query, so its killer wouldn't otherwise reflect user-initiated cancellation. Drop V1 separate-merge orchestration that the new algorithm supersedes: - pkg/statistics/handle/globalstats/topn.go (mergeGlobalStatsTopN dispatcher and the parallel-by-batch path; the in-process call sites are gone after the switch above). - merge_worker.go: keep StatsWrapper / NewStatsWrapper (the async path still ferries hist+topN through it) and drop the rest of the file. - pkg/sessionctx/variable.SessionVars.AnalyzePartitionMergeConcurrency, whose only remaining reader was topn.go. - MaxPartitionMergeBatchSize and the gpool plumbing in global_stats.go, unused after the switch. - handle/util/pool.go comment update, the gpool is now described as serving "the global stats merge" generically rather than the deleted mergeGlobalStatsTopN helper. The combined merge can end a global bucket on a value introduced by a merge cut, which no partition bucket owns, so that bucket carries Repeat 0. A bucket upper is a value observed in the data, so that zero means "no point frequency recorded", not "no rows". equalRowCount would otherwise treat it as an exact answer, so both the column and the index path now fall through to estimateRowCountWithUniformDistribution when a matched Repeat is zero, the same treatment IsLastBucketEndValueUnderrepresented already applies to a matched Repeat that looks too small. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ed merge Add tests covering the new combined-merge algorithm and refresh existing test expectations affected by the algorithm switch. Unit / fuzz tests: - pkg/statistics/merge_global_test.go + merge_global_cases_test.go: table-driven coverage of MergePartTopNAndHistToGlobal. The fixture is algorithm-agnostic, cases describe (per-partition TopN entries + buckets) plus optional pinned expectations, and the merge call site is a single line so the same fixture can be retargeted to a different merge implementation. Structural invariants run on every case. Cases include: disjoint / overlapping / equi-depth / spread-value / no-inflation / TopN-sorted-by-encoded / fat-value-promoted / hot-value-cross-partition / ghost-bucket- guard / killer-propagation / index-path / varchar-with-collation / exp-buckets edge cases / numTopN=0. Case boundary_repeat_stays_with_left_bucket pins that a value sitting on a bucket boundary keeps its Repeat on the bucket whose upper it is, asserting the masses, the Repeat placement and the EqualRowCount result. The singleton-filter test pins that the pruning gate follows tidb_analyze_default_num_topn (mirroring BuildHistAndTopN) rather than the compiled-in constant, and that a candidate pool of exactly numTopN keeps its singletons, the boundary between the existing over-capacity and below-capacity checks. - pkg/statistics/handle/globalstats/topn_bench_test.go: renamed to BenchmarkGlobalStatsMerge plus a skewed-input variant; seed both fixture builders with rand.New(rand.NewSource(20150401)) so cross-branch benchstat comparisons aren't perturbed by per-run RNG drift. - pkg/statistics/histogram_fuzz_test.go: property-based fuzz checking eight structural invariants on the merge output (no inverted bounds, ordered bucket lowers, monotonic cumulative counts, total rows preserved, bucket cap, TopN sorted, no Repeat at TopN-matching upper, every bucket has positive mass). Fuzz dimensions include int / varchar source columns and both column and index histograms. Refreshed integration test expectations for the new bucket shape: - handle/globalstats/global_stats_internal_test.go: testIssues24349 layout, column b now produces three global buckets ([1,1] mass 2 Repeat 2, [1,3] mass 2 Repeat 1, [3,4] mass 4 Repeat 1) instead of one global bucket collapsing the whole range: non-promoted TopN values enter the merge as virtual single-value buckets, letting the equi-depth gate cut between a TopN value and a wide bucket sharing its upper. The expectation carries a comment recording that this layout does not improve that fixture's estimates, since b=1 and b=3 each have rows spread over several refs that the merge does not reunite. - handle/globalstats/global_stats_test.go: TestGlobalStatsMerge Combined pins the global stats_topn / stats_buckets shape, and TestGlobalStatsMergePathConsistency verifies async ≡ blocking merge produce identical global stats. - handle/handletest/analyze/analyze_test.go: TestAnalyzeWithDynamic PartitionPruneMode now sees two global buckets preserving the gap between p0 and p1, with cumulative count 6 at the last bucket; bucket bound assertions added. Bazel: pkg/statistics/BUILD.bazel adds merge_global_test.go and merge_global_cases_test.go plus histogram_fuzz_test.go to srcs and //pkg/util/sqlkiller to test deps; pkg/statistics/handle/globalstats/ BUILD.bazel drops the deleted topn_test.go. The merge test files live in package statistics_test so a case can also assert what the optimizer estimates from the merged stats, through the cardinality entry points, rather than only how the buckets are shaped. Cases declare globalNDV, the NDV the merge's caller fills in from the merged FMSketch and the uniform fallback divides by. Estimates are now pinned for the boundary-Repeat, promoted-TopN, unobserved-boundary, thin-tail-range and index cases. Wiring the merge output through those entry points also showed it needs PreCalculateScalar before the range estimators can interpolate over it, which storage does on load. pkg/planner/cardinality/selectivity_test.go gains coverage for the non-merge source of a zero Repeat: the sampled builder writes int64(min(count/ndv, sampleFactor)), which truncates to zero when the estimated NDV exceeds the histogram's row count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Remove V1 dead code now that the combined merge is the only path: - pkg/statistics/histogram.go: BinarySearchRemoveVal, the bucket4Merging type and its sync.Pool, MergePartitionHist2GlobalHist and its mergeBucketNDV / mergePartitionBuckets / sortBucketsByUpperBound / checkBucket4MergingIsSorted / TopNMeta.buildBucket4Merging helpers, and the now-unused failpoint import. - pkg/statistics/cmsketch_util.go: DatumMapCache (only consumer was the V1 separate TopN merge in topn.go / merge_worker.go, now deleted). - pkg/statistics/histogram_test.go: drop genBucket4Merging4Test and TestMergeBucketNDV (V1-internal-helper tests with no V2 equivalent); TestMergePartitionLevelHist is retained, with its body ported to call the new MergePartTopNAndHistToGlobal. The V1 popedTopN argument is carried as one partition's TopN with numTopN=0 so nothing is promoted globally and every entry flows into Pass 2 as a leftover- TopN injection. NDV per bucket is always 0 in the new merge so bucket4Test.ndv is no longer asserted. - DELETE pkg/statistics/histogram_bench_test.go: benchmarked the removed bucket-merge code path; the new merge is benchmarked in pkg/statistics/handle/globalstats/topn_bench_test.go. - pkg/statistics/BUILD.bazel: drop histogram_bench_test.go from srcs. Note: calcFraction4Datums in pkg/statistics/scalar.go is retained for use by the new combined merge (introduced in the earlier commit on this branch).
Add structured info logs at each phase of the async global stats merge: - prepare: per-partition meta fetch start, and a "done" summary with partition / skipped / missing counts and realtime/modify count totals. - loadFmsketch / loadCMsketch / loadHistogramAndTopN: phase start with table/index identity, partition count, hist count, and the system tables being read. These give operators a low-cardinality timeline of long-running global merges and pinpoint which IO phase a stuck/slow merge is in.
5a20460 to
ac6db17
Compare
|
@Reminiscent Following up as promised, and with a change of plan from what I said above. I did implement the back-fill for output parity, and then took it back out. So I went with your suggestion in this PR after all. My reason for wanting it separate What is in ac6db17:
Correcting one thing I said earlier: For the regression test you asked for, the merge cases moved to |
@0xPoe confirmed, fixed. The candidate heap is bounded at The walk now counts candidates and prunes only above |
|
/retest |
ti-chi-bot
Bot
added
lgtm
approved
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0xPoe, terry1purcell, time-and-fate 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 |
[P1] Use one canonical ordering for TopN and histogram candidatesThe two input streams in the new combined merge are not ordered by the same relation:
Those orders differ for production types including ENUM, SET, BIT, and JSON. As a result, equal values can be consumed at different positions and never aggregated. I can reproduce this through partition SET @@tidb_analyze_version = 2;
SET @@tidb_partition_prune_mode = 'dynamic';
SET @@GLOBAL.tidb_persist_analyze_options = 0;
CREATE TABLE t_enum_order (p INT, e ENUM('z','a'))
PARTITION BY RANGE (p) (
PARTITION p0 VALUES LESS THAN (10),
PARTITION p1 VALUES LESS THAN (20)
);
INSERT INTO t_enum_order VALUES
(1,'z'),(1,'z'),(1,'z'),(1,'z'),(1,'z'),
(11,'a'),(11,'a'),(11,'a'),(11,'a'),
(11,'a'),(11,'a'),(11,'a'),
(11,'z'),(11,'z'),(11,'z');
ANALYZE TABLE t_enum_order PARTITION p1 WITH 0 TOPN, 2 BUCKETS;
ANALYZE TABLE t_enum_order PARTITION p0 WITH 3 TOPN, 2 BUCKETS;
SHOW STATS_TOPN
WHERE table_name = 't_enum_order'
AND partition_name = 'global'
AND column_name = 'e';The real partition statistics are: The persisted global TopN is: The correct result has one A direct call to The merge needs one consistent ordering/equality relation for both cursors—for example, order decoded TopN values with the histogram field-type semantics while retaining the encoded bytes as the identity key. |
[P1] Rebuild virtual ENUM/SET/BIT bounds with a datum kind compatible with the histogramWhen a partition TopN value is not promoted to the global TopN, For ENUM, SET, and BIT, codec decoding produces a This is reachable through a normal partition SET @@tidb_analyze_version = 2;
SET @@tidb_partition_prune_mode = 'dynamic';
SET @@GLOBAL.tidb_persist_analyze_options = 0;
CREATE TABLE t_enum_virtual (p INT, e ENUM('z','a'))
PARTITION BY RANGE (p) (
PARTITION p0 VALUES LESS THAN (10),
PARTITION p1 VALUES LESS THAN (20)
);
INSERT INTO t_enum_virtual VALUES
(1,'z'),(1,'z'),(1,'z'),(1,'z'),(1,'z'),
(11,'a'),(11,'a'),(11,'a'),(11,'a'),(11,'a'),
(11,'a'),(11,'a'),(11,'a'),(11,'a'),(11,'a');
ANALYZE TABLE t_enum_virtual PARTITION p1 WITH 0 TOPN, 1 BUCKETS;
ANALYZE TABLE t_enum_virtual PARTITION p0 WITH 1 TOPN, 1 BUCKETS;
SHOW ANALYZE STATUS;Here p1 contributes a real singleton bucket The partition The stack is: If an earlier partial global merge succeeded, the failed job leaves that previous global statistics row in place, so users can see a successful SQL response together with stale global statistics. The low-level append issue can also be reached on the base commit with Please convert the decoded value back to a datum matching |
topNMetaToDatum decoded a TopN key with codec.DecodeOne and returned it as-is, so ENUM, SET and BIT came back as KindUint64 while a histogram bound for those types is KindMysqlEnum / KindMysqlSet / KindMysqlBit. Two failures follow, both reported by winoros on pingcap#68147: - Datum.Compare dispatches on kind, so a bound compared against another bound orders ENUM by element name while the same bound compared against a plain number orders numerically. The merge walks the TopN stream against the bucket stream, so equal values are visited at different positions and never aggregated: a value occupies two global TopN slots and QueryTopN returns only one of the counts. - Appending that datum to a virtual histogram reaches Column.AppendUint64 on a variable-length chunk column and panics with an out-of-range access, which the async merge records as a failed job while ANALYZE reports success, leaving stale global stats in place. Both predate this PR. On the base commit, MergePartitionHist2GlobalHist converts a poped TopN entry with the same helper and panics identically, and MergePartTopN2GlobalTopN uses the decoded datum for EqualRowCount and BinarySearchRemoveVal, so a value with 5 rows in one partition's TopN and 3 in another's histogram is recorded with a count of 5. ENUM, SET and BIT global TopN counts have been wrong since well before this change. The conversion is now tablecodec.Unflatten, which is the shared helper for turning a flattened value into a column datum and already handles these three types plus the time and float cases topNMetaToDatum was special-casing by hand. Ordering needs one more thing: the TopN stream must be sorted by the relation the walk compares with. flattenSortedTopN sorts by the decoded datum for the types whose memcomparable encoding disagrees with Datum.Compare, and keeps the byte sort everywhere else, so the many partition case pays nothing. The encoded key stays the identity for compaction. merge_global_types_test.go covers every column type, signed and unsigned, and both string collations, asserting that a value shared between a partition's TopN and another's bucket upper aggregates, that a non-promoted TopN value can be rebuilt, and both of those through the index path, which is unaffected because its two streams share one encoding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fuzz test picked between TypeLong and TypeVarchar. Both have an order-preserving encoding and decode back to their own datum kind, so none of topNMetaToDatum's type-dependent branches were reached and neither ENUM/SET/BIT defect was reachable by fuzzing. tpKind now selects across the types where something type-dependent happens: ENUM and SET with a small and a large element list, BIT, datetime and float, alongside the original two. The two element-list sizes cover both storage forms: a SET bitmap is one byte up to 8 elements and two beyond, and an ENUM index is one byte up to 255 elements and two beyond. Element names are the reverse of their numeric order on purpose, since those types encode by value but compare by name, and that disagreement is what the merge has to survive. The generated bounds are now sorted the way the stored bound compares. They were sorted by encoded bytes for every type, with a comment claiming the merge orders by encoded form regardless of source type, which is the assumption that allowed the defect. A column histogram is ordered by Datum.Compare, an index histogram stores the encoded key itself and so is ordered by those bytes. Types with a small value domain, a 6 element ENUM, cannot give every partition a disjoint band or supply many distinct bounds, so the shape is clamped and overlap forced rather than skipping the iteration. Seeds cover every added type through both the column and the index path. Against the previous commit's code the seed corpus fails immediately with the out-of-range append; with it, 1.2M executions over three minutes are clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ts tests sortTopNEntries now owns the whole ordering decision, so needsDatumOrdering is gone and the caller no longer repeats the isIndex condition. The dedupe map keeps a note about why it is keyed by value: the same value is typically in many partitions' TopN, and these types have few distinct values, so the decode cost follows the value domain rather than the partition count. Dropping that map would trade U decodes for N. The type matrix that came with the fix was larger than the fix warrants: - TestMergeAggregatesEqualValuesAcrossTypes is removed. It was written before the sort half of the bug was understood, and TestMergeAggregatesBothValuesAcrossTypes covers everything it did: a single-entry TopN stream cannot be out of order, so the two-entry case is what actually exercises the ordering. - TestMergeIndexPathAcrossTypes runs two representative types rather than all of them. It proves a negative, that the index path never leaves the encoded form and so was never affected, and breadth adds nothing to that. - TestOutOfScopeTypesCarryNoStats is removed, along with the testkit dependency it added to this package's tests. It spun a session to confirm three facts that change roughly never. The types it covered are recorded in a comment naming what was checked by hand: ANALYZE produces no stats_histograms row for a JSON or vector column, and a geometry column cannot be created. What remains fails without the fix: the rebuild test panics on ENUM, SET and BIT, and the both-values test leaves a value in two TopN slots. TestTopNOrderingMatchesDatumOrder pins the type list in both directions, and TestTypeMatrixCoversAllColumnTypes fails for a column type that is neither covered nor declared out of scope. 157 subtests become 67, and the file drops from 550 lines to 467. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@winoros Thank you, both reproduce and both are fixed. They are the same defect. Both predate this PR. On the base commit The conversion now goes through Tests are extended, including the fuzz test, which now covers these types rather than I added these changes as commits on top of the previous logical 6 commits, so you and others can review these changes separately if needed. |
Benchmark results (first 6 logical commits, not including the ENUM/SET/BIT fixes, which should not affect this)ac6db17 vs its merge-base 93f713c, both rebuilt from source.
Performance
Splitting out the phase this PR actually changes: per-partition collection is AccuracyNo regression: row count ratio 1.0000, ΔNDV = 0 on all 20 columns, The PR also fixes a row-count bug in the old merge. Comparing each merged
Deterministic — identical in both async modes and reproduced exactly in an Effect on cardinality estimates228 point/range probes per branch, exact ground truth by counting, then
Range predicates never get worse. Worst-case q-error across Two honest caveats:
|
mjonss
removed
the
do-not-merge/hold
|
Removed the |
7 participants
What problem does this PR solve?
Issue Number: ref #66220
Problem Summary:
Global stats merge for partitioned tables had three problems:
ANALYZEwall-clock on tables with many partitions.ANALYZE.Repeatfrom per-partitionEqualRowCountestimates at the bucket upper, inflating equality counts and double-counting TopN-promoted rows in cumulative bucket counts. This distorted range and equality estimates and sometimes flipped plan choices. Measured on a unique column, that recomputation returns roughly one row per covering partition: 7, 63 and 252 at 7, 64 and 256 partitions, where every trueRepeatis 1.What changed and how does it work?
Replaced the merge with a two-phase algorithm:
Repeatcounts. In-bucket (estimated range) counts are not added to TopN counts. This is a deliberate change from the prior algorithm, which scanned partition histograms and added estimated in-bucket counts, inflating TopN counts by up to ~7600×. A value that is frequent overall but appears only as a histogram-bound repeat in some partitions is still picked correctly.calcFraction4Datums: the left portion contributes to the bucket just closed, the right portion stays for the next one. What changed is how the partition buckets are represented during the merge. The new implementation walks per-partition bucket references (4 bytes each, reading bounds on demand from a reusable scratchDatumbuffer) instead of copying every bound'sDatuminto a sorted slice. At 8k partitions × 500 buckets that is roughly 32 MB vs ~350 MB, which is the source of the memory reduction. Partition TopN entries not promoted to the global TopN are injected back as single-row bucket contributions. Buckets whose upper matches a global TopN value have theirRepeatzeroed so those rows are not double-counted.Other changes in this PR:
SQLKilleris now honored during merges, soKILLand connection close take effect mid-merge.tidb_merge_partition_stats_concurrencyis deprecated: setting it to a non-1 value emits a deprecation warning and is otherwise ignored; reads always return1.Bucket.Repeatas "no point frequency recorded" rather than "zero rows". A bucket's upper bound is by construction a value present in the data, soequalRowCountOnColumnandequalRowCountOnIndexnow fall through toestimateRowCountWithUniformDistributioninstead of returning an exact zero. This replaces the back-fill removed in problem 3 above: the merge reports only observed repeats, and the estimator decides what an absent one means, using a single global ratio rather than a sum that grows with the partition count. It also covers ordinary single-table histograms, wherebuildHistwritesint64(min(count/ndv, sampleFactor))and truncates to zero when the estimated NDV exceeds the histogram's row count. No plan changes resulted: the fulltests/integrationtestcorpus andpkg/planner/cardinalityare both unchanged.End-to-end results on a realistic single-partition
ANALYZE(8000 partitions × 33 columns × 30M rows, see #67501 perf report for full details):async_merge=ON(default)async_merge=OFFPlus measurably more accurate global stats: the old algorithm had several distinct correctness bugs (TopN count inflation up to ~7600×, TopN truncation, histogram repeat inflation, cumulative count over-counting up to +82M rows) which this PR fixes.
Possible follow-up: cap the merged TopN size so its lowest entry's count stays well above the histogram's per-bucket density. Today the merge respects the user-configured TopN size even when the long tail of low-count entries would be more accurately represented as histogram buckets.
Reviewing. The branch is 6 commits, each one logical chunk: deprecate sysvar → add the new algorithm → switch callers (and thread
SQLKiller) → add tests + refresh expectations + benchmark → drop dead V1 code → add diagnostic logs. Reviewers can step through commit-by-commit rather than reading the squashed diff.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Deprecations
Refactor
Behavior Changes
Tests & Tools