Skip to content

planner: align core casetests with plan_tree - #67113

Merged
ti-chi-bot[bot] merged 12 commits into
pingcap:masterfrom
hawkingrei:planner-casetest-plan-tree
Mar 18, 2026
Merged

ti-chi-bot[bot] merged 12 commits into
pingcap:masterfrom
hawkingrei:planner-casetest-plan-tree

Conversation

@hawkingrei

@hawkingrei hawkingrei commented Mar 18, 2026 •

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: ref #67112

Problem Summary:

  • pkg/planner/core/casetest still had a mix of default explain, brief, and plan_tree outputs in ordinary planner assertions
  • the mixed formats made testdata inconsistent and left several inline assertions tied to brief-only columns

What changed and how does it work?

  • switched ordinary planner core casetests to explain format = 'plan_tree'
  • kept the requested exceptions unchanged:
    • explain-format-specific tests
    • explain for connection
    • plan cache tests
    • cost trace tests
    • TPCH / TPCDS tests
    • explain analyze cases that do not support plan_tree
  • updated inline assertions that compare plan_tree output directly
  • re-recorded the affected suite outputs under pkg/planner/core/casetest/**/testdata

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test

Validation commands:

TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/ch -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/correlated -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/cbotest -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/enforcempp -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/hint -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/index -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/indexmerge -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/join
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/logicalplan
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/mpp -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/parallelapply
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/partition -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/planstats -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/rule -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/scalarsubquery
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/schema -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/vectorsearch -args -record
TMPDIR=/home/hawkingrei/.cache/go-tmp go test -count=1 -tags=intest,deadlock ./pkg/planner/core/casetest/windows -args -record
make lint

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

None

Summary by CodeRabbit

  • Tests
    • Standardized planner test outputs to use a unified plan-tree format and normalized plan text (removed instance-specific cost/ID noise) across many planner test cases — observable behavior and query results unchanged.
  • Chores
    • Updated tester guidelines and docs to reflect the new explain-format conventions for future planner tests.

@ti-chi-bot ti-chi-bot Bot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 18, 2026
@pantheon-ai

pantheon-ai Bot commented Mar 18, 2026 •

Copy link
Copy Markdown

Review failed due to infrastructure/execution failure after retries. Please re-trigger review.

ℹ️ Learn more details on Pantheon AI.

@ti-chi-bot ti-chi-bot Bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/planner SIG: Planner labels Mar 18, 2026
@coderabbitai

