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: ignore integer zerofill size attribute when changing the column types #20862

Merged
merged 11 commits into from
Nov 11, 2020

Conversation

AilinKid
Copy link
Contributor

@AilinKid AilinKid commented Nov 5, 2020

What problem does this PR solve?

Issue Number: close #20529

What is changed and how it works?

how does it work?

Like int(3), bigint(2), the number enclosed will be stored as flen in the field type, while it shouldn't be concerned when we make column type change from int(3) to(2) . Because bigint is quite larger than int, there is no need to take a reorg process.

what is changed

We should use the default flen of the specified field type rather than the origin flen.

Check List

Tests

  • Unit test
  • Integration test

Release note

  • ddl: ignore integer zerofill size attribute when changing the column types

AilinKid and others added 3 commits November 5, 2020 14:32
Signed-off-by: AilinKid <314806019@qq.com>
.
Signed-off-by: AilinKid <314806019@qq.com>
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Nov 5, 2020
@bb7133 bb7133 changed the title ddl: column type change shouldn't take integer display length into consideration ddl: ignore integer zerofill size attribute when changing the column types Nov 5, 2020
ddl/column_type_change_test.go Outdated Show resolved Hide resolved
ddl/column_type_change_test.go Outdated Show resolved Hide resolved
.
Signed-off-by: AilinKid <314806019@qq.com>
@AilinKid AilinKid added type/bug The issue is confirmed as a bug. type/bugfix This PR fixes a bug. and removed type/bug The issue is confirmed as a bug. labels Nov 5, 2020
ddl/column.go Outdated Show resolved Hide resolved
ddl/column.go Outdated
Comment on lines 696 to 698
needTruncationOrToggleSignForInteger := func() bool {
return newColFlen > 0 && newColFlen < oldColFlen || toUnsigned != originUnsigned
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use a variable instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cause for most of the cases, we don't need to trigger the computation of it, like a kind of lazy call if necessary?

ddl/column.go Outdated Show resolved Hide resolved
Copy link
Member

@wjhuang2016 wjhuang2016 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 Nov 11, 2020
@AilinKid
Copy link
Contributor Author

/run-all-tests

@bb7133
Copy link
Member

bb7133 commented Nov 11, 2020

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 11, 2020
@bb7133
Copy link
Member

bb7133 commented Nov 11, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 11, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@AilinKid merge failed.

@AilinKid
Copy link
Contributor Author

/run-unit-test

1 similar comment
@AilinKid
Copy link
Contributor Author

/run-unit-test

@AilinKid AilinKid merged commit 38f8760 into pingcap:master Nov 11, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Nov 11, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #20986

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integer zerofill size attribute should not prevent type modification
5 participants