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: check non-BMP characters and return error when the charset is utf8 and sql mode is strict mode #8738

Merged
merged 13 commits into from
Dec 19, 2018
Merged

Conversation

winkyao
Copy link
Contributor

@winkyao winkyao commented Dec 18, 2018

What problem does this PR solve?

Before this PR, TiDB don't handle the non-BMP character when the charset is utf8. Run the sqls in mysql:

mysql> create table t1(a varchar(100) charset utf8);
Query OK, 0 rows affected (0.01 sec)

mysql> insert into t1 values(x'f09f8c80');
ERROR 1366 (HY000): Incorrect string value: '\xF0\x9F\x8C\x80' for column 'a' at row 1

But in TiDB:

mysql> create table t1(a varchar(100) charset utf8);
Query OK, 0 rows affected (0.01 sec)

mysql> insert into t1 values(x'f09f8c80');
Query OK, 1 row affected (0.00 sec)

What is changed and how it works?

utf8 cannot handle the Unicode character length larger than 3.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

This change is Reviewable

@winkyao
Copy link
Contributor Author

winkyao commented Dec 18, 2018

@kennytm PTAL

Copy link
Contributor

@ciscoxll ciscoxll left a comment

Choose a reason for hiding this comment

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

PR need to be cherry-pick to 2.1 and 2.0.

Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed 2 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@kennytm kennytm added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 18, 2018
@shenli
Copy link
Member

shenli commented Dec 18, 2018

@tiancaiamao @jackysp @lysu PTAL

@winkyao
Copy link
Contributor Author

winkyao commented Dec 18, 2018

@ciscoxll This PR will break backward compatibility, I will not pick it to 2.0 and I'm considering to pick it to 2.1

@zz-jason
Copy link
Member

@winkyao Please add some proper labels, for example, component, type, etc.

Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

lysu
lysu previously approved these changes Dec 19, 2018
Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

:lgtm: maybe tikv also need check this too @breeswish

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lysu lysu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 19, 2018
@lysu
Copy link
Contributor

lysu commented Dec 19, 2018

/run-all-tests

Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@winkyao
Copy link
Contributor Author

winkyao commented Dec 19, 2018

/run-all-tests

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

Successfully merging this pull request may close these issues.

6 participants