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.

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

collation: add utf8mb4_zh_pinyin_tidb_as_cs collation interface #20504

Merged
merged 25 commits into from
Nov 3, 2020

Conversation

xiongjiwei
Copy link
Contributor

@xiongjiwei xiongjiwei commented Oct 19, 2020

What problem does this PR solve?

Problem Summary:

we are going to add a new collation named utf8mb4_zh_pinyin_tidb_as_cs for pinyin order, this PR is aim to add the interface for it.

What is changed and how it works?

Proposal: #19984

What's Changed:

  • move some constant and function to common file
  • a new function to avoid access collation id from mysql.collation directly

Related changes

Check List

Tests

  • Unit test
  • Integration test

Release note

  • No release note

@sre-bot
Copy link
Contributor

sre-bot commented Oct 19, 2020

@ti-challenge-bot
Copy link

Usage:

      /command [argument]

The commands are:

      /ping            ping the bot
      /pick-up      pick up the issue
      /give-up      give up the issue
      /reward score      reward the score to PR


More Details

You can find more details on ti-challenge-bot-docs.

If you think there is a problem, you can report bug on report-a-bug.

If you think the bot is dead, please contact Rustin-Liu to fix it.

@xiongjiwei
Copy link
Contributor Author

/run-all-tests

1 similar comment
@xiongjiwei
Copy link
Contributor Author

/run-all-tests

@xiongjiwei xiongjiwei changed the title collation: add utf8mb4_general_zh_ci pinyin-order interface collation: add utf8mb4_zh_pinyin_tidb_as_cs collation interface Oct 22, 2020
@xiongjiwei xiongjiwei marked this pull request as ready for review October 22, 2020 12:44
@xiongjiwei xiongjiwei requested a review from a team as a code owner October 22, 2020 12:44
@xiongjiwei xiongjiwei requested review from XuHuaiyu and removed request for a team October 22, 2020 12:44
if ok {
return v
if coll, err := charset.GetCollationByID(int(collate.RestoreCollationIDIfNeeded(c))); err == nil {
return coll.Name
}
logutil.BgLogger().Warn(
Copy link
Member

Choose a reason for hiding this comment

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

log the err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

Choose a reason for hiding this comment

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

You should log it as a warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The err and the original log say the same thing and the original log have more detail. It's better to use the original code.

}
logutil.BgLogger().Warn(
"Unable to get collation name from ID, use name of the default collation instead",
zap.Int32("id", c),
zap.Int("default collation ID", mysql.DefaultCollationID),
zap.String("default collation", mysql.DefaultCollationName),
)

Copy link
Member

Choose a reason for hiding this comment

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

this line can be removed.


package collate

type zhPinyinTiDBASCS struct {
Copy link
Member

Choose a reason for hiding this comment

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

What does 'ASCS' mean exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rename it to zhPinyinTiDBASCSCollator and add comments. 'ASCS' for accent-sensitive and case-sensitive

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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 2, 2020
Copy link
Member

@bb7133 bb7133 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 status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 3, 2020
@bb7133
Copy link
Member

bb7133 commented Nov 3, 2020

/merge

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

/run-all-tests

@ti-srebot
Copy link
Contributor

@xiongjiwei merge failed.

@bb7133
Copy link
Member

bb7133 commented Nov 3, 2020

/run-check_dev
/run-unit-test

@wjhuang2016
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@xiongjiwei merge failed.

@wjhuang2016
Copy link
Member

/run-unit-test

@wjhuang2016 wjhuang2016 merged commit f1c464a into pingcap:master Nov 3, 2020
@xiongjiwei xiongjiwei deleted the pinyin-order branch November 3, 2020 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants