-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
util/ranger: add missing Selection
for range scan from like
on PAD SPACE column | tidb-test=pr/2251
#48845
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #48845 +/- ##
================================================
+ Coverage 71.0004% 72.7255% +1.7251%
================================================
Files 1367 1392 +25
Lines 404899 411851 +6952
================================================
+ Hits 287480 299521 +12041
+ Misses 97382 93459 -3923
+ Partials 20037 18871 -1166
Flags with carried forward coverage won't be shown. Click here to find out more.
|
/test check-dev2 |
@time-and-fate: The specified target(s) for
Use In 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/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tangenta, winoros The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
// the `like` function. Therefore, a Selection is needed to filter the data. | ||
// Since all collations, except for binary, implemented in tidb are PAD SPACE collations for now, we use a simple | ||
// collation != binary check here. | ||
if collation != charset.CollationBin { |
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.
What about make PAD SPACE as a new attribute for collation, so that we won't import bugs when a new NO PAD collation is added?
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.
Yes. But I think it's better for SQL team to add this into collation-related packages, especially when they start to implement NO PAD collations. I added an isPadSpaceCollation
function in #48984 since we need the same check there.
This won't cause bugs. If we use current logic when NO PAD collation is added, there will be unnecessary Selection
, that's not perfect but won't cause bugs.
…D SPACE column (pingcap#48845) ref pingcap#48181, close pingcap#48821
UPDATE
Note that this fix introduced a very subtle regression, and it's not covered by any existing test cases. We'll try to fix this later.
In brief, if you are accessing a multi-column index using a multi-column range, and some columns of the ranges come from
like
function that doesn't contain%
, and it's a binary collation, then after this PR, tidb may be able to make use of fewer columns to do the range scan.Example:
Before (v7.5.0):
Now:
What problem does this PR solve?
Issue Number: close #48821 ref #48181
What changed and how does it work?
As the title says.
And please read the comments in the code.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.