Skip to content

planner: fix wrong TopN's ByItem with expression.ScalarFunction when to PushDownTopN#60822

Merged
ti-chi-bot[bot] merged 31 commits into
pingcap:masterfrom
hawkingrei:60655
May 7, 2025
Merged

planner: fix wrong TopN's ByItem with expression.ScalarFunction when to PushDownTopN#60822
ti-chi-bot[bot] merged 31 commits into
pingcap:masterfrom
hawkingrei:60655

Conversation

@hawkingrei
Copy link
Copy Markdown
Member

@hawkingrei hawkingrei commented Apr 24, 2025

What problem does this PR solve?

Issue Number: close #60655

Problem Summary:

In the #40593,

There is a column in topN.ByItems is generated by proj, when topN is pushed down below proj, topN cannot obtain the column from datasource, to fix this, we can check whether topN.ByItems contains a column(with ID=0) generated by proj, if so, proj will prevent the optimizer from pushing topN down.

  1. A column with ID=0 indicates that the column cannot be resolved by data source.
  2. To determine whether a column is generated by projection, we can check whether the child of proj contains the column.

What changed and how does it work?

If you find that the TopN you can push down cannot actually be pushed down, you should use the original TopN to assemble into a structure of TopN -> Projection, rather than using the processed TopN to assemble.

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.

planner: fix wrong TopN's ByItem with expression.ScalarFunction when to PushDownTopN

修复在 TopN 下推过程中错误生成带表达式的TopN排序项 

@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-triage-completed do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 24, 2025
@hawkingrei
Copy link
Copy Markdown
Member Author

/check-issue-triage-complete

@hawkingrei
Copy link
Copy Markdown
Member Author

/test fast_tiprow_test

@tiprow
Copy link
Copy Markdown

tiprow Bot commented Apr 28, 2025

@ti-chi-bot[bot]: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test fast_test_tiprow
  • /test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

  • tidb_parser_test
Details

In response to this:

@hawkingrei: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test check-dev
/test check-dev2
/test mysql-test
/test pull-br-integration-test
/test pull-integration-ddl-test
/test pull-integration-e2e-test
/test pull-lightning-integration-test
/test pull-mysql-client-test
/test pull-unit-test-ddlv1
/test unit-test

The following commands are available to trigger optional jobs:

/test pingcap/tidb/canary_ghpr_unit_test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-integration-python-orm-test
/test pull-next-gen-real-tikv-test
/test pull-sqllogic-test
/test pull-tiflash-test

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test
pingcap/tidb/pull_integration_ddl_test
pingcap/tidb/pull_integration_e2e_test
pingcap/tidb/pull_mysql_client_test

In response to this:

/test fast_test_tiprow

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.

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 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 Apr 29, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.5929%. Comparing base (a0293bf) to head (ecb5e20).
Report is 11 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #60822        +/-   ##
================================================
+ Coverage   73.1429%   73.5929%   +0.4499%     
================================================
  Files          1722       1722                
  Lines        476948     481220      +4272     
================================================
+ Hits         348854     354144      +5290     
+ Misses       106703     105535      -1168     
- Partials      21391      21541       +150     
Flag Coverage Δ
integration 42.6384% <63.6363%> (?)
unit 72.3697% <100.0000%> (+0.0125%) ⬆️

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

Components Coverage Δ
dumpling 52.7804% <ø> (ø)
parser ∅ <ø> (∅)
br 46.4900% <ø> (-0.9561%) ⬇️
🚀 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

@hawkingrei hawkingrei changed the title planner: fix wrong schema when to PushDownPush planner: fix wrong schema when to PushDownTopN Apr 29, 2025
@hawkingrei hawkingrei requested a review from Copilot April 29, 2025 07:58
@pingcap pingcap deleted a comment from ti-chi-bot Bot Apr 29, 2025
@pingcap pingcap deleted a comment from tiprow Bot Apr 29, 2025
Copy link
Copy Markdown

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 PR addresses a bug that causes the wrong schema to be used when pushing down TopN into a projection. Key changes include:

  • Removing an extra blank line in optimizer.go.
  • Adding a new unit test to verify the TopN push down behavior in projection.
  • Updating the push down logic in logical_projection.go with an additional check using a new helper method (isSetTopNChild).

Reviewed Changes

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

File Description
pkg/planner/core/optimizer.go Removed an unnecessary blank line to improve code clarity.
pkg/planner/core/operator/logicalop/logicalop_test/logical_operator_test.go Added unit tests that verify the correct behavior for projection push down TopN.
pkg/planner/core/operator/logicalop/logical_projection.go Updated TopN push down logic and added the helper method isSetTopNChild to better determine if the TopN child is set appropriately.
Files not reviewed (3)
  • pkg/planner/core/operator/logicalop/logicalop_test/BUILD.bazel: Language not supported
  • tests/integrationtest/r/planner/core/plan.result: Language not supported
  • tests/integrationtest/t/planner/core/plan.test: Language not supported
Comments suppressed due to low confidence (1)

pkg/planner/core/operator/logicalop/logical_projection.go:228

  • [nitpick] The helper function name 'isSetTopNChild' is ambiguous. Consider renaming it to something like 'areTopNByItemsInChildSchema' so that its purpose is clearer.
if !p.Children()[0].Schema().Contains(col) {

@pingcap pingcap deleted a comment from tiprow Bot Apr 29, 2025
@pingcap pingcap deleted a comment from ti-chi-bot Bot Apr 29, 2025
@ti-chi-bot ti-chi-bot Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 29, 2025
@hawkingrei hawkingrei requested a review from Copilot April 29, 2025 08:18
Copy link
Copy Markdown

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 PR fixes an issue with the TopN push down logic in the planner when dealing with projections that generate columns with ID=0. The changes update the push-down behavior in LogicalProjection and add new unit tests to verify the correct operator tree under different configuration scenarios.

  • Update LogicalProjection.PushDownTopN to create a TopN operator without by-items when a projection-generated column is detected.
  • Add unit tests in logical_operator_test to ensure the correct handling of TopN push down.

Reviewed Changes

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

File Description
pkg/planner/core/operator/logicalop/logicalop_test/logical_operator_test.go Adds tests validating the TopN push down logic with projections.
pkg/planner/core/operator/logicalop/logical_projection.go Adjusts the push-down logic to generate a new TopN operator when the child schema is missing a projection-generated column.
Files not reviewed (3)
  • pkg/planner/core/operator/logicalop/logicalop_test/BUILD.bazel: Language not supported
  • tests/integrationtest/r/planner/core/plan.result: Language not supported
  • tests/integrationtest/t/planner/core/plan.test: Language not supported
Comments suppressed due to low confidence (2)

pkg/planner/core/operator/logicalop/logical_projection.go:228

  • The condition checking for a projection-generated column (col.ID == 0) may inadvertently trigger the creation and push down of a TopN operator without by-items even for cases that are not truly projection-related. Consider refining this condition with additional checks to ensure that only columns generated by projection result in this behavior.
if !p.Children()[0].Schema().Contains(col) {

pkg/planner/core/operator/logicalop/logicalop_test/logical_operator_test.go:128

  • [nitpick] Consider adding more granular assertions on the plan structure rather than relying on exact string matching, to reduce brittleness if plan formatting changes.
AS s 

@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

2 similar comments
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

hawkingrei added 2 commits May 7, 2025 14:59
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
@ti-chi-bot ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 7, 2025
hawkingrei added 4 commits May 7, 2025 15:26
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

Comment thread pkg/planner/core/casetest/vectorsearch/testdata/ann_index_suite_in.json Outdated
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented May 7, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, winoros

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 May 7, 2025
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented May 7, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-05-07 07:21:07.397709417 +0000 UTC m=+426114.580612676: ☑️ agreed by AilinKid.
  • 2025-05-07 16:40:32.145241422 +0000 UTC m=+459679.328144678: ☑️ agreed by winoros.

@ti-chi-bot ti-chi-bot Bot merged commit 15c9cb7 into pingcap:master May 7, 2025
24 checks passed
@hawkingrei
Copy link
Copy Markdown
Member Author

/cherrypick release-8.5
/cherrypick release-8.1
/cherrypick release-7.5

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request May 8, 2025
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Copy Markdown
Member

@hawkingrei: new pull request created to branch release-7.5: #61004.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherrypick release-8.5
/cherrypick release-8.1
/cherrypick release-7.5

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 ti-community-infra/tichi repository.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request May 8, 2025
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Copy Markdown
Member

@hawkingrei: new pull request created to branch release-8.1: #61005.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherrypick release-8.5
/cherrypick release-8.1
/cherrypick release-7.5

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 ti-community-infra/tichi repository.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request May 8, 2025
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Copy Markdown
Member

@hawkingrei: new pull request created to branch release-8.5: #61006.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherrypick release-8.5
/cherrypick release-8.1
/cherrypick release-7.5

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 ti-community-infra/tichi repository.

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

Labels

approved lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

Throw Can't find column xxx in schema Column Error when execute SQL

5 participants