infoschema: fix bug of column size of set and enum type #7347

imtbkcat · 2018-08-10T03:38:34Z

What have you changed? (mandatory)

I add code to fix bug when query column size of set and enum type.
Originally the column size of set and enum type while be default value 2^64 - 1. (See mysql/util:65)
This is different from MySQL's result.
For example, in MySQL 5.7.

mysql> create table bug22613 ( s set('a','bc','def','ghij') default NULL, t enum('a', 'ab', 'cdef'), s2 
SET('1','2','3','4','1585','ONE','TWO','Y','N','THREE'));
Query OK, 0 rows affected (0.11 sec)

mysql> SELECT column_name,         character_maximum_length  FROM   
information_schema.columns  WHERE  table_schema = Database()         AND         table_name = 
'bug22613'         AND         column_name = 't';
+-------------+--------------------------+
| column_name | character_maximum_length |
+-------------+--------------------------+
| t           |                        4 |
+-------------+--------------------------+
1 row in set (0.00 sec)

But in TiDB:

mysql> SELECT column_name,         character_maximum_length  FROM   
information_schema.columns  WHERE  table_schema = Database()         AND         table_name = 
'bug22613'         AND         column_name = 't';
+-------------+--------------------------+
| column_name | character_maximum_length |
+-------------+--------------------------+
| t           |     18446744073709551615 |
+-------------+--------------------------+
1 row in set (0.00 sec)

I fix this bug by adding code to calculate size of set and enum like MySQL.

What is the type of the changes? (mandatory)

Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

unit test

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Does this PR need to be added to the release notes? (mandatory)

No

Refer to a related PR or issue link (optional)

Fix #7297

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

shenli · 2018-08-10T03:42:49Z

infoschema/tables_test.go

+		testkit.Rows("s2 30"))
+	tk.MustQuery("select column_name, character_maximum_length from information_schema.columns where table_schema=Database() and table_name = 't' and column_name = 'e1'").Check(
+		testkit.Rows("e1 4"))
+	//


Remove this line.

shenli · 2018-08-10T03:45:25Z

infoschema/tables.go

