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

ddl: modify different character sets varchar type column maximum length limit #8818

Merged
merged 6 commits into from
Jan 14, 2019

Conversation

ciscoxll
Copy link
Contributor

@ciscoxll ciscoxll commented Dec 25, 2018

What problem does this PR solve?

We execute the following sql , found that tidb and mysql behave differently.

# tidb master branch / tidb v2.1.0:

tidb>  CREATE TABLE tr (id INT, name VARCHAR(20000), purchased DATE )  DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin;
ERROR 1074 (42000): Column length too big for column 'name' (max = 16383); use BLOB or TEXT instead
# tidb v2.0.0 :

MySQL [test]> CREATE TABLE tr (id INT, name VARCHAR(20000), purchased DATE )  DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin;
Query OK, 0 rows affected (0.01 sec)

mysql>  CREATE TABLE tr (id INT, name VARCHAR(65536), purchased DATE )  DEFAULT CHARSET=utf8 COLLATE=utf8_bin;
ERROR 1074 (42000): Column length too big for column 'name' (max = 21845); use BLOB or TEXT instead
# mysql 5.7
mysql>  CREATE TABLE tr (id INT, name VARCHAR(65536), purchased DATE )  DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin;
ERROR 1074 (42000): Column length too big for column 'name' (max = 16383); use BLOB or TEXT instead

mysql>  CREATE TABLE tr (id INT, name VARCHAR(65536), purchased DATE )  DEFAULT CHARSET=utf8 COLLATE=utf8_bin;
ERROR 1074 (42000): Column length too big for column 'name' (max = 21845); use BLOB or TEXT instead

mysql>  CREATE TABLE tr (id INT, name VARCHAR(65536), purchased DATE ) DEFAULT CHARSET=latin1;
ERROR 1074 (42000): Column length too big for column 'name' (max = 65535); use BLOB or TEXT instead

What is changed and how it works?

  1. First use the character set of Column to set the value;
  2. If not set above, use the default character set value of the corresponding table;
  3. If not set above, use the default character set value of the corresponding database;
  4. If not set above, use the character_set_server setting.

Check List

Tests

  • Unit test

This change is Reviewable

@ciscoxll
Copy link
Contributor Author

@winkyao @zimulala @crazycs520 PTAL

ddl/ddl_api.go Outdated Show resolved Hide resolved
planner/core/preprocess.go Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
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.

Please fix tests.

@ciscoxll
Copy link
Contributor Author

ciscoxll commented Jan 3, 2019

@winkyao @zimulala @crazycs520 PTAL

2 similar comments
@ciscoxll
Copy link
Contributor Author

ciscoxll commented Jan 7, 2019

@winkyao @zimulala @crazycs520 PTAL

@ciscoxll
Copy link
Contributor Author

ciscoxll commented Jan 8, 2019

@winkyao @zimulala @crazycs520 PTAL

@ciscoxll
Copy link
Contributor Author

ciscoxll commented Jan 9, 2019

@zimulala @crazycs520 PTAL

ddl/ddl_api.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Jan 9, 2019

Codecov Report

Merging #8818 into master will decrease coverage by <.01%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8818      +/-   ##
==========================================
- Coverage   67.27%   67.26%   -0.01%     
==========================================
  Files         371      371              
  Lines       75972    75997      +25     
==========================================
+ Hits        51108    51123      +15     
- Misses      20354    20362       +8     
- Partials     4510     4512       +2
Impacted Files Coverage Δ
planner/core/preprocess.go 89.15% <100%> (+0.38%) ⬆️
ddl/ddl_api.go 75.06% <86.66%> (+0.21%) ⬆️
util/systimemon/systime_mon.go 80% <0%> (-20%) ⬇️
store/tikv/lock_resolver.go 41.7% <0%> (-0.95%) ⬇️
store/tikv/gcworker/gc_worker.go 41.14% <0%> (-0.43%) ⬇️

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 53707ce...709777f. Read the comment docs.

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

@ciscoxll ciscoxll added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 10, 2019
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 14, 2019
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

@ciscoxll ciscoxll added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jan 14, 2019
@zimulala
Copy link
Contributor

/run-all-tests

@ciscoxll
Copy link
Contributor Author

/run-all-tests

@ciscoxll ciscoxll merged commit 44fd7d4 into pingcap:master Jan 14, 2019
@ciscoxll ciscoxll deleted the varchar-limit branch January 14, 2019 09:39
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
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/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants