Skip to content

statistics: remove unused global singleton estimator - #70928

Merged
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
0xPoe:cleanup/remove-unused-global-singleton-estimator
Sep 9, 2026
Merged

statistics: remove unused global singleton estimator#70928
ti-chi-bot[bot] merged 1 commit into
pingcap:masterfrom
0xPoe:cleanup/remove-unused-global-singleton-estimator

Conversation

@0xPoe

@0xPoe 0xPoe commented Sep 7, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: ref #67449

Problem Summary: EstimateGlobalSingletonBySketches has no production callers.

What changed and how does it work?

Remove the unused estimator, its private helpers and dedicated tests; regenerate Bazel metadata. Keep NDV estimation unchanged.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Validation (Ready profile; failpoints enabled and cleaned up):

make bazel_prepare
./tools/check/failpoint-go-test.sh pkg/statistics -run '^TestEstimateNDVByGEE$' -count=1
make lint
git diff --check

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Summary by CodeRabbit

  • Removed

    • Removed global singleton estimation based on sketch data.
    • The associated public estimation capability is no longer available.
  • Tests

    • Removed coverage for the retired global singleton estimation behavior, including edge cases and invalid-input scenarios.

@ti-chi-bot ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. component/statistics sig/planner SIG: Planner labels Sep 7, 2026
@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: d010640e-e579-4d82-801f-12cdeb24ab82

📥 Commits

Reviewing files that changed from the base of the PR and between aec988e and d2afdda.

📒 Files selected for processing (3)
  • pkg/statistics/BUILD.bazel
  • pkg/statistics/estimate.go
  • pkg/statistics/estimate_test.go
💤 Files with no reviewable changes (3)
  • pkg/statistics/BUILD.bazel
  • pkg/statistics/estimate.go
  • pkg/statistics/estimate_test.go

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The change removes global singleton estimation from pkg/statistics, deletes its test suite, and removes the test file from the Bazel target.

Changes

Cohort / File(s) Summary
Global singleton estimation removal
pkg/statistics/estimate.go, pkg/statistics/estimate_test.go, pkg/statistics/BUILD.bazel
Removes the estimation API, supporting helpers, validation logic, aggregation tests, and Bazel source declaration.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 3bf7d

This change removes an unused estimator and its associated tests and build metadata while leaving NDV estimation unchanged. No concrete current-head merge risk is identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the package and the main change: removal of the unused global singleton estimator.
Description check ✅ Passed The description follows the required template, references issue #67449, explains the problem and changes, records validation steps, and includes side-effect, documentation, and release-note sections.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

A rabbit reads each line,
The patch grows clear beneath the moon,
Small changes hop in place,
Tests guard the garden path,
Reviews bloom before the dawn.

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.9336%. Comparing base (5acf657) to head (3bf7d8c).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #70928        +/-   ##
================================================
- Coverage   76.3212%   72.9336%   -3.3876%     
================================================
  Files          2041       2109        +68     
  Lines        556443     599132     +42689     
================================================
+ Hits         424684     436969     +12285     
- Misses       130859     160280     +29421     
- Partials        900       1883       +983     
Flag Coverage Δ
integration 42.1823% <ø> (+2.5147%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 58.8258% <ø> (ø)
parser ∅ <ø> (∅)
br 46.5879% <ø> (-16.1200%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@0xPoe

0xPoe commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

/hold

@ti-chi-bot ti-chi-bot Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2026
@0xPoe
0xPoe force-pushed the cleanup/remove-unused-global-singleton-estimator branch from d2afdda to 3bf7d8c Compare September 9, 2026 08:21

@0xPoe 0xPoe left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔢 Self-check (PR reviewed by myself and ready for feedback)

  • Code compiles successfully

  • Unit tests added

  • No AI-generated elegant nonsense in PR.

  • Comments added where necessary

  • PR title and description updated

  • Documentation PR created (or confirmed not needed)

  • PR size is reasonable

/cc @qw4990 @mjonss

@ti-chi-bot
ti-chi-bot Bot requested review from mjonss and qw4990 September 9, 2026 08:24

@mjonss mjonss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup!

@ti-chi-bot ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 9, 2026
@ti-chi-bot

ti-chi-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mjonss, qw4990

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 9, 2026
@ti-chi-bot

ti-chi-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-09-09 08:28:18.327414339 +0000 UTC m=+1872733.498508510: ☑️ agreed by mjonss.
  • 2026-09-09 08:30:53.169329093 +0000 UTC m=+1872888.340423203: ☑️ agreed by qw4990.

@0xPoe

0xPoe commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

/unhold

@ti-chi-bot ti-chi-bot Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2026
@0xPoe

0xPoe commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

/retest

@ti-chi-bot
ti-chi-bot Bot merged commit 3b25bda into pingcap:master Sep 9, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved component/statistics lgtm release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants