Skip to content

planner, util: fix statement summary history and display correctness - #70159

Open
jiong-nba wants to merge 7 commits into
pingcap:masterfrom
jiong-nba:fix/stmt-summary-read-display-correctness-pr2
Open

planner, util: fix statement summary history and display correctness#70159
jiong-nba wants to merge 7 commits into
pingcap:masterfrom
jiong-nba:fix/stmt-summary-read-display-correctness-pr2

Conversation

@jiong-nba

@jiong-nba jiong-nba commented Jul 28, 2026

Copy link
Copy Markdown

What problem does this PR solve?

Issue Number: #70161

Problem Summary:

Statement Summary v1/v2 has several independent read and display correctness issues: open-ended history predicates are narrowed to one hour, history readers return stale intervals, execution averages use the commit count, table names can contain trailing separators, and v2 DIGEST_TEXT ignores tidb_stmt_summary_max_sql_length.

What changed and how does it work?

  • Preserve open-ended statement-summary time predicates with MinDatetime/MaxDatetime.
  • Return the latest configured history intervals in chronological order for regular and evicted v1 history.
  • Use EXEC_COUNT as the denominator for execution-based KV/PD/backoff/write-response averages in v1 and v2.
  • Filter empty table entries before joining TABLE_NAMES in v1 and v2.
  • Format v2 DIGEST_TEXT with the same configured SQL-length behavior as v1.
  • Add focused regressions for every fix.

Each fix is kept in its own commit to make review and selective backporting straightforward.

Check List

Tests

  • Unit test
    • ./tools/check/failpoint-go-test.sh pkg/util/stmtsummary -run '^(TestHistoryClearAndResizeKeepsLatestIntervals|TestEvictedHistoryCollectionKeepsLatestIntervals|TestExecutionAverageColumnsUseExecCount|TestTableNamesSkipEmptyTables)$' -tags=intest,deadlock -count=1
    • go test ./pkg/util/stmtsummary/v2 -run '^(TestExecutionAverageColumnsUseExecCount|TestStmtRecordTableNamesSkipEmptyTables|TestStmtRecordFormatsDigestText)$' -tags=intest,deadlock -count=1
    • v1 and v2 focused regressions passed with -race.
    • go test ./pkg/planner/core/operator/logicalop/logicalop_test -run '^TestStatementsSummaryExtractorOpenEndedTimeRange$' -tags=intest,deadlock -count=1
    • make lint
    • make bazel_prepare is blocked locally because bazel is not installed.
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

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.

Fix incorrect Statement Summary history ranges, interval selection, average timings, table names, and v2 digest text formatting.

Summary by CodeRabbit

Summary by CodeRabbit

  • Bug Fixes
    • Improved statement summary time filtering for queries with only a start or end time, preserving open-ended ranges.
    • Corrected average execution-time metrics to use execution counts.
    • History resizing and clearing now retain the most recent intervals.
    • Fixed table-name formatting when entries are incomplete.
    • Applied SQL length formatting consistently to stored statement records.
  • Tests
    • Added coverage for open-ended time ranges, history interval retention, and updated averaging logic.

Copilot AI review requested due to automatic review settings July 28, 2026 08:41
@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-triage-completed labels Jul 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@ti-chi-bot ti-chi-bot Bot added sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 28, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign winoros for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found 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

@pingcap-cla-assistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


jiong seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3b241229-a777-4f22-9d03-f997cb96a126

📥 Commits

Reviewing files that changed from the base of the PR and between 96eaf4c and 2d6d889.

📒 Files selected for processing (14)
  • pkg/planner/core/memtable_predicate_extractor.go
  • pkg/planner/core/operator/logicalop/logicalop_test/BUILD.bazel
  • pkg/planner/core/operator/logicalop/logicalop_test/logical_mem_table_predicate_extractor_test.go
  • pkg/util/stmtsummary/BUILD.bazel
  • pkg/util/stmtsummary/evicted.go
  • pkg/util/stmtsummary/evicted_test.go
  • pkg/util/stmtsummary/reader.go
  • pkg/util/stmtsummary/statement_summary.go
  • pkg/util/stmtsummary/statement_summary_test.go
  • pkg/util/stmtsummary/v2/BUILD.bazel
  • pkg/util/stmtsummary/v2/column.go
  • pkg/util/stmtsummary/v2/column_test.go
  • pkg/util/stmtsummary/v2/record.go
  • pkg/util/stmtsummary/v2/record_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
  • pkg/util/stmtsummary/evicted.go
  • pkg/planner/core/operator/logicalop/logicalop_test/logical_mem_table_predicate_extractor_test.go
  • pkg/util/stmtsummary/v2/column.go
  • pkg/util/stmtsummary/v2/record_test.go
  • pkg/util/stmtsummary/v2/column_test.go
  • pkg/util/stmtsummary/evicted_test.go
  • pkg/util/stmtsummary/reader.go
  • pkg/planner/core/memtable_predicate_extractor.go
  • pkg/util/stmtsummary/v2/record.go
  • pkg/util/stmtsummary/statement_summary_test.go
  • pkg/util/stmtsummary/statement_summary.go

📝 Walkthrough

Walkthrough

The changes correct statement-summary time-bound extraction, history retention order, execution-based averages, table-name joining, and v2 digest text formatting. Regression tests cover planner predicates, history resizing, metric calculations, table names, and SQL formatting.

Changes

Statement summary correctness

Layer / File(s) Summary
Open-ended time-range extraction
pkg/planner/core/memtable_predicate_extractor.go, pkg/planner/core/operator/logicalop/logicalop_test/...
Missing predicate bounds use minimum or maximum datetime sentinels, with explanation formatting and tests for both open-ended cases.
Latest history retention
pkg/util/stmtsummary/evicted.go, pkg/util/stmtsummary/statement_summary.go, pkg/util/stmtsummary/*_test.go
History collection and clearing retain the newest intervals while preserving chronological result order.
V1 summary calculations and names
pkg/util/stmtsummary/reader.go, pkg/util/stmtsummary/statement_summary.go, pkg/util/stmtsummary/statement_summary_test.go
Timing averages use execution counts, and table names omit incomplete entries and separators.
V2 summary record formatting
pkg/util/stmtsummary/v2/*
V2 timing averages use execution counts, table names skip empty tables, and normalized SQL uses configured formatting limits.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

  • pingcap/tidb#70158: Covers the same planner time-range, history, average-calculation, table-name, and SQL-formatting corrections.
  • pingcap/tidb#70161: Covers the same statement-summary correctness objectives addressed here.

Suggested reviewers: xuhuaiyu, yibin87

Poem

A rabbit hops through history’s queue,
Keeping the newest rows in view.
Open bounds stretch wide and true,
Averages count executions too.
Clean names and digests shine—
“Hop!” says the bunny, “All look fine!”

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: statement summary history and display correctness fixes across planner and util.
Description check ✅ Passed The description matches the template and covers the required sections; the issue reference is present but should use close or ref formatting.
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

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

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 31.11111% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.8341%. Comparing base (2ddad9e) to head (2d6d889).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #70159        +/-   ##
================================================
- Coverage   76.3254%   73.8341%   -2.4913%     
================================================
  Files          2041       2060        +19     
  Lines        559516     579756     +20240     
================================================
+ Hits         427053     428058      +1005     
- Misses       131563     151325     +19762     
+ Partials        900        373       -527     
Flag Coverage Δ
integration 40.9121% <31.1111%> (+1.2542%) ⬆️

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

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

@jiong-nba
jiong-nba force-pushed the fix/stmt-summary-read-display-correctness-pr2 branch from 96eaf4c to 2d6d889 Compare July 28, 2026 09:55
@ti-chi-bot ti-chi-bot Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 28, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-linked-issue label, please provide the linked issue number on one line in the PR body, for example: Issue Number: close #123 or Issue Number: ref #456.

📖 For more info, you can check the "Contribute Code" section in the development guide.

@ti-chi-bot

ti-chi-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

@jiong-nba: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
idc-jenkins-ci-tidb/unit-test 2d6d889 link true /test unit-test

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

2 participants