Skip to content

Conversation

@terry1purcell
Copy link
Contributor

@terry1purcell terry1purcell commented Dec 19, 2025

What problem does this PR solve?

Issue Number: ref #63118

Problem Summary:

What changed and how does it work?

Both EXPLAIN FORMAT=PLAN_TREE & FORMAT=BRIEF will remove the "physical operator numbers" from the explain output - to reduce those numbers causing irrelevant changes to optimizer tests. But that does NOT apply to the "physical operator number" assigned to each column in the plan.

This PR removes the "Column#num" suffix from EXPLAIN FORMAT=PLAN_TREE.

This does not extend to FORMAT=BRIEF simply because there are already 90+ file changes in this PR. FORMAT=BRIEF can be targeted in a separate PR.

Claude code's review is here:

75 Integration Test Result Files (.result files):
All changes in these files are exclusively column suffix removals. Every change follows this
pattern:

  • Column#5 → Column
  • Column#7 → Column
  • Column#10 → Column
  • etc.

Examples from the diff:

  • HashAgg root group by:Column#5, funcs:firstrow(Column#5)->Column#5
  • HashAgg root group by:Column, funcs:firstrow(Column)->Column
  • funcs:avg(Column#9, Column#10)->Column#5
  • funcs:avg(Column, Column)->Column

Non-Test File Changes (2 files):

  1. pkg/planner/core/common_plans.go - Adds the removeColumnNumbers() function to strip column
    number suffixes in plan_tree format

Verification:
✅ CONFIRMED: The only changes to test files are column suffix removals (e.g., Column#14 →
Column)
✅ CONFIRMED: No other modifications to test content, logic, or structure
✅ CONFIRMED: All 76 test files follow the exact same pattern
The changes are clean and consistent across all test files - purely cosmetic removal of column
number suffixes with no functional changes to the tests themselves.

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.

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

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/planner SIG: Planner labels Dec 19, 2025
@tiprow
Copy link

tiprow bot commented Dec 19, 2025

Hi @terry1purcell. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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.

@ti-chi-bot
Copy link

ti-chi-bot bot commented Dec 19, 2025

[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 hawkingrei, windtalker 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

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request suppresses column numbers in plan_tree explain output format, changing references like Column#6 to simply Column throughout the execution plans. This cosmetic change aims to simplify the explain output and make it more readable by removing internal column identifiers that are not typically meaningful to end users.

Key changes:

  • Removed numeric suffixes from column references in plan tree output (e.g., Column#6->Column#7 becomes Column->Column)
  • Updated all test result files to reflect this change in explain format output
  • No actual code functionality changes - only presentation of explain plans

Reviewed changes

Copilot reviewed 77 out of 77 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/integrationtest/r/window_function.result Updated window function explain outputs to use Column instead of numbered columns
tests/integrationtest/r/util/ranger.result Updated explain outputs for index range scan queries
tests/integrationtest/r/tpch.result Updated TPC-H benchmark query explain outputs
tests/integrationtest/r/topn_pushdown.result Updated TopN pushdown explain outputs
tests/integrationtest/r/table/partition.result Updated partition-related explain outputs
tests/integrationtest/r/subquery.result Updated subquery explain outputs
tests/integrationtest/r/session/clustered_index.result Updated clustered index explain outputs
tests/integrationtest/r/select.result Updated various select query explain outputs
tests/integrationtest/r/planner/core/*.result Updated numerous planner test explain outputs across multiple subdirectories
tests/integrationtest/r/null_rejected.result Updated null rejection explain outputs
tests/integrationtest/r/infoschema/infoschema.result Updated information schema explain outputs
tests/integrationtest/r/expression/misc.result Updated expression-related explain outputs

@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.2530%. Comparing base (1e08348) to head (6a73fd7).
⚠️ Report is 6 commits behind head on master.

⚠️ Current head 6a73fd7 differs from pull request most recent head 36e3645

Please upload reports for the commit 36e3645 to get more accurate results.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #65148        +/-   ##
================================================
- Coverage   70.7452%   68.2530%   -2.4922%     
================================================
  Files          1895       1872        -23     
  Lines        517984     510232      -7752     
================================================
- Hits         366449     348249     -18200     
- Misses       127012     139565     +12553     
+ Partials      24523      22418      -2105     
Flag Coverage Δ
integration 41.5383% <100.0000%> (-6.6325%) ⬇️

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

Components Coverage Δ
dumpling 52.7568% <ø> (ø)
parser ∅ <ø> (∅)
br 38.3735% <ø> (-19.8240%) ⬇️
🚀 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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 78 out of 89 changed files in this pull request and generated no new comments.

@terry1purcell
Copy link
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 Dec 20, 2025
@tiprow
Copy link

tiprow bot commented Dec 20, 2025

@terry1purcell: 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
fast_test_tiprow 36e3645 link true /test fast_test_tiprow

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.

@ti-chi-bot
Copy link

ti-chi-bot bot commented Dec 20, 2025

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

Test name Commit Details Required Rerun command
pull-unit-test-next-gen 36e3645 link true /test pull-unit-test-next-gen
idc-jenkins-ci-tidb/unit-test 36e3645 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

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.

1 participant