executor: make the results of `show create table` more consistent with MySQL #9229

spongedu · 2019-01-30T10:48:41Z

What problem does this PR solve?

Fix issue #9211 , Make results of show create table more consistent with MySQL

What is changed and how it works?

Don't show default values for generated columns
Refine demonstration for CHARSET for character type columns

Check List

Tests

Unit test

spongedu · 2019-01-30T10:50:17Z

@zimulala @zz-jason @xiekeyi98 PTAL :)

codecov-io · 2019-01-30T11:01:44Z

Codecov Report

Merging #9229 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #9229      +/-   ##
==========================================
+ Coverage   67.03%   67.17%   +0.14%     
==========================================
  Files         371      371              
  Lines       77531    77531              
==========================================
+ Hits        51973    52085     +112     
+ Misses      20922    20788     -134     
- Partials     4636     4658      +22

Impacted Files	Coverage Δ
executor/show.go	`43.72% <100%> (+0.45%)`	⬆️
ddl/session_pool.go	`82.75% <0%> (-10.35%)`	⬇️
ddl/delete_range.go	`73.54% <0%> (-1.59%)`	⬇️
executor/index_lookup_join.go	`77.28% <0%> (-0.64%)`	⬇️
executor/join.go	`78.38% <0%> (-0.53%)`	⬇️
executor/distsql.go	`73% <0%> (+0.46%)`	⬆️
expression/schema.go	`94.53% <0%> (+0.78%)`	⬆️
util/systimemon/systime_mon.go	`100% <0%> (+20%)`	⬆️
util/filesort/filesort.go	`76.48% <0%> (+35.42%)`	⬆️

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 f73c4e2...2561a4b. Read the comment docs.

xiekeyi98 · 2019-01-30T11:30:37Z

executor/show.go