coderabbitai Bot commented Mar 18, 2026 •

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR standardizes EXPLAIN usage in planner tests by switching many test assertions and testdata from EXPLAIN format = 'brief' to EXPLAIN format = 'plan_tree', and normalizes the textual plan representations (removing numeric costs/Column#IDs and standardizing operator labels). It also adds planner explain-format guidelines.

Changes

Cohort / File(s) Summary
Test code — explain format migration
pkg/planner/core/casetest/correlated/correlated_test.go, pkg/planner/core/casetest/enforcempp/enforce_mpp_test.go, pkg/planner/core/casetest/hint/hint_test.go, pkg/planner/core/casetest/index/index_test.go, pkg/planner/core/casetest/indexmerge/indexmerge_intersection_test.go, pkg/planner/core/casetest/integration_test.go, pkg/planner/core/casetest/join/join_test.go, pkg/planner/core/casetest/logicalplan/logical_plan_builder_test.go, pkg/planner/core/casetest/mpp/mpp_test.go, pkg/planner/core/casetest/parallelapply/parallel_apply_test.go, pkg/planner/core/casetest/partition/...test.go, pkg/planner/core/casetest/plan_test.go, pkg/planner/core/casetest/rule/...test.go, pkg/planner/core/casetest/schema/cannot_find_column_test.go
Replaced EXPLAIN format = 'brief' with EXPLAIN format = 'plan_tree' in test queries and adjusted the captured/expected Plan strings and assertions accordingly.
Testdata — broad plan text normalization
pkg/planner/core/casetest/*/testdata/*_out.json, pkg/planner/core/casetest/*/testdata/*_xut.json (many files under correlated/, hint/, index/, indexmerge/, mpp/, partition/, rule/, schema/, vectorsearch/, etc.)
Normalized plan text across many fixtures: removed numeric costs/timings, collapsed Column#N → Column, standardized "root"/"data"/"cop[tikv]" labels and Build/Probe annotations, and flattened/reformatted nested operator lines while preserving plan topology and Result entries.
Correlated-subquery fixtures
pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.json, pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.json
Rewrote correlated-subquery plan text to use compact, normalized labels (generic Column, simplified TableReader/TableFullScan descriptors, streamlined join and aggregation node text).
MPP/enforce MPP fixtures
pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json, pkg/planner/core/casetest/mpp/testdata/integration_suite_in.json
Switched EXPLAIN entries to plan_tree and normalized plan text for MPP-related tests (joins, aggregates, exchange sender/receiver formatting).
Index & IndexMerge fixtures
pkg/planner/core/casetest/index/testdata/*, pkg/planner/core/casetest/indexmerge/testdata/*
Standardized index plan representations: removed per-operator cost markers, simplified IndexReader/IndexLookUp/IndexRangeScan and PartitionUnion formatting, and normalized partition annotations.
Rule/optimizer fixtures
pkg/planner/core/casetest/rule/testdata/*, pkg/planner/core/casetest/rule/*_test.go
Normalized rule-optimizer plan outputs (derive_topn, outer2inner, predicate pushdown/simplification): removed numeric costs, simplified column IDs, and aligned operator indentation/labels.
JSON plan execution metadata
pkg/planner/core/casetest/testdata/json_plan_suite_out.json, pkg/planner/core/casetest/testdata/json_plan_suite_xut.json
Updated executeInfo/timing fields in JSON plan fixtures (time/open/close/loops, fetch_resp_duration, rpc_info) without structural changes to plan nodes.
Documentation / guidelines
.agents/skills/tidb-test-guidelines/SKILL.md, .agents/skills/tidb-test-guidelines/references/planner-guide.md
Added planner explain-format conventions and guidance: prefer plan_tree for ordinary optimizer assertions, keep cost_trace/special formats where appropriate, and notes for test updates.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

ok-to-test, skip-issue-check

Suggested reviewers

  • qw4990
  • AilinKid
  • winoros

Poem

🐰 From brief to tree I hop with cheer,
Plans shrink their costs and IDs disappear,
Roots and nodes now neatly aligned,
A tidy plan garden, well-designed 🌿
Hopping off to write the next test case!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.80% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: switching planner core casetests to use the plan_tree explain format.
Description check ✅ Passed The PR description is comprehensive, covering the problem, solution, validation commands, and test results with all required template sections addressed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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 and usage tips.

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

Copy link
Copy Markdown
Contributor

afraid of we can hardly catch estimation change /cc @qw4990 @0xPoe

@AilinKid

Copy link
Copy Markdown
Contributor

/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 Mar 18, 2026
output[i].Plan = testdata.ConvertRowsToStrings(testKit.MustQuery("explain format = 'brief' " + tt).Rows())
output[i].Plan = testdata.ConvertRowsToStrings(testKit.MustQuery("explain format = 'plan_tree' " + tt).Rows())
})
// The test point is the row count estimation for aggregations and joins.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As it is said, we should not change it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have revert some modifies about stats/analyze test cases.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pkg/planner/core/casetest/hint/testdata/integration_suite_xut.json (1)

8-154: ⚠️ Potential issue | 🟡 Minor

Migrate the remaining TestQBHintHandlerDuplicateObjects case to plan_tree format.

This test case at line 1932 uses raw EXPLAIN (default format with numeric costs), while all 115 other test cases in the file use explain format = 'plan_tree'. The migration is incomplete and creates format inconsistency. Update the SQL to include the format specifier and convert the Plan output to plan_tree format (without numeric costs).

Current vs. expected format

Current (default format):

"SQL": "EXPLAIN WITH t AS (SELECT /*+ inl_join(e) */ em.* FROM t_employees em JOIN t_employees e WHERE em.store_id = e.department_id) SELECT * FROM t;",
"Plan": [
  "IndexJoin_18 12500.00 root  inner join, inner:IndexReader_34, ...",
  "├─TableReader_32(Build) 10000.00 root  data:TableFullScan_31",

Expected (plan_tree format):

"SQL": "explain format = 'plan_tree' WITH t AS (SELECT /*+ inl_join(e) */ em.* FROM t_employees em JOIN t_employees e WHERE em.store_id = e.department_id) SELECT * FROM t;",
"Plan": [
  "IndexJoin root  inner join, inner:IndexReader, ...",
  "├─TableReader(Build) root  data:TableFullScan",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/planner/core/casetest/hint/testdata/integration_suite_xut.json` around
lines 8 - 154, The test case TestQBHintHandlerDuplicateObjects uses default
EXPLAIN with numeric costs and operator suffixes (e.g., IndexJoin_18,
TableReader_32, cost values like 12500.00) while the file expects plan_tree
format; update the SQL string to prefix with "explain format = 'plan_tree' "
(e.g., change "EXPLAIN WITH ..." to "explain format = 'plan_tree' WITH ...") and
rewrite the corresponding "Plan" array entries to the plan_tree style by
removing numeric costs and internal suffixes (drop trailing _NN tokens and cost
numbers) and normalizing operator names to their plain forms (e.g., IndexJoin,
TableReader, TableFullScan) so the plan lines match the other plan_tree test
cases.
🧹 Nitpick comments (2)
pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json (1)

184-205: Pre-existing issue: JavaScript-style comments in JSON are non-standard.

The static analysis tool flagged parse errors due to // comment syntax (lines 184, 188, 191, 193, 195, 197, 199, 201, 203). Standard JSON does not support comments. This appears to be a pre-existing issue (the comments existed before this PR), and the test framework likely uses a lenient parser that tolerates them.

The explain format = 'plan_tree' changes themselves look correct and consistent with the PR objectives.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json`
around lines 184 - 205, Remove the JavaScript-style line comments (e.g., lines
starting with "// The most simple case.", "// Can work when there's global
limit/topn", etc.) from the JSON test data so the file is valid JSON; either
delete those comment lines or convert each comment into a proper JSON value (for
example add a parallel "_comment" property or include the text as a string
entry) and ensure the array elements remain only the SQL strings like "explain
format = 'plan_tree' with c1 as (select * from t) ...".
pkg/planner/core/casetest/ch/ch_test.go (1)

50-50: Rename briefFormat now that it points to plan_tree.

The current name says “brief” while the SQL now emits plan_tree in both tests, which makes the assertion path harder to scan. As per coding guidelines, "Code SHOULD be self-documenting through clear naming and structure."

Also applies to: 92-92

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/planner/core/casetest/ch/ch_test.go` at line 50, The variable briefFormat
should be renamed to reflect that it contains a plan_tree format string; update
the identifier briefFormat to a clearer name like planTreeFormat (or
planTreeSQL) wherever it's declared and referenced (e.g., in ch_test.go where
briefFormat := `explain format = 'plan_tree' ` and the other occurrence around
line 92), and ensure any assertions or usages that reference briefFormat are
updated to the new identifier so the code clearly documents that the SQL emits a
plan_tree format.
🤖 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/planner/core/casetest/cbotest/testdata/analyze_suite_in.json`:
- Around line 253-256: The test inputs for TestLimitIndexEstimation now use
"explain format = 'plan_tree'" (see the two SQL lines with that format) but the
corresponding golden output for that test in the paired analyze_suite_out.json
is still the old brief-format plans; update/re-record the
TestLimitIndexEstimation output block in analyze_suite_out.json so the golden
matches the new "plan_tree" explain output (regenerate the expected plans for
those two queries and replace the brief-format block with the
plan_tree-formatted plans).

In `@pkg/planner/core/casetest/testdata/json_plan_suite_out.json`:
- Line 108: Remove the timing-only "executeInfo" refresh entries injected into
the "tidb_json" suite and revert those lines in both json_plan_suite_out.json
and json_plan_suite_xut.json; specifically delete any "executeInfo" values that
are only runtime deltas like "time:..., open:..., close:...,
fetch_resp_duration:..." so the suite only contains structural plan fields that
plan_test.go asserts (id, row counts, task type, access object, operator info).

---

Outside diff comments:
In `@pkg/planner/core/casetest/hint/testdata/integration_suite_xut.json`:
- Around line 8-154: The test case TestQBHintHandlerDuplicateObjects uses
default EXPLAIN with numeric costs and operator suffixes (e.g., IndexJoin_18,
TableReader_32, cost values like 12500.00) while the file expects plan_tree
format; update the SQL string to prefix with "explain format = 'plan_tree' "
(e.g., change "EXPLAIN WITH ..." to "explain format = 'plan_tree' WITH ...") and
rewrite the corresponding "Plan" array entries to the plan_tree style by
removing numeric costs and internal suffixes (drop trailing _NN tokens and cost
numbers) and normalizing operator names to their plain forms (e.g., IndexJoin,
TableReader, TableFullScan) so the plan lines match the other plan_tree test
cases.

---

Nitpick comments:
In `@pkg/planner/core/casetest/ch/ch_test.go`:
- Line 50: The variable briefFormat should be renamed to reflect that it
contains a plan_tree format string; update the identifier briefFormat to a
clearer name like planTreeFormat (or planTreeSQL) wherever it's declared and
referenced (e.g., in ch_test.go where briefFormat := `explain format =
'plan_tree' ` and the other occurrence around line 92), and ensure any
assertions or usages that reference briefFormat are updated to the new
identifier so the code clearly documents that the SQL emits a plan_tree format.

In `@pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json`:
- Around line 184-205: Remove the JavaScript-style line comments (e.g., lines
starting with "// The most simple case.", "// Can work when there's global
limit/topn", etc.) from the JSON test data so the file is valid JSON; either
delete those comment lines or convert each comment into a proper JSON value (for
example add a parallel "_comment" property or include the text as a string
entry) and ensure the array elements remain only the SQL strings like "explain
format = 'plan_tree' with c1 as (select * from t) ...".

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 35b05abf-a3e6-4da1-95de-297d48a18357

📥 Commits

Reviewing files that changed from the base of the PR and between 943819a and e1b63bf.

📒 Files selected for processing (78)
  • pkg/planner/core/casetest/cbotest/testdata/analyze_suite_in.json
  • pkg/planner/core/casetest/cbotest/testdata/analyze_suite_out.json
  • pkg/planner/core/casetest/cbotest/testdata/analyze_suite_xut.json
  • pkg/planner/core/casetest/ch/ch_test.go
  • pkg/planner/core/casetest/ch/testdata/ch_suite_out.json
  • pkg/planner/core/casetest/ch/testdata/ch_suite_xut.json
  • pkg/planner/core/casetest/correlated/correlated_test.go
  • pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.json
  • pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.json
  • pkg/planner/core/casetest/enforcempp/enforce_mpp_test.go
  • pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json
  • pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_out.json
  • pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_xut.json
  • pkg/planner/core/casetest/hint/hint_test.go
  • pkg/planner/core/casetest/hint/testdata/integration_suite_out.json
  • pkg/planner/core/casetest/hint/testdata/integration_suite_xut.json
  • pkg/planner/core/casetest/index/index_test.go
  • pkg/planner/core/casetest/index/testdata/index_range_out.json
  • pkg/planner/core/casetest/index/testdata/index_range_xut.json
  • pkg/planner/core/casetest/index/testdata/integration_suite_out.json
  • pkg/planner/core/casetest/index/testdata/integration_suite_xut.json
  • pkg/planner/core/casetest/indexmerge/indexmerge_intersection_test.go
  • pkg/planner/core/casetest/indexmerge/testdata/index_merge_suite_out.json
  • pkg/planner/core/casetest/indexmerge/testdata/index_merge_suite_xut.json
  • pkg/planner/core/casetest/integration_test.go
  • pkg/planner/core/casetest/join/join_test.go
  • pkg/planner/core/casetest/logicalplan/logical_plan_builder_test.go
  • pkg/planner/core/casetest/mpp/mpp_test.go
  • pkg/planner/core/casetest/mpp/testdata/integration_suite_in.json
  • pkg/planner/core/casetest/mpp/testdata/integration_suite_out.json
  • pkg/planner/core/casetest/mpp/testdata/integration_suite_xut.json
  • pkg/planner/core/casetest/parallelapply/parallel_apply_test.go
  • pkg/planner/core/casetest/partition/integration_partition_test.go
  • pkg/planner/core/casetest/partition/partition_pruner_test.go
  • pkg/planner/core/casetest/partition/testdata/integration_partition_suite_in.json
  • pkg/planner/core/casetest/partition/testdata/integration_partition_suite_out.json
  • pkg/planner/core/casetest/partition/testdata/integration_partition_suite_xut.json
  • pkg/planner/core/casetest/partition/testdata/partition_pruner_in.json
  • pkg/planner/core/casetest/partition/testdata/partition_pruner_out.json
  • pkg/planner/core/casetest/partition/testdata/partition_pruner_xut.json
  • pkg/planner/core/casetest/plan_test.go
  • pkg/planner/core/casetest/planstats/plan_stats_test.go
  • pkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.json
  • pkg/planner/core/casetest/planstats/testdata/plan_stats_suite_xut.json
  • pkg/planner/core/casetest/rule/dual_test.go
  • pkg/planner/core/casetest/rule/rule_common_handle_ordering_test.go
  • pkg/planner/core/casetest/rule/rule_derive_topn_from_window_test.go
  • pkg/planner/core/casetest/rule/rule_eliminate_empty_selection_test.go
  • pkg/planner/core/casetest/rule/rule_join_reorder_test.go
  • pkg/planner/core/casetest/rule/rule_outer2inner_test.go
  • pkg/planner/core/casetest/rule/rule_predicate_pushdown_test.go
  • pkg/planner/core/casetest/rule/rule_predicate_simplification_test.go
  • pkg/planner/core/casetest/rule/testdata/derive_topn_from_window_out.json
  • pkg/planner/core/casetest/rule/testdata/derive_topn_from_window_xut.json
  • pkg/planner/core/casetest/rule/testdata/join_reorder_suite_out.json
  • pkg/planner/core/casetest/rule/testdata/join_reorder_suite_xut.json
  • pkg/planner/core/casetest/rule/testdata/outer2inner_out.json
  • pkg/planner/core/casetest/rule/testdata/outer2inner_xut.json
  • pkg/planner/core/casetest/rule/testdata/predicate_pushdown_suite_out.json
  • pkg/planner/core/casetest/rule/testdata/predicate_pushdown_suite_xut.json
  • pkg/planner/core/casetest/rule/testdata/predicate_simplification_out.json
  • pkg/planner/core/casetest/rule/testdata/predicate_simplification_xut.json
  • pkg/planner/core/casetest/schema/cannot_find_column_test.go
  • pkg/planner/core/casetest/schema/testdata/cannot_find_column_suite_out.json
  • pkg/planner/core/casetest/schema/testdata/cannot_find_column_suite_xut.json
  • pkg/planner/core/casetest/stats_test.go
  • pkg/planner/core/casetest/testdata/json_plan_suite_out.json
  • pkg/planner/core/casetest/testdata/json_plan_suite_xut.json
  • pkg/planner/core/casetest/testdata/stats_suite_out.json
  • pkg/planner/core/casetest/testdata/stats_suite_xut.json
  • pkg/planner/core/casetest/vectorsearch/testdata/ann_index_suite_in.json
  • pkg/planner/core/casetest/vectorsearch/testdata/ann_index_suite_out.json
  • pkg/planner/core/casetest/vectorsearch/testdata/ann_index_suite_xut.json
  • pkg/planner/core/casetest/vectorsearch/vector_index_test.go
  • pkg/planner/core/casetest/windows/testdata/window_push_down_suite_in.json
  • pkg/planner/core/casetest/windows/testdata/window_push_down_suite_out.json
  • pkg/planner/core/casetest/windows/testdata/window_push_down_suite_xut.json
  • pkg/planner/core/casetest/windows/window_push_down_test.go

Comment thread pkg/planner/core/casetest/cbotest/testdata/analyze_suite_in.json Outdated
Comment thread pkg/planner/core/casetest/testdata/json_plan_suite_out.json
output[i].Query = input[i]
if isSelect {
// explain the query
output[i].Result = testdata.ConvertRowsToStrings(testKit.MustQuery("explain format='brief' " + sql).Rows())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think planstats should also remain unchanged.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have reverted it.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
.agents/skills/tidb-test-guidelines/references/planner-guide.md (1)

78-80: Optional: Vary sentence beginnings for improved readability.

Three consecutive bullets begin with "For," which slightly reduces readability. Consider varying the sentence structure.

✍️ Proposed alternatives
-- For cost-oriented assertions, prefer `EXPLAIN format = 'cost_trace'` rather than `plan_tree`.
-- For plan cache tests, preserve the existing `explain for connection` / `EXPLAIN format='plan_cache'` style; do not rewrite those assertions to `plan_tree`.
-- For hint tests, preserve the existing format when the test asserts warning text, hint applicability, or other format-sensitive output. Only move a hint test to `plan_tree` when the nearby suite already uses `plan_tree` for that behavior.
+- Cost-oriented assertions should use `EXPLAIN format = 'cost_trace'` rather than `plan_tree`.
+- Plan cache tests should preserve the existing `explain for connection` / `EXPLAIN format='plan_cache'` style; do not rewrite those assertions to `plan_tree`.
+- Hint tests should preserve the existing format when asserting warning text, hint applicability, or other format-sensitive output. Only move a hint test to `plan_tree` when the nearby suite already uses `plan_tree` for that behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.agents/skills/tidb-test-guidelines/references/planner-guide.md around lines
78 - 80, The three consecutive bullet lines starting with "For" (the lines
referencing `EXPLAIN format = 'cost_trace'` vs `plan_tree`, `explain for
connection` / `EXPLAIN format='plan_cache'`, and hint test guidance) should be
rewritten to vary sentence openings for readability; edit each bullet to start
differently (e.g., "Use", "Preserve", "When dealing with") while keeping the
same technical terms and rules (`EXPLAIN format = 'cost_trace'`, `plan_tree`,
`explain for connection`, `EXPLAIN format='plan_cache'`, hint tests and
`plan_tree`) and ensure the meaning remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.agents/skills/tidb-test-guidelines/SKILL.md:
- Line 41: Revise the long bullet about pkg/planner/core/casetest to avoid
repeated sentence starts by splitting it into two bullets or rephrasing into a
single clearer sentence: first state the default change (ordinary optimizer
assertions -> EXPLAIN format = 'plan_tree'), then in a second bullet list the
exceptions (keep special-format suites unchanged — e.g.,
stats/analyze/load-stats tests like stats_test.go and planstats, cost-oriented
assertions using EXPLAIN format = 'cost_trace', plan cache tests keeping
"explain for connection" or format='plan_cache', and hint tests preserving the
nearby suite's existing format). Ensure the examples (stats_test.go, planstats,
EXPLAIN format = 'plan_tree', 'cost_trace', 'plan_cache') are retained for
clarity.

---

Nitpick comments:
In @.agents/skills/tidb-test-guidelines/references/planner-guide.md:
- Around line 78-80: The three consecutive bullet lines starting with "For" (the
lines referencing `EXPLAIN format = 'cost_trace'` vs `plan_tree`, `explain for
connection` / `EXPLAIN format='plan_cache'`, and hint test guidance) should be
rewritten to vary sentence openings for readability; edit each bullet to start
differently (e.g., "Use", "Preserve", "When dealing with") while keeping the
same technical terms and rules (`EXPLAIN format = 'cost_trace'`, `plan_tree`,
`explain for connection`, `EXPLAIN format='plan_cache'`, hint tests and
`plan_tree`) and ensure the meaning remains identical.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 14a190d3-577e-4e4e-b098-0cf77336b4d8

📥 Commits

Reviewing files that changed from the base of the PR and between e1b63bf and a5059f6.

📒 Files selected for processing (2)
  • .agents/skills/tidb-test-guidelines/SKILL.md
  • .agents/skills/tidb-test-guidelines/references/planner-guide.md

Comment thread .agents/skills/tidb-test-guidelines/SKILL.md Outdated

@coderabbitai coderabbitai Bot 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.

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 @.agents/skills/tidb-test-guidelines/references/planner-guide.md:
- Around line 75-80: The bullets repeat "For ..." at the start of many lines;
edit the listed bullets (those mentioning ordinary planner/core casetests and
`EXPLAIN format = 'plan_tree'`, explain-format-specific suites,
stats/analyze/load-stats-related tests, cost-oriented assertions preferring
`EXPLAIN format = 'cost_trace'`, plan cache tests preserving `explain for
connection` / `EXPLAIN format='plan_cache'`, and hint tests) to vary sentence
openings for readability—use alternatives like "Prefer", "Keep", "Do not",
"Avoid switching", or "When asserting" so each bullet begins differently while
preserving the exact technical guidance and quoted formats (`plan_tree`,
`cost_trace`, `plan_cache`, `EXPLAIN ANALYZE`) unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8c3229d6-c856-4e2d-af84-76fe3335124c

📥 Commits

Reviewing files that changed from the base of the PR and between 6264047 and 15ffefa.

📒 Files selected for processing (2)
  • .agents/skills/tidb-test-guidelines/SKILL.md
  • .agents/skills/tidb-test-guidelines/references/planner-guide.md

Comment thread .agents/skills/tidb-test-guidelines/references/planner-guide.md
@ti-chi-bot

ti-chi-bot Bot commented Mar 18, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, 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 Mar 18, 2026
@ti-chi-bot

ti-chi-bot Bot commented Mar 18, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-03-18 08:13:16.052073856 +0000 UTC m=+344723.139731383: ☑️ agreed by AilinKid.
  • 2026-03-18 09:12:13.537175362 +0000 UTC m=+348260.624832899: ☑️ agreed by qw4990.

@hawkingrei

Copy link
Copy Markdown
Contributor 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 Mar 18, 2026
@codecov

codecov Bot commented Mar 18, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.5341%. Comparing base (102f7af) to head (d5b3c78).
⚠️ Report is 10 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #67113        +/-   ##
================================================
- Coverage   77.7098%   77.5341%   -0.1757%     
================================================
  Files          2016       1936        -80     
  Lines        552001     541048     -10953     
================================================
- Hits         428959     419497      -9462     
- Misses       121300     121545       +245     
+ Partials       1742          6      -1736     
Flag Coverage Δ
integration 41.0069% <ø> (-7.1199%) ⬇️
unit 76.5774% <ø> (+0.3260%) ⬆️

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

Components Coverage Δ
dumpling 57.0098% <ø> (ø)
parser ∅ <ø> (∅)
br 48.8645% <ø> (-11.9500%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (1)
pkg/planner/core/casetest/hint/testdata/integration_suite_xut.json (1)

8-153: Consider a small companion cost/cardinality-sensitive suite for key hint paths.

plan_tree improves stability, but it won’t surface estimation/cost drift. Keeping a small targeted companion (e.g., cost-trace-focused cases) would preserve that signal.

Based on learnings: Applies to pkg/planner/** : For planner rule or logical/physical plan changes, perform targeted planner unit tests and update rule testdata when needed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/planner/core/casetest/hint/testdata/integration_suite_xut.json` around
lines 8 - 153, The review asks to add a small companion
cost/cardinality-sensitive test suite to catch estimation/cost regressions that
plan_tree doesn't reveal; update pkg/planner tests by adding targeted planner
unit tests (cost-trace-focused cases) alongside existing rule/plan tests and
expand the testdata (e.g., the hint integration JSON used in
pkg/planner/core/casetest/hint/testdata/integration_suite_xut.json) to include
scenarios that exercise cost estimates and cardinality-sensitive paths; when you
change planner rules or logical/physical plan code (e.g., functions/classes that
implement planner rules or cost estimation), add or update unit tests
referencing those symbols so the rule testdata and expected cost traces are
updated accordingly.
🤖 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/planner/core/casetest/hint/testdata/integration_suite_xut.json`:
- Around line 8-153: The review asks to add a small companion
cost/cardinality-sensitive test suite to catch estimation/cost regressions that
plan_tree doesn't reveal; update pkg/planner tests by adding targeted planner
unit tests (cost-trace-focused cases) alongside existing rule/plan tests and
expand the testdata (e.g., the hint integration JSON used in
pkg/planner/core/casetest/hint/testdata/integration_suite_xut.json) to include
scenarios that exercise cost estimates and cardinality-sensitive paths; when you
change planner rules or logical/physical plan code (e.g., functions/classes that
implement planner rules or cost estimation), add or update unit tests
referencing those symbols so the rule testdata and expected cost traces are
updated accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 88844cad-b6b8-4dcd-87f7-c65c53b75c10

📥 Commits

Reviewing files that changed from the base of the PR and between 15ffefa and 64b450e.

📒 Files selected for processing (3)
  • pkg/planner/core/casetest/hint/testdata/integration_suite_in.json
  • pkg/planner/core/casetest/hint/testdata/integration_suite_out.json
  • pkg/planner/core/casetest/hint/testdata/integration_suite_xut.json

@hawkingrei

Copy link
Copy Markdown
Contributor Author

/retest

5 similar comments
@hawkingrei

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

Copy link
Copy Markdown
Contributor Author

/retest

@hawkingrei

Copy link
Copy Markdown
Contributor Author

/ok-to-test

@ti-chi-bot ti-chi-bot Bot added the ok-to-test Indicates a PR is ready to be tested. label Mar 18, 2026
@hawkingrei

Copy link
Copy Markdown
Contributor Author

/retest

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

Labels

approved lgtm ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants