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

executor, infoschema: SHOW COLLATIONS shows supported collations only #10186

Merged
merged 2 commits into from
Apr 24, 2019

Conversation

bb7133
Copy link
Member

@bb7133 bb7133 commented Apr 18, 2019

What problem does this PR solve?

This PR aims to fix 2 compatibility issues:

  • In TIDB, the SHOW CHARSET command shows only supported charsets:
mysql> show charset;
+---------+---------------+-------------------+--------+
| Charset | Description   | Default collation | Maxlen |
+---------+---------------+-------------------+--------+
| utf8    | UTF-8 Unicode | utf8_bin          |      3 |
| utf8mb4 | UTF-8 Unicode | utf8mb4_bin       |      4 |
| ascii   | US ASCII      | ascii_bin         |      1 |
| latin1  | Latin1        | latin1_bin        |      1 |
| binary  | binary        | binary            |      1 |
+---------+---------------+-------------------+--------+

But SHOW COLLATIONS shows all collations that supported in MySQL 5.7:

mysql> show collation;
+--------------------------+----------+------+---------+----------+---------+
| Collation                | Charset  | Id   | Default | Compiled | Sortlen |
+--------------------------+----------+------+---------+----------+---------+
| big5_chinese_ci          | big5     |    1 | Yes     | Yes      |       1 |
| latin2_czech_cs          | latin2   |    2 |         | Yes      |       1 |
| dec8_swedish_ci          | dec8     |    3 | Yes     | Yes      |       1 |
| cp850_general_ci         | cp850    |    4 | Yes     | Yes      |       1 |
| latin1_german1_ci        | latin1   |    5 |         | Yes      |       1 |
| hp8_english_ci           | hp8      |    6 | Yes     | Yes      |       1 |
| koi8r_general_ci         | koi8r    |    7 | Yes     | Yes      |       1 |
| latin1_swedish_ci        | latin1   |    8 | Yes     | Yes      |       1 |
| latin2_general_ci        | latin2   |    9 | Yes     | Yes      |       1 |
...
| utf8mb4_german2_ci       | utf8mb4  |  244 |         | Yes      |       1 |
| utf8mb4_croatian_ci      | utf8mb4  |  245 |         | Yes      |       1 |
| utf8mb4_unicode_520_ci   | utf8mb4  |  246 |         | Yes      |       1 |
| utf8mb4_vietnamese_ci    | utf8mb4  |  247 |         | Yes      |       1 |
+--------------------------+----------+------+---------+----------+---------+
219 rows in set (0.01 sec)

It's better for SHOW COLLATIONS to display only supported collations.

  • The Default column is wrong for some of SHOW COLLATION results:
mysql> show collation where Collation='utf8_bin';
+-----------+---------+------+---------+----------+---------+
| Collation | Charset | Id   | Default | Compiled | Sortlen |
+-----------+---------+------+---------+----------+---------+
| utf8_bin  | utf8    |   83 |         | Yes      |       1 |
+-----------+---------+------+---------+----------+---------+
1 row in set (0.00 sec)

The default collation of utf8 in TiDB is utf8_bin

What is changed and how it works?

This main change is from parser PR: pingcap/parser#295

Check List

Tests

  • Integration test

Code changes

  • Has interface methods change

Side effects

  • Breaking backward compatibility in some SHOW result

Related changes

  • Need to cherry-pick to the release branch

@morgo
Copy link
Contributor

morgo commented Apr 18, 2019

Thank you for working on this. I think it's a great enhancement!

@@ -861,7 +861,7 @@ func (e *ShowExec) fetchShowCreateDatabase() error {
}

func (e *ShowExec) fetchShowCollation() error {
collations := charset.GetCollations()
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this function in parser now?

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks reasonable, I will try to remove it in another PR, thanks

@tiancaiamao
Copy link
Contributor

LGTM

@zz-jason
Copy link
Member

@bb7133 Please merge master && resolve conflicts. BTW please add some proper labels for this PR.

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM, please fix ci

@winkyao winkyao added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 22, 2019
@tiancaiamao
Copy link
Contributor

The circle CI fail is misleading, it actually pass all tests
I remove the redundant 'parser replace check'
#10228

@codecov
Copy link

codecov bot commented Apr 24, 2019

Codecov Report

Merging #10186 into master will increase coverage by 0.0076%.
The diff coverage is 66.6666%.

@@               Coverage Diff               @@
##            master     #10186        +/-   ##
===============================================
+ Coverage   77.949%   77.9566%   +0.0076%     
===============================================
  Files          407        407                
  Lines        83543      82601       -942     
===============================================
- Hits         65121      64393       -728     
+ Misses       13591      13431       -160     
+ Partials      4831       4777        -54

@bb7133 bb7133 removed the status/DNM label Apr 24, 2019
@bb7133
Copy link
Member Author

bb7133 commented Apr 24, 2019

/run-all-tests

@bb7133
Copy link
Member Author

bb7133 commented Apr 24, 2019

PTAL @crazycs520 @zz-jason

@winkyao winkyao merged commit b2910d7 into pingcap:master Apr 24, 2019
@bb7133 bb7133 deleted the bb7133/show_collations branch December 29, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/charset status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants