-
Notifications
You must be signed in to change notification settings - Fork 6.1k
planner, core: implement partial order TopN attach2Task and partial order flow #65799
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Skipping CI for Draft Pull Request. |
|
Hi @elsa0520. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions 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. |
|
/ok-to-test |
There was a problem hiding this 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 implements core planner changes for partial order TopN optimization with prefix indexes, enabling the optimizer to use prefix indexes for ORDER BY ... LIMIT queries to avoid full table scans.
Changes:
- Extended
PhysicalTopNandPhysicalLimitoperators with partial order fields (PartialOrderedLimit,PrefixColID,PrefixLen) - Added
PartialOrderInfostructure toPhysicalPropertyto track partial order requirements through the planning pipeline - Implemented two-layer execution mode (TiDB TopN + TiKV Limit) and one-layer mode (TiDB TopN only) in
handlePartialOrderTopN - Modified
matchPartialOrderPropertyto validate and set prefix index parameters - Updated
GetOriginalPhysicalIndexScansignature to acceptneedKeepOrderparameter handling both normal and partial order cases
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/planner/property/physical_property.go | Adds PartialOrderInfo structure with prefix column tracking fields and helper methods (NeedKeepOrder, GetSortDesc, GetSortItemsForKeepOrder) |
| pkg/planner/core/task.go | Implements handlePartialOrderTopN for two-layer/one-layer plan generation and estimateMaxXForPartialOrder stub |
| pkg/planner/core/operator/physicalop/physical_topn.go | Adds partial order fields to PhysicalTopN and updates MemoryUsage, ExplainInfo methods |
| pkg/planner/core/operator/physicalop/physical_limit.go | Adds partial order fields to PhysicalLimit and updates MemoryUsage, ExplainInfo methods |
| pkg/planner/core/operator/physicalop/physical_index_scan.go | Updates GetOriginalPhysicalIndexScan to accept needKeepOrder parameter and use GetSortDesc() for both cases |
| pkg/planner/core/find_best_task.go | Refactors matchPartialOrderProperty to update PartialOrderInfo fields directly; updates convertToIndexScan to use NeedKeepOrder() and GetSortItemsForKeepOrder() |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #65799 +/- ##
================================================
- Coverage 77.7931% 77.5623% -0.2309%
================================================
Files 1999 1923 -76
Lines 544702 541027 -3675
================================================
- Hits 423741 419633 -4108
- Misses 119297 121346 +2049
+ Partials 1664 48 -1616
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this 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 6 out of 6 changed files in this pull request and generated 8 comments.
Co-authored-by: Copilot <[email protected]>
|
/retest |
|
/unhold |
|
/hold |
|
@pantheon-bot please review |
|
P2: Partial-order TopN can produce incorrect results when enabled Severity: P2 because the optimization is opt-in (default Issue: The planner constructs a partial-order Evidence:
Fix: Don't push down the limit until TiKV supports partial-ordered limit semantics, or extend the protobuf to include prefix fields and implement the "read N then continue until prefix changes" behavior in TiKV. P2: Test incompatibility with new enum sysvar Severity: P2 because it's a test consistency issue, not a production runtime breakage (invalid Issue: The sysvar Evidence:
Fix: Update P2: AccessPath.Clone() drops ForcePartialOrder flag Severity: P2 because the reachability is unclear (flag is only set during skyline pruning for partial-order TopN, which is gated behind opt-in feature), but it's a completeness bug that could cause inconsistent hint enforcement. Issue: The Evidence:
Fix: Add |
3f0df54 to
5af179f
Compare
The connect part code not include this PR because the tipb change has not been merged. |
The test has been changed in this PR |
Has been changed in PR |
|
/test mysql-test |
|
@elsa0520: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
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. |
|
@elsa0520: The specified target(s) for Use DetailsIn response to this:
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. |
|
/test mysql-test |
|
@elsa0520: The specified target(s) for Use DetailsIn response to this:
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. |
|
/unhold |
|
/retest |
|
@elsa0520: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qw4990, winoros, yudongusa The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: close #65813
Problem Summary:
Currently, TiDB's optimizer cannot effectively utilize prefix indexes for
ORDER BY ... LIMITqueries. When a query usesORDER BYon a column with a prefix index, the optimizer performs a full table scan, leading to significant performance issues for large tables.What changed and how does it work?
1. Extended Data Structures
Added partial order support to physical operators:
PhysicalTopN/PhysicalLimit
2. Planner Flow
The optimization is integrated into the existing planner pipeline:
3. Key Functions
matchPartialOrderProperty(): Validates prefix index matchPrefixColIDandPrefixLeninPartialOrderInfohandlePartialOrderTopN(): Generates execution plan4. Two-Layer vs One-Layer Plans
Two-layer mode :
One-layer mode (when
IndexPlanFinished=trueor hasRootTaskConds):5. session variable behavior
tidb_opt_partial_ordered_index_for_topn : range: disable, cost.
use indexhint and thecostparameter together. To allow users to specify partial order + index.2.1 :cost + use index + match partial order property: force use the index with partial order optimize
2.2: cost + use index + not match partial order property: degenerate into normal index use behavior without considering partial order optimization.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.