-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
ddl,util: modify the collation of the column of multi-valued index to "binary" #46993
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #46993 +/- ##
================================================
- Coverage 72.9609% 72.7765% -0.1845%
================================================
Files 1338 1359 +21
Lines 399186 406172 +6986
================================================
+ Hits 291250 295598 +4348
- Misses 89120 91855 +2735
+ Partials 18816 18719 -97
Flags with carried forward coverage won't be shown. Click here to find out more.
|
c44df02
to
75c5d43
Compare
/retest |
83016b7
to
5fa5e95
Compare
/retest |
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
5fa5e95
to
815763b
Compare
/retest |
1 similar comment
/retest |
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.
LGTM
/hold This PR will make |
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, zimulala 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 |
/unhold I think it's a bug of dist task framework. It occurs less frequently in master, but I think it's still possible. I'll confirm it with the developer of dist task framework. |
The reported bug (importing succeed with 0 row) has been fixed by #46957 (though, maybe not intentional). |
/retest |
2 similar comments
/retest |
/retest |
What problem does this PR solve?
Issue Number: close #46717
What is changed and how it works?
This PR changes the collation of the column of multi-valued index to "binary". I think it's a better way to solve #46717. It doesn't need to modify tipb/tikv and the influence is much more limited.
Background
utf8mb4_bin
will trim the tailing whitespace, the "restored data" of theutf8mb4_bin
string is an integer, which represents the count of tailing whitespaces.utf8mb4_bin
collation, but the restored data is not written and the tailing whitespaces are not trimmed. When theTiKV
is about to restore the original value from the index, it'll fail to read the "restored data" and report an error.This issue will only happen in compound index which contains at least one string column (which will actually write restored data), because when the index value is too short, both TiDB and TiKV will just regard it as an "old format" and skip the restore logic, then nothing is reported.
Q&A
columnInfo
#46717? By setting the collation to "binary", TiKV will not try to read the restored value.Another question is, why it's a better solution than pingcap/tipb#317 tikv/tikv#15537 #46718 ?
When I was drafting pingcap/tipb#317 tikv/tikv#15537 #46718, I didn't fully understand the design of original multi-valued index type. Now I'm pretty confident about this PR 👍 .
Check List
Tests
You can test whether #46717 is solved by using the following SQL:
I'll add them to
tidb-test
later, as I want to test across tidb and tikv 🤔 . (I'm not sure whethertests/integrationtest
will run with TiKV after merged. I'll confirm later. If so,tests/integrationtest
looks better).Release note