@@ -614,7 +615,8 @@ func (e *ShowExec) fetchShowCreateTable() error {
 			if mysql.HasNotNullFlag(col.Flag) {
 				buf.WriteString(" NOT NULL")
 			}
-			if !mysql.HasNoDefaultValueFlag(col.Flag) {
+			// don't show default value for generated columns


I think we'd better to explain why "not show default value for generated columns".
For example, "Default value are not shown for generated columns in mysql".
The current comment, I think, is the same as the code(a little bit redundant)

Reasonable ~

xiekeyi98 · 2019-01-31T02:28:43Z

executor/show.go

-		if col.Charset != "binary" {
-			fmt.Fprintf(&buf, " CHARSET %s COLLATE %s", col.Charset, col.Collate)
+		// charset && collate are not shown for generated columns in MySQL
+		if col.Charset != "binary" && !col.IsGenerated() {


"Because it is generated , charset and collate are not shown." OR "Because it is generated and charset is binary, charset and collate are not shown".

xiekeyi98 · 2019-01-31T02:30:50Z

executor/show_test.go

+			"  `b` char(10) GENERATED ALWAYS AS (rtrim(`a`)) VIRTUAL\n"+
+			") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin",
+	))
+	tk.MustExec("drop table t")
 }


I was wondering if you could add test like this create table test ( a char(10) charset gb2312 , b char(10) charset utf8 as (rtrim(a)) );?
BTW, The following is the result of MYSQL:

mysql> create table test ( a char(10) charset gb2312 , b char(10) charset utf8 as (rtrim(a)) ); Query OK, 0 rows affected (0.02 sec) mysql> show create table test; +-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | test | CREATE TABLE `test` ( `a` char(10) CHARACTER SET gb2312 DEFAULT NULL, `b` char(10) CHARACTER SET utf8 GENERATED ALWAYS AS (rtrim(`a`)) VIRTUAL ) ENGINE=InnoDB DEFAULT CHARSET=latin1 | +-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (0.00 sec) mysql> select version(); +-------------------------+ | version() | +-------------------------+ | 5.7.25-0ubuntu0.18.04.2 | +-------------------------+ 1 row in set (0.00 sec)

@xiekeyi98 I seems make a mistake here. I made several tests and found that whether a column is generated do not actually determine whether CHARSET && COLLATE are shown. In fact, it's determined by whether CHARSET && COLLATE are specified explicitly.
I'd prefer left it to another issue and make this pr clearer.

BTW, TiDB seems don't support gb2312 for now.
The following are my tests:

mysql> select version(); +------------+ | version() | +------------+ | 5.7.17-log | +------------+ 1 row in set (0.01 sec) mysql> create table test ( a char(10) charset utf8 , b char(10) charset utf8 as (rtrim(a)) ); Query OK, 0 rows affected (0.03 sec) mysql> show create table test; +-------+-------------------------------------------------------------------------------------------------------------------------------------+| Table | Create Table |+-------+-------------------------------------------------------------------------------------------------------------------------------------+| test | CREATE TABLE `test` ( `a` char(10) CHARACTER SET utf8 DEFAULT NULL, `b` char(10) CHARACTER SET utf8 GENERATED ALWAYS AS (rtrim(`a`)) VIRTUAL ) ENGINE=InnoDB DEFAULT CHARSET=latin1 | +-------+-------------------------------------------------------------------------------------------------------------------------------------+1 row in set (0.00 sec) mysql> drop table test; Query OK, 0 rows affected (0.02 sec) mysql> create table test ( a char(10) charset utf8 , b char(10) as (rtrim(a)) ); Query OK, 0 rows affected (0.03 sec) mysql> show create table test; +-------+-------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +-------+-------------------------------------------------------------------------------------------------------------------------------------+ | test | CREATE TABLE `test` ( `a` char(10) CHARACTER SET utf8 DEFAULT NULL, `b` char(10) GENERATED ALWAYS AS (rtrim(`a`)) VIRTUAL ) ENGINE=InnoDB DEFAULT CHARSET=latin1 | +-------+-------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (0.00 sec) mysql> drop table test; Query OK, 0 rows affected (0.02 sec) mysql> create table test ( a char(10), b char(10) as (rtrim(a)) ); Query OK, 0 rows affected (0.03 sec) mysql> show create table test; +-------+-------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +-------+-------------------------------------------------------------------------------------------------------------------------------------+ | test | CREATE TABLE `test` ( `a` char(10) DEFAULT NULL, `b` char(10) GENERATED ALWAYS AS (rtrim(`a`)) VIRTUAL ) ENGINE=InnoDB DEFAULT CHARSET=latin1 | +-------+-------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (0.00 sec) mysql> desc test; +-------+----------+------+-----+---------+-------------------+ | Field | Type | Null | Key | Default | Extra | +-------+----------+------+-----+---------+-------------------+ | a | char(10) | YES | | NULL | | | b | char(10) | YES | | NULL | VIRTUAL GENERATED | +-------+----------+------+-----+---------+-------------------+ 2 rows in set (0.01 sec)

Sorry for my negligence.
Whether gb2312 or not is not necessary, I just want to show something like this.
In my mind, whether mysql displays that fields depends on whether the table and column charset are consistent(I am not very sure).

@xiekeyi98 I've opened another issue #9232 for the demonstration of the demonstration of CHARSET && COLLATE. It's not related to generated columns, indeed. I'll look into this later.

Well done. Thanks for you contribution.

seems you've working on this in #9113 :P

xiekeyi98

LGTM，Thanks for your contribution.

winkyao

LGTM

ngaut · 2019-01-31T09:00:31Z

/run-all-tests

spongedu · 2019-01-31T09:28:31Z

It seems that the test cases should be updated. @zz-jason @winkyao

xiekeyi98 · 2019-02-14T08:49:20Z

@spongedu
Would you mind solve the conflicts?
And then I will update the test cases for this PR.

spongedu · 2019-02-14T09:59:54Z

@xiekeyi98 OK

spongedu · 2019-02-14T10:40:46Z

@xiekeyi98 PTAL :)

xiekeyi98 · 2019-02-14T13:13:43Z

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

zimulala · 2019-02-14T15:08:25Z

/run-unit-test

xiekeyi98 · 2019-02-15T02:46:35Z

/run-all-tests

xiekeyi98

LGTM, thank you.

zimulala

LGTM

Fix pingcap#9211: make more consistent with MySQL

56f69ad

xiekeyi98 added type/compatibility contribution This PR is from a community contributor. sig/execution SIG execution labels Jan 30, 2019

xiekeyi98 changed the title ~~Fix #9211: make more consistent with MySQL~~ executor: make more consistent with MySQL Jan 30, 2019

xiekeyi98 reviewed Jan 30, 2019

View reviewed changes

refine comments

a319ed1

spongedu changed the title ~~executor: make more consistent with MySQL~~ executor: make the results of show create table more consistent with MySQL Jan 30, 2019

xiekeyi98 reviewed Jan 31, 2019

View reviewed changes

revert demonstration of && for generated columns

e9f7775

spongedu mentioned this pull request Jan 31, 2019

Results of show create table are not consistent with MySQL for character type columns #9232

Closed

xiekeyi98 reviewed Jan 31, 2019

View reviewed changes

xiekeyi98 added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 31, 2019

winkyao previously approved these changes Jan 31, 2019

View reviewed changes

Merge branch 'master' into fix_#9211

6ddb875

ngaut added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 31, 2019

Merge remote-tracking branch 'upstream/master' into fix_#9211

2561a4b

spongedu dismissed winkyao’s stale review via 2561a4b February 14, 2019 10:39

xiekeyi98 mentioned this pull request Feb 14, 2019

Message about Charset&&Collation in show create table is not same with MYSQL #9307

Open

xiekeyi98 added the status/all tests passed label Feb 15, 2019

xiekeyi98 reviewed Feb 15, 2019

View reviewed changes

xiekeyi98 added status/LGT1 Indicates that a PR has LGTM 1. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Feb 15, 2019

zimulala approved these changes Feb 15, 2019

View reviewed changes

zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 15, 2019

zimulala merged commit 3d4ad1f into pingcap:master Feb 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: make the results of `show create table` more consistent with MySQL #9229

executor: make the results of `show create table` more consistent with MySQL #9229

spongedu commented Jan 30, 2019 •

edited

Loading

spongedu commented Jan 30, 2019 •

edited

Loading

codecov-io commented Jan 30, 2019 •

edited

Loading

xiekeyi98 Jan 30, 2019

spongedu Jan 30, 2019

xiekeyi98 Jan 31, 2019

xiekeyi98 Jan 31, 2019

spongedu Jan 31, 2019

xiekeyi98 Jan 31, 2019

spongedu Jan 31, 2019

xiekeyi98 Jan 31, 2019

spongedu Jan 31, 2019

xiekeyi98 left a comment •

edited

Loading

winkyao left a comment

ngaut commented Jan 31, 2019

spongedu commented Jan 31, 2019

xiekeyi98 commented Feb 14, 2019

spongedu commented Feb 14, 2019

spongedu commented Feb 14, 2019

xiekeyi98 commented Feb 14, 2019

zimulala commented Feb 14, 2019

xiekeyi98 commented Feb 15, 2019

xiekeyi98 left a comment

zimulala left a comment

executor: make the results of show create table more consistent with MySQL #9229

executor: make the results of show create table more consistent with MySQL #9229

Conversation

spongedu commented Jan 30, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

spongedu commented Jan 30, 2019 • edited Loading

codecov-io commented Jan 30, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyi98 left a comment • edited Loading

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

ngaut commented Jan 31, 2019

spongedu commented Jan 31, 2019

xiekeyi98 commented Feb 14, 2019

spongedu commented Feb 14, 2019

spongedu commented Feb 14, 2019

xiekeyi98 commented Feb 14, 2019

zimulala commented Feb 14, 2019

xiekeyi98 commented Feb 15, 2019

xiekeyi98 left a comment

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

executor: make the results of `show create table` more consistent with MySQL #9229

executor: make the results of `show create table` more consistent with MySQL #9229

spongedu commented Jan 30, 2019 •

edited

Loading

spongedu commented Jan 30, 2019 •

edited

Loading

codecov-io commented Jan 30, 2019 •

edited

Loading

xiekeyi98 left a comment •

edited

Loading