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

util: rewrite parser warings for integer display width #18775

Merged
merged 17 commits into from
Aug 24, 2020

Conversation

AilinKid
Copy link
Contributor

@AilinKid AilinKid commented Jul 24, 2020

Signed-off-by: AilinKid 314806019@qq.com

What problem does this PR solve?

The linked Parser: pingcap/parser#939

Problem Summary: For integerType:

tinyint, smallint, mediumint, int, bigint, integer, int1, int2, int3, int4, int8

if they are followed with (num) to specify the length, it should have the warnings

What is changed and how it works?

How it Works:
mark the warings in parser, rewrite the parser with terror class {errcode 1681, errmsg "xxx"} in TiDB

Check List

Tests

  • Unit test
  • Integration test

Release note

  • util: rewrite parser warings for integer display width

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jul 24, 2020
go.mod Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

Merging #18775 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #18775   +/-   ##
===========================================
  Coverage   79.3811%   79.3811%           
===========================================
  Files           546        546           
  Lines        148219     148219           
===========================================
  Hits         117658     117658           
  Misses        21078      21078           
  Partials       9483       9483           

@ghost
Copy link

ghost commented Jul 27, 2020

To clarify, this is a MySQL 8.0 compatible behavior, not a MySQL 5.7 one:

mysql [localhost:5731] {msandbox} (test) > CREATE TABLE t1 (id int(10) not null);
Query OK, 0 rows affected (0.02 sec)

mysql [localhost:5731] {msandbox} (test) > SHOW WARNINGS;
Empty set (0.00 sec)

..

mysql [localhost:8021] {msandbox} (test) > CREATE TABLE t1 (id int(10) not null);
Query OK, 0 rows affected, 1 warning (0.03 sec)

mysql [localhost:8021] {msandbox} (test) > show warnings;
+---------+------+------------------------------------------------------------------------------+
| Level   | Code | Message                                                                      |
+---------+------+------------------------------------------------------------------------------+
| Warning | 1681 | Integer display width is deprecated and will be removed in a future release. |
+---------+------+------------------------------------------------------------------------------+
1 row in set (0.00 sec)

Looking at this patch, it looks like it doesn't modify SHOW CREATE TABLE output. Thus, if you CREATE a table without integer display width, and then SHOW CREATE TABLE (as a dumper does) and then try and restore that, you will get a warning :( I think this is a bit of a noisy behavior (especially since display width does nothing in TiDB).

There is still a use-case in MySQL to use integer display width because of ZEROFILL, but since this is not supported in TiDB there is actually no use case! May I instead propose that the warning work side-by-side with an option to SHOW CREATE TABLE to not display integer width? It is probably safe enough to be enabled by default, but others may have input here.

See also: #17682

@AilinKid
Copy link
Contributor Author

AilinKid commented Jul 27, 2020

To clarify, this is a MySQL 8.0 compatible behavior, not a MySQL 5.7 one:

mysql [localhost:5731] {msandbox} (test) > CREATE TABLE t1 (id int(10) not null);
Query OK, 0 rows affected (0.02 sec)

mysql [localhost:5731] {msandbox} (test) > SHOW WARNINGS;
Empty set (0.00 sec)

..

mysql [localhost:8021] {msandbox} (test) > CREATE TABLE t1 (id int(10) not null);
Query OK, 0 rows affected, 1 warning (0.03 sec)

mysql [localhost:8021] {msandbox} (test) > show warnings;
+---------+------+------------------------------------------------------------------------------+
| Level   | Code | Message                                                                      |
+---------+------+------------------------------------------------------------------------------+
| Warning | 1681 | Integer display width is deprecated and will be removed in a future release. |
+---------+------+------------------------------------------------------------------------------+
1 row in set (0.00 sec)

Looking at this patch, it looks like it doesn't modify SHOW CREATE TABLE output. Thus, if you CREATE a table without integer display width, and then SHOW CREATE TABLE (as a dumper does) and then try and restore that, you will get a warning :( I think this is a bit of a noisy behavior (especially since display width does nothing in TiDB).

There is still a use-case in MySQL to use integer display width because of ZEROFILL, but since this is not supported in TiDB there is actually no use case! May I instead propose that the warning work side-by-side with an option to SHOW CREATE TABLE to not display integer width? It is probably safe enough to be enabled by default, but others may have input here.

See also: #17682

So that means we should fix the integer display length in the output of show create table, right? @nullnotnil

@AilinKid
Copy link
Contributor Author

I wanna do it in another PR.
Since the show create table result will ignore the display length
For example, int(11) -> int, bigint(20) -> bigint and so on, there is a huge change int TiDB test collection.
@nullnotnil @morgo

@AilinKid
Copy link
Contributor Author

/run-unit-test

@ghost
Copy link

ghost commented Jul 29, 2020

I wanna do it in another PR.
Since the show create table result will ignore the display length
For example, int(11) -> int, bigint(20) -> bigint and so on, there is a huge change int TiDB test collection.
@nullnotnil @morgo

I think it is important that a CREATE TABLE -> SHOW CREATE TABLE -> CREATE TABLE doesn't produce warnings. Perhaps in terms of simplifying PRs:

  • This PR introduces a setting tidb_strict_integer_display_width. The default is OFF. When set to ON, the following behaviors apply: (1) SHOW CREATE TABLE does not show display length (2) Creating a table that has display length generates this warning.

  • A future PR changes the default for tidb_strict_integer_display_width, and updates all test cases to support the new default.

What do you think? (The actual setting name may need some discussion. There is nothing else called tidb_strict_* yet, but several things called tidb_enable* or tidb_skip*.

@AilinKid
Copy link
Contributor Author

  • tidb_strict_integer_display_width. The default is OFF. When set to ON, the following behaviors apply: (1) SHOW CREATE TABLE does not show display leng

Sounds great

util/misc.go Outdated Show resolved Hide resolved
@AilinKid AilinKid requested a review from a team as a code owner August 18, 2020 08:18
@AilinKid AilinKid requested review from qw4990 and removed request for a team August 18, 2020 08:18
@AilinKid
Copy link
Contributor Author

PTAL @tangenta @djshow832 @zimulala

@github-actions github-actions bot added the sig/execution SIG execution label Aug 20, 2020
Signed-off-by: AilinKid <314806019@qq.com>
.
Signed-off-by: AilinKid <314806019@qq.com>
Signed-off-by: AilinKid <314806019@qq.com>
.
Signed-off-by: AilinKid <314806019@qq.com>
Signed-off-by: AilinKid <314806019@qq.com>
Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 21, 2020
@AilinKid
Copy link
Contributor Author

/run-check_dev_2

@AilinKid
Copy link
Contributor Author

/run-unit-test

Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -5298,3 +5299,59 @@ func init() {
domain.SchemaOutOfDateRetryInterval = int64(50 * time.Millisecond)
domain.SchemaOutOfDateRetryTimes = int32(50)
}

func (s *testSerialDBSuite) TestCreateTableWithIntegerLengthWaring(c *C) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (s *testSerialDBSuite) TestCreateTableWithIntegerLengthWaring(c *C) {
func (s *testSerialDBSuite) TestCreateTableWithIntegerLengthWarning(c *C) {

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 24, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 24, 2020
@AilinKid
Copy link
Contributor Author

/run-all-tests

@AilinKid
Copy link
Contributor Author

/run-common-test

@AilinKid
Copy link
Contributor Author

/run-integration-ddl-test

@AilinKid
Copy link
Contributor Author

/run-common-test

@AilinKid
Copy link
Contributor Author

/run-integration-ddl-test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/config sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants