Skip to content
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.

Sign up for GitHub

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

Jump to bottom

charset: add utf8_unicode_ci and utf8mb4_unicode_ci interface #18678

Merged
merged 14 commits into from
Jul 24, 2020
Merged

charset: add utf8_unicode_ci and utf8mb4_unicode_ci interface #18678

merged 14 commits into from
Jul 24, 2020

Conversation

xiongjiwei
Copy link
Contributor

@xiongjiwei xiongjiwei commented Jul 19, 2020 •
edited
Loading

What problem does this PR solve?

This is first PR of #17596

What is changed and how it works?

This PR is aim to add utf8_unicode_ci and utf8mb4_unicode_ci interface but not implement.

Tests

  • Unit test
  • Integration test

Release note

  • add utf8_unicode_ci and utf8mb4_unicode_ci interface.

@ti-srebot ti-srebot added the contribution This PR is from a community contributor. label Jul 19, 2020
@codecov
Copy link

codecov bot commented Jul 19, 2020 •
edited
Loading

Codecov Report

Merging #18678 into master will increase coverage by 0.0194%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #18678        +/-   ##
================================================
+ Coverage   79.2029%   79.2224%   +0.0194%     
================================================
  Files           542        543         +1     
  Lines        146247     146244         -3     
================================================
+ Hits         115832     115858        +26     
+ Misses        21080      21051        -29     
  Partials       9335       9335

@xiongjiwei xiongjiwei changed the title add unicode_ci interface charset: add utf8_unicode_ci and utf8mb4_unicode_ci interface Jul 20, 2020
@xiongjiwei
Copy link
Contributor Author

/run-all-tests

@xiongjiwei xiongjiwei marked this pull request as ready for review July 20, 2020 08:03
@xiongjiwei xiongjiwei requested a review from a team as a code owner July 20, 2020 08:03
@xiongjiwei xiongjiwei requested review from wshwsh12 and removed request for a team July 20, 2020 08:03
wjhuang2016
wjhuang2016 reviewed Jul 20, 2020
View reviewed changes
util/collate/unicode_ci.go Outdated Show resolved Hide resolved
util/collate/unicode_ci.go Outdated Show resolved Hide resolved
@xiongjiwei
Copy link
Contributor Author

xiongjiwei commented Jul 20, 2020 •
edited
Loading

hi, @wjhuang2016 I changed the comments. BTW, is this PR too small? and should I not just add interface but implement it

@wjhuang2016
Copy link
Member

hi, @wjhuang2016 I changed the comments. BTW, is this PR too small? and should I not just add interface but implement it

I think it's ok here. Implement an optimized algorithm may be complex and need some benchmarks.

@xiongjiwei
Copy link
Contributor Author

/run-all-tests

@xiongjiwei
Copy link
Contributor Author

@wjhuang2016 PTAL

@xiongjiwei
Copy link
Contributor Author

/rebuild

wjhuang2016
wjhuang2016 reviewed Jul 22, 2020
View reviewed changes
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 22, 2020
djshow832
djshow832 reviewed Jul 24, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

util/collate/unicode_ci.go Outdated Show resolved Hide resolved
djshow832
djshow832 approved these changes Jul 24, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 24, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 24, 2020
@djshow832
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 24, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@xiongjiwei merge failed.

@xiongjiwei
Copy link
Contributor Author

xiongjiwei commented Jul 24, 2020 •
edited
Loading

@djshow832 hi, need do something

@wjhuang2016
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit b642045 into pingcap:master Jul 24, 2020
@xiongjiwei xiongjiwei deleted the unicode-collation branch July 24, 2020 09:17
@xiongjiwei
Copy link
Contributor Author

/label needs-cherry-pick-4.0

@xiongjiwei
Copy link
Contributor Author

/run-cherry-pick

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Dec 30, 2020
@xiongjiwei @ti-srebot
cherry pick pingcap#18678 to release-4.0
f32d36e 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Dec 30, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #22099

ti-srebot added a commit that referenced this pull request Jan 27, 2021
@ti-srebot
charset: add utf8_unicode_ci and utf8mb4_unicode_ci interface (#18678) (
52a350a 
#22099)

Signed-off-by: ti-srebot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjhuang2016 wjhuang2016 wjhuang2016 left review comments

@djshow832 djshow832 djshow832 approved these changes

@wshwsh12 wshwsh12 Awaiting requested review from wshwsh12

@ti-srebot ti-srebot Awaiting requested review from ti-srebot

Assignees
No one assigned
Labels
component/expression contribution This PR is from a community contributor. sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@xiongjiwei @wjhuang2016 @djshow832 @ti-srebot @qw4990