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

table, infoschema, executor: fix Charset/Collation shown in column desc #10007

Merged
merged 4 commits into from
Apr 9, 2019

Conversation

bb7133
Copy link
Member

@bb7133 bb7133 commented Apr 2, 2019

What problem does this PR solve?

With the help of a parser PR(pingcap/parser#270), this request aims to fix:

  1. Collate descriptions displayed in SHOW FULL COLUMNS statement. It will: close compatibility: “show full columns” prints wrong Collation for date, datatime, timestamp columns #9807, close Need to check and fix more types in show (full) columns #9837.

  2. Some corner cases in information_schema.COLUMNS table, take the following statements as instance:

mysql> create table t1(a tinytext);
Query OK, 0 rows affected (0.08 sec)

In MySQL 5.7:

mysql> SELECT column_name, character_set_name, collation_name FROM information_schema.COLUMNS WHERE table_schema = "test" AND table_name = "t1";
+-------------+--------------------+--------------------+
| column_name | character_set_name | collation_name     |
+-------------+--------------------+--------------------+
| a           | utf8mb4            | utf8mb4_general_ci |
+-------------+--------------------+--------------------+
1 row in set (0.03 sec)

In TiDB master before this PR:

mysql> SELECT column_name, character_set_name, collation_name FROM information_schema.COLUMNS WHERE table_schema = "test" AND table_name = "t1";
+-------------+--------------------+----------------+
| column_name | character_set_name | collation_name |
+-------------+--------------------+----------------+
| a           | NULL               | NULL           |
+-------------+--------------------+----------------+
1 row in set (0.01 sec)

What is changed and how it works?

It used a HasCharset() function ported from MySQL source and use it to determine if given column's Charset/Collation should be displayed, following the way MySQL does:

sql_show.cc::store_column_type()

Check List

Tests

  • Integration test

Code changes

  • Has interface methods change

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

Copy link
Contributor

@xiekeyi98 xiekeyi98 left a comment

Choose a reason for hiding this comment

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

  1. TODO && FIXME, I think use one of them is enough.
  2. Maybe we should delete or change the mysql-test.

executor/show_test.go Outdated Show resolved Hide resolved
infoschema/tables_test.go Outdated Show resolved Hide resolved
@xiekeyi98
Copy link
Contributor

/run-all-tests

@bb7133
Copy link
Member Author

bb7133 commented Apr 4, 2019

/run-all-tests tidb-test=pr/777

@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

Merging #10007 into master will decrease coverage by 0.0673%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##            master     #10007        +/-   ##
===============================================
- Coverage   78.033%   77.9657%   -0.0674%     
===============================================
  Files          404        404                
  Lines        81996      81995         -1     
===============================================
- Hits         63984      63928        -56     
- Misses       13316      13354        +38     
- Partials      4696       4713        +17

@ngaut
Copy link
Member

ngaut commented Apr 4, 2019

/run-all-tests tidb-test=pr/777

@bb7133
Copy link
Member Author

bb7133 commented Apr 8, 2019

/run-all-tests tidb-test=pr/777

@bb7133
Copy link
Member Author

bb7133 commented Apr 9, 2019

/run-all-tests tidb-test=pr/777

Copy link
Contributor

@xiekeyi98 xiekeyi98 left a comment

Choose a reason for hiding this comment

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

LGTM

@xiekeyi98 xiekeyi98 added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 9, 2019
executor/show_test.go Outdated Show resolved Hide resolved
table/column.go Outdated Show resolved Hide resolved
@bb7133
Copy link
Member Author

bb7133 commented Apr 9, 2019

Addressed, thanks @zimulala

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala
Copy link
Contributor

zimulala commented Apr 9, 2019

/run-all-tests tidb-test=pr/777

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 9, 2019
@xiekeyi98 xiekeyi98 merged commit f723f0b into pingcap:master Apr 9, 2019
@tshqin
Copy link
Contributor

tshqin commented Apr 10, 2019

need a cherry pick to 2.1 branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT2 Indicates that a PR has LGTM 2. type/compatibility
Projects
None yet
5 participants