Skip to content

planner: refresh pruned TopN schema before inline projection#67171

Merged
ti-chi-bot[bot] merged 5 commits intopingcap:masterfrom
hawkingrei:issue-65892-topn-ref-child
Mar 20, 2026
Merged

planner: refresh pruned TopN schema before inline projection#67171
ti-chi-bot[bot] merged 5 commits intopingcap:masterfrom
hawkingrei:issue-65892-topn-ref-child

Conversation

@hawkingrei
Copy link
Copy Markdown
Member

@hawkingrei hawkingrei commented Mar 20, 2026

What problem does this PR solve?

Issue Number: close #65892

Problem Summary:

LogicalTopN.PruneColumns could keep a stale schema after child pruning. When hidden sort
columns were duplicated in that stale schema, the physical TopN inline-projection resolve path
could no longer match its output schema to the child schema and failed with:

Some columns of TopN_xxx cannot find the reference from its child(ren).

What changed and how does it work?

  • refresh LogicalTopN schema from the pruned child before running inline projection
  • add a focused regression test covering the stale-schema case with duplicated hidden sort columns

Check List

Tests

  • Unit test
  • Integration test
  • Manual test
  • No need to test

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

  • Bug Fixes

    • Top‑n operator schema is now always refreshed during column pruning.
  • Tests

    • Added unit test validating schema refresh and preserved column order for top‑n pruning.
    • Added SQL regression cases, DDL/data setup, and expected outputs to cover related scenarios.
    • Adjusted test sharding configuration.
  • Chores

    • CI workflow updated to expand Bazel/Starlark file matching for automated commits and to change workflow triggers.

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. labels Mar 20, 2026
@pantheon-ai
Copy link
Copy Markdown

pantheon-ai bot commented Mar 20, 2026

Review Complete

Findings: 0 issues
Posted: 0
Duplicates/Skipped: 0

ℹ️ Learn more details on Pantheon AI.

@ti-chi-bot ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/planner SIG: Planner labels Mar 20, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 20, 2026

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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 3f13cbdc-4b92-4f77-989c-94e4ddc0d23d

📥 Commits

Reviewing files that changed from the base of the PR and between 75ae812 and a3f3304.

📒 Files selected for processing (1)
  • .github/workflows/bazel-lint-crossbuild.yml
💤 Files with no reviewable changes (1)
  • .github/workflows/bazel-lint-crossbuild.yml

📝 Walkthrough

Walkthrough

LogicalTopN.PruneColumns now always resets the operator schema (SetSchema(nil)) after pruning its child and before calling InlineProjection(...). A unit test was added and multiple regression test cases plus CI/test shard and workflow file selector updates were included for issue 65892.

Changes

Cohort / File(s) Summary
LogicalTopN schema change
pkg/planner/core/operator/logicalop/logical_top_n.go
Always clears TopN schema (SetSchema(nil)) after child PruneColumns and before InlineProjection(...) (removed conditional skip).
LogicalTopN unit test & build
pkg/planner/core/operator/logicalop/logicalop_test/logical_operator_test.go, pkg/planner/core/operator/logicalop/logicalop_test/BUILD.bazel
Adds TestLogicalTopNPruneColumnsRefreshesSchemaBeforeInlineProjection and import updates; adjusts test shard_count from 42 to 43.
Schema regression tests
pkg/planner/core/casetest/schema/cannot_find_column_test.go, pkg/planner/core/casetest/schema/testdata/cannot_find_column_suite_in.json, pkg/planner/core/casetest/schema/testdata/cannot_find_column_suite_out.json, pkg/planner/core/casetest/schema/testdata/cannot_find_column_suite_xut.json
Adds DDL/data setup and new JSON test cases for issue 65892 (new tables/views, data inserts, and expected plan/results validating TopN+join scenario).
CI workflow updates
.github/workflows/update-bazel-files.yml, .github/workflows/bazel-lint-crossbuild.yml
Adjusts Bazel file commit selector (disables shell globbing; expands repo-style globbing) and removes push trigger for master in the crossbuild workflow.

Sequence Diagram(s)

sequenceDiagram
    participant Parent
    participant TopN as LogicalTopN
    participant Child

    Parent->>TopN: PruneColumns(parentUsedCols)
    TopN->>Child: PruneColumns(snapParentUsedCols)
    Child-->>TopN: child.PruneColumns done
    TopN->>TopN: SetSchema(nil)
    TopN->>TopN: InlineProjection(snapParentUsedCols)
    TopN-->>Parent: PruneColumns returns
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • qw4990
  • guo-shaoge
  • AilinKid

Poem

🐰
I brushed the schema, soft and neat,
Cleared the traces of every feat.
TopN hops tidy, columns align,
Tests nibble carrots — all is fine. 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning All changes are scoped to fixing issue #65892: the core LogicalTopN.PruneColumns logic fix, a focused regression test, test data additions, and necessary BUILD configuration updates. Workflow changes are unrelated to the issue fix. Review the workflow changes in .github/workflows/update-bazel-files.yml and bazel-lint-crossbuild.yml to ensure they are intentional and not out of scope for this fix.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'planner: refresh pruned TopN schema before inline projection' accurately and specifically describes the main code change—refreshing the schema before inline projection.
Description check ✅ Passed The description follows the template structure with a clear Issue Number, Problem Summary, What changed section, and completed checklists. A unit test is included and all required fields are properly filled.
Linked Issues check ✅ Passed The PR directly addresses issue #65892 by fixing the stale schema problem in LogicalTopN.PruneColumns and adding a regression test that covers the duplicated hidden sort columns scenario described in the issue.

✏️ 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.

Copy link
Copy Markdown

@pantheon-ai pantheon-ai bot left a comment

Choose a reason for hiding this comment

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

✅ Code looks good. No issues found.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.4528%. Comparing base (485bd4b) to head (a3f3304).
⚠️ Report is 13 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #67171        +/-   ##
================================================
- Coverage   77.7126%   77.4528%   -0.2598%     
================================================
  Files          2016       1936        -80     
  Lines        552501     540752     -11749     
================================================
- Hits         429363     418828     -10535     
- Misses       121396     121921       +525     
+ Partials       1742          3      -1739     
Flag Coverage Δ
integration 41.0214% <100.0000%> (-7.1017%) ⬇️
unit 76.6027% <100.0000%> (+0.3616%) ⬆️

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

Components Coverage Δ
dumpling 61.5065% <ø> (+4.4967%) ⬆️
parser ∅ <ø> (∅)
br 48.7487% <ø> (-12.1171%) ⬇️
🚀 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.

@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

1 similar comment
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

Signed-off-by: Weizhen Wang <hawking.rei@gmail.com>
@ti-chi-bot ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Mar 20, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot bot commented Mar 20, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guo-shaoge, 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 20, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot bot commented Mar 20, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-03-20 08:44:25.608125203 +0000 UTC m=+519392.695782740: ☑️ agreed by qw4990.
  • 2026-03-20 08:53:51.748446681 +0000 UTC m=+519958.836104208: ☑️ agreed by guo-shaoge.

@hawkingrei
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 Mar 20, 2026
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 20, 2026
@hawkingrei
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 Mar 20, 2026
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

1 similar comment
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

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

Labels

AI-Correction Bugfix by AI approved lgtm release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Some columns of TopN cannot find the reference from its child

3 participants