+			}
+			colLen = sumLen + cnt
+		}
+		if col.Tp == mysql.TypeEnum {


jackysp · 2018-08-10T03:47:28Z

infoschema/tables.go

@@ -797,6 +797,30 @@ func dataForColumnsInTable(schema *model.DBInfo, tbl *model.TableInfo) [][]types
 		if colLen == types.UnspecifiedLength {
 			colLen = defaultFlen
 		}
+		if col.Tp == mysql.TypeSet {
+			//example: In MySQL set('a','bc','def','ghij') has length 13, This is because
+			//len('a')+len('bc')+len('def')+len('ghij')+len(ThreeComma)=13


It's better to check whether MySQL uses this rule.

It is better to add a reference link.

Ok, I'm looking for MySQL's method.

Reference link: https://bugs.mysql.com/bug.php?id=22613

It's better to add the link to the comments.

jackysp · 2018-08-10T03:47:41Z

infoschema/tables.go

+		}
+		if col.Tp == mysql.TypeEnum {
+			//exapmle: In MySQL enum('a', 'ab', 'cdef') has length 4, This is because
+			//the longest string in the enum is 'cdef'


imtbkcat · 2018-08-10T08:52:51Z

/run-all-tests

imtbkcat · 2018-08-10T10:22:50Z

/rebuild

imtbkcat · 2018-08-10T11:06:21Z

/run-all-tests

alivxxx · 2018-08-13T03:15:22Z

infoschema/tables_test.go

@@ -77,6 +77,16 @@ func (s *testSuite) TestDataForTableRowsCountField(c *C) {
 	tk.MustExec("create user xxx")
 	tk.MustExec("flush privileges")

+	//Test for length of enmu and set


enmu -> enum. And it is better to add a space between // and comment.

alivxxx · 2018-08-13T03:16:56Z

infoschema/tables.go

+			}
+			colLen = sumLen + cnt
+		} else if col.Tp == mysql.TypeEnum {
+			//exapmle: In MySQL enum('a', 'ab', 'cdef') has length 4, This is because


exapmle-> example.

It is better to add a space between // and comment.

It is better to remove the This is.

imtbkcat · 2018-08-13T06:11:03Z

/run-all-tests

imtbkcat · 2018-08-13T06:50:14Z

PTAL @jackysp @zhexuany @shenli

jackysp

LGTM

crazycs520 · 2018-08-13T08:11:01Z

infoschema/tables.go

+			// Example: In MySQL set('a','bc','def','ghij') has length 13, because
+			// len('a')+len('bc')+len('def')+len('ghij')+len(ThreeComma)=13
+			// Reference link: https://bugs.mysql.com/bug.php?id=22613
+			sumLen, cnt := 0, 0


how about this:

colLen = 0 for _, ele := range col.Elems { colLen += len(ele) } if len(col.Elems) != 0 { colLen = len(col.Elems) - 1 }

@crazycs520
it looks grate. But it should be:

if len(col.Elems) != 0 { colLen += (len(col.Elems) - 1) }

winoros · 2018-08-13T09:00:32Z

infoschema/tables_test.go

@@ -77,6 +77,16 @@ func (s *testSuite) TestDataForTableRowsCountField(c *C) {
 	tk.MustExec("create user xxx")
 	tk.MustExec("flush privileges")

+	//Test for length of enum and set


Add space. // Test....

crazycs520

LGTM

imtbkcat · 2018-08-13T09:02:56Z

PTAL @winoros @crazycs520

imtbkcat · 2018-08-13T09:14:14Z

/run-all-tests

imtbkcat added type/compatibility type/bugfix This PR fixes a bug. component/mysql-protocol labels Aug 10, 2018

imtbkcat requested review from coocood, shenli and jackysp August 10, 2018 03:38

shenli reviewed Aug 10, 2018

View reviewed changes

infoschema/tables.go Outdated

}

colLen = sumLen + cnt

}

if col.Tp == mysql.TypeEnum {

Copy link

Member

shenli Aug 10, 2018

Choose a reason for hiding this comment

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

else if

jackysp reviewed Aug 10, 2018

View reviewed changes

imtbkcat added 3 commits August 10, 2018 11:48

add code to fix issue 7297

e57f2de

remove an empty line

f5e68fb

change to else if

f2a43d9

Merge branch 'master' into fix7297

30e6b3c

imtbkcat added the status/all tests passed label Aug 10, 2018

Update tables.go

4c65f1e

alivxxx reviewed Aug 13, 2018

View reviewed changes

Lingyu Song added 4 commits August 13, 2018 11:26

Update tables.go

02f3040

Update tables_test.go

5814142

Merge branch 'master' into fix7297

531d5a1

Merge branch 'master' into fix7297

249e07c

imtbkcat removed the component/mysql-protocol label Aug 13, 2018

jackysp reviewed Aug 13, 2018

View reviewed changes

imtbkcat added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 13, 2018

crazycs520 reviewed Aug 13, 2018

View reviewed changes

update tables.go

a2d6e42

winoros reviewed Aug 13, 2018

View reviewed changes

Update tables_test.go

11f58a9

crazycs520 reviewed Aug 13, 2018

View reviewed changes

crazycs520 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 13, 2018

Merge branch 'master' into fix7297

549a2ed

crazycs520 approved these changes Aug 13, 2018

View reviewed changes

Merge branch 'master' into fix7297

694eacd

imtbkcat merged commit e43e548 into pingcap:master Aug 13, 2018

imtbkcat deleted the fix7297 branch August 13, 2018 11:35

infoschema: fix bug of column size of set and enum type #7347

infoschema: fix bug of column size of set and enum type #7347

Uh oh!

Conversation

imtbkcat commented Aug 10, 2018

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imtbkcat commented Aug 10, 2018

Uh oh!

imtbkcat commented Aug 10, 2018

Uh oh!

imtbkcat commented Aug 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imtbkcat commented Aug 13, 2018

Uh oh!

imtbkcat commented Aug 13, 2018

Uh oh!

jackysp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazycs520 left a comment

Choose a reason for hiding this comment

Uh oh!

imtbkcat commented Aug 13, 2018

Uh oh!

imtbkcat commented Aug 13, 2018

Uh oh!

Uh oh!