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:adjust 'show create table' for mysql compatibility #9113

Merged
merged 27 commits into from
Feb 14, 2019
Merged

executor:adjust 'show create table' for mysql compatibility #9113

merged 27 commits into from
Feb 14, 2019

Conversation

xiekeyi98
Copy link
Contributor

show create table should not print the charset of the column
if it is the same with table charset( same with MySQL)

What problem does this PR solve?

If the charset of column is the same with table charset , we should better not show the carset of the column.
(In order to be consistent with MySQL.)

What is changed and how it works?

Change func (e *ShowExec) fetchShowCreateTable() error{} , to determine if the column charset and table charset are equal.

Check List

Tests

  • Unit test

Before

mysql> show create table ptest\G
*************************** 1. row ***************************
       Table: ptest
Create Table: CREATE TABLE `ptest` (
  `a` int(11) NOT NULL,
  `b` double NOT NULL DEFAULT '2.0',
  `c` varchar(10) CHARSET utf8mb4 COLLATE utf8mb4_bin NOT NULL,
  `d` time DEFAULT NULL,
  `e` timestamp NULL DEFAULT NULL,
  `f` timestamp NULL DEFAULT NULL,
  PRIMARY KEY (`a`),
  UNIQUE KEY `d` (`d`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin
1 row in set (0.00 sec)

After

mysql> show create table ptest\G
ERROR 2006 (HY000): MySQL server has gone away
No connection. Trying to reconnect...
Connection id:    1
Current database: test

*************************** 1. row ***************************
       Table: ptest
Create Table: CREATE TABLE `ptest` (
  `a` int(11) NOT NULL,
  `b` double NOT NULL DEFAULT '2.0',
  `c` varchar(10) NOT NULL,
  `d` time DEFAULT NULL,
  `e` timestamp NULL DEFAULT NULL,
  `f` timestamp NULL DEFAULT NULL,
  PRIMARY KEY (`a`),
  UNIQUE KEY `d` (`d`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin
1 row in set (0.00 sec)

@xiekeyi98 xiekeyi98 added type/enhancement The issue or PR belongs to an enhancement. sig/execution SIG execution labels Jan 18, 2019
@xiekeyi98 xiekeyi98 changed the title executor:adjust 'show create table' executor:adjust 'show create table' for mysql compatibility Jan 18, 2019
@codecov-io
Copy link

codecov-io commented Jan 18, 2019

Codecov Report

Merging #9113 into master will increase coverage by <.01%.
The diff coverage is 45.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9113      +/-   ##
==========================================
+ Coverage   67.17%   67.18%   +<.01%     
==========================================
  Files         371      371              
  Lines       77524    77525       +1     
==========================================
+ Hits        52079    52083       +4     
+ Misses      20791    20789       -2     
+ Partials     4654     4653       -1
Impacted Files Coverage Δ
executor/show.go 43.26% <45.45%> (+0.08%) ⬆️
executor/join.go 77.86% <0%> (-0.53%) ⬇️
executor/distsql.go 73% <0%> (+0.46%) ⬆️
util/systimemon/systime_mon.go 100% <0%> (+20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de56ea6...dca2737. Read the comment docs.

@xiekeyi98
Copy link
Contributor Author

/run-all-tests

@xiekeyi98
Copy link
Contributor Author

/run-common-test tidb-test=pr/724
/run-integration-common-test tidb-test=pr/724

show create table should not print the charset of the column
if it is the same with table charset( same with MySQL)
@xiekeyi98
Copy link
Contributor Author

/run-common-test tidb-test=pr/724
/run-integration-common-test tidb-test=pr/724

1 similar comment
@xiekeyi98
Copy link
Contributor Author

/run-common-test tidb-test=pr/724
/run-integration-common-test tidb-test=pr/724

@xiekeyi98
Copy link
Contributor Author

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

executor/show.go Outdated Show resolved Hide resolved
executor/show.go Outdated Show resolved Hide resolved
@xiekeyi98
Copy link
Contributor Author

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

@xiekeyi98
Copy link
Contributor Author

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

@xiekeyi98
Copy link
Contributor Author

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

@xiekeyi98
Copy link
Contributor Author

/run-common-test tidb-test=pr/724
/run-integration-common-test tidb-test=pr/724

@xiekeyi98
Copy link
Contributor Author

/run-common-test tidb-test=pr/724

@AndrewDi
Copy link
Contributor

AndrewDi commented Feb 9, 2019

@xiekeyi98 For your reference in my test, if we create column with specific charset, even column's charset is the same as table's charset, SHOW CREATE TABLE will show CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci part.

mysql> use test
Database changed
mysql> create table t (a int,b varchar(20));
Query OK, 0 rows affected (0.02 sec)

mysql> show create table t\G
*************************** 1. row ***************************
       Table: t
Create Table: CREATE TABLE `t` (
  `a` int(11) DEFAULT NULL,
  `b` varchar(20) DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci
1 row in set (0.00 sec)

mysql> create table t1 (a int,b varchar(20) charset utf8mb4);
Query OK, 0 rows affected (0.02 sec)

mysql> show create table t1\G
*************************** 1. row ***************************
       Table: t1
Create Table: CREATE TABLE `t1` (
  `a` int(11) DEFAULT NULL,
  `b` varchar(20) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci
1 row in set (0.00 sec)

mysql> select version()\G
*************************** 1. row ***************************
version(): 8.0.15
1 row in set (0.00 sec)

@xiekeyi98
Copy link
Contributor Author

@AndrewDi Thanks for your help.
Would you mind review my code again?

executor/show.go Outdated Show resolved Hide resolved
executor/show.go Outdated Show resolved Hide resolved
@xiekeyi98
Copy link
Contributor Author

Because I don't understand how MYSQL handles it, I rolled back some commits.
This PR will only resolve table && column are inconsistent.
I will create new issues to explain more.

@xiekeyi98
Copy link
Contributor Author

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

@xiekeyi98
Copy link
Contributor Author

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

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

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 cherry-pick this PR to 2.1 branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/execution SIG execution type/compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants