Skip to content

range: wrongly skip the candidate in the extractBestCNFItemRanges#62585

Merged
ti-chi-bot[bot] merged 21 commits intopingcap:masterfrom
hawkingrei:62547
Oct 22, 2025
Merged

range: wrongly skip the candidate in the extractBestCNFItemRanges#62585
ti-chi-bot[bot] merged 21 commits intopingcap:masterfrom
hawkingrei:62547

Conversation

@hawkingrei
Copy link
Copy Markdown
Member

@hawkingrei hawkingrei commented Jul 23, 2025

What problem does this PR solve?

Issue Number: close #62547

Problem Summary:

What changed and how does it work?

func extractBestCNFItemRanges(sctx *rangerctx.RangerContext, conds []expression.Expression, cols []*expression.Column,
lengths []int, rangeMaxSize int64, convertToSortKey bool) (*cnfItemRangeResult, []*valueInfo, error) {
if len(conds) < 2 {
return nil, nil, nil
}
var bestRes *cnfItemRangeResult
columnValues := make([]*valueInfo, len(cols))
for i, cond := range conds {
tmpConds := []expression.Expression{cond}
colSets := expression.ExtractColumnSet(cond)
if colSets.Len() == 0 {
continue
}
// When we build ranges for the CNF item, we choose not to merge consecutive ranges because we hope to get point
// ranges here. See https://github.com/pingcap/tidb/issues/41572 for more details.
//
// Here is an example. Assume that the index is `idx(a,b,c)` and the condition is `((a,b) in ((1,1),(1,2)) and c = 1`.
// We build ranges for `(a,b) in ((1,1),(1,2))` and get `[1 1, 1 1] [1 2, 1 2]`, which are point ranges and we can
// append `c = 1` to the point ranges. However, if we choose to merge consecutive ranges here, we get `[1 1, 1 2]`,
// which are not point ranges, and we cannot append `c = 1` anymore.
res, err := detachCondAndBuildRange(sctx, tmpConds, cols, lengths, rangeMaxSize, convertToSortKey, false)
if err != nil {
return nil, nil, err
}
if len(res.Ranges) == 0 {
return &cnfItemRangeResult{rangeResult: res, offset: i}, nil, nil
}
// take the union of the two columnValues
columnValues = unionColumnValues(columnValues, res.ColumnValues)
if len(res.AccessConds) == 0 || len(res.RemainedConds) > 0 {
continue
}
curRes := getCNFItemRangeResult(sctx, res, i)
bestRes = mergeTwoCNFRanges(sctx, cond, bestRes, curRes)
}
if bestRes != nil && bestRes.rangeResult != nil {
bestRes.rangeResult.IsDNFCond = false
}
return bestRes, columnValues, nil
}

image

The implementation here has an issue. If a remaining condition is found in extractBestCNFItemRanges, it will not be considered in the final range, which is incorrect.

for example

create table t2 (a varchar(10), b varchar(10), c varchar(10), d varchar(10), index idx(a(2), b(2), c(2)));

select * from t2 where (a, b) in (('aaa', 'bbb'), ('ccc', 'ddd')) and c = 'eee';

Because his index is a very restrictive condition, selection filtering is needed to make good use of this index. At this time, this range must be retained instead of being excluded just because it also contains remaining conditions.

before this PR

+---------------------------+---------+-----------+------------------------------+----------------------------------------------------------------------------------------------------------------------------+
| id                        | estRows | task      | access object                | operator info                                                                                                              |
+---------------------------+---------+-----------+------------------------------+----------------------------------------------------------------------------------------------------------------------------+
| IndexLookUp_12            | 1.00    | root      |                              |                                                                                                                            |
| ├─IndexRangeScan_9(Build) | 20.00   | cop[tikv] | table:t2, index:idx(a, b, c) | range:["aa","aa"], ["cc","cc"], keep order:false, stats:pseudo                                                             |
| └─Selection_11(Probe)     | 1.00    | cop[tikv] |                              | eq(test.t2.c, "eee"), or(and(eq(test.t2.a, "aaa"), eq(test.t2.b, "bbb")), and(eq(test.t2.a, "ccc"), eq(test.t2.b, "ddd"))) |
|   └─TableRowIDScan_10     | 20.00   | cop[tikv] | table:t2                     | keep order:false, stats:pseudo                                                                                             |
+---------------------------+---------+-----------+------------------------------+----------------------------------------------------------------------------------------------------------------------------+

after this pr

+---------------------------+---------+-----------+------------------------------+----------------------------------------------------------------------------------------------------------------------------+
| id                        | estRows | task      | access object                | operator info                                                                                                              |
+---------------------------+---------+-----------+------------------------------+----------------------------------------------------------------------------------------------------------------------------+
| IndexLookUp_12            | 1.00    | root      |                              |                                                                                                                            |
| ├─IndexRangeScan_9(Build) | 1.25    | cop[tikv] | table:t2, index:idx(a, b, c) | range:["aa" "bb" "ee","aa" "bb" "ee"], ["cc" "dd" "ee","cc" "dd" "ee"], keep order:false, stats:pseudo                     |
| └─Selection_11(Probe)     | 1.00    | cop[tikv] |                              | eq(test.t2.c, "eee"), or(and(eq(test.t2.a, "aaa"), eq(test.t2.b, "bbb")), and(eq(test.t2.a, "ccc"), eq(test.t2.b, "ddd"))) |
|   └─TableRowIDScan_10     | 1.25    | cop[tikv] | table:t2                     | keep order:false, stats:pseudo                                                                                             |
+---------------------------+---------+-----------+------------------------------+----------------------------------------------------------------------------------------------------------------------------+

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 release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed do-not-merge/needs-tests-checked size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/needs-tests-checked do-not-merge/needs-triage-completed labels Jul 23, 2025
@hawkingrei hawkingrei marked this pull request as draft July 23, 2025 13:07
@ti-chi-bot ti-chi-bot Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 23, 2025
@ti-chi-bot ti-chi-bot Bot added sig/planner SIG: Planner 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 Sep 19, 2025
@hawkingrei hawkingrei changed the title ranger: get better range in the extractBestCNFItemRanges planner: merge Or predicate and eq condition in the datasource Sep 19, 2025
@hawkingrei hawkingrei marked this pull request as ready for review September 19, 2025 10:02
@ti-chi-bot ti-chi-bot Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2025
@guo-shaoge guo-shaoge self-requested a review October 14, 2025 12:14
@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 Oct 16, 2025
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>
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>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
@hawkingrei
Copy link
Copy Markdown
Member Author

/hold

@guo-shaoge PTAL

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

ti-chi-bot Bot commented Oct 22, 2025

@hawkingrei: 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
non-block-pull-check-next-gen 0a2f953 link false /test pull-check-next-gen

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.

@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 Oct 22, 2025
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

@hawkingrei
Copy link
Copy Markdown
Member Author

/unhold

@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 Oct 22, 2025
@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 Oct 22, 2025
@ti-chi-bot ti-chi-bot Bot merged commit 1d746d8 into pingcap:master Oct 22, 2025
29 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 Oct 22, 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: #64091.
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 Oct 22, 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: #64092.
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 Oct 22, 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: #64093.
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.

@hawkingrei
Copy link
Copy Markdown
Member Author

/cherrypick release-nextgen-20251011

@ti-chi-bot
Copy link
Copy Markdown
Member

@hawkingrei: new pull request created to branch release-nextgen-20251011: #64416.

Details

In response to this:

/cherrypick release-nextgen-20251011

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.

hawkingrei added a commit to ti-chi-bot/tidb that referenced this pull request Dec 2, 2025
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
hawkingrei added a commit to ti-chi-bot/tidb that referenced this pull request Dec 3, 2025
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@hawkingrei hawkingrei deleted the 62547 branch December 29, 2025 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Only partial column is used for composite index scan under mixed single-column and multi-column conditions

4 participants