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: update the error message of "modify/change column" to make it easier to understand #13457

Merged
merged 3 commits into from
Nov 25, 2019

Conversation

zimulala
Copy link
Contributor

What problem does this PR solve?

When executing "change/modify column" we encounter some error messages as follows:

create table t1 (a int(11) default null);
alter table t1 change a a varchar(16);

before this PR:

[ddl:8200]Unsupported modify charset from binary to utf8mb4

after this PR:

[ddl:8200]Unsupported modify column: type varchar(16) not match origin int(11)

Make this error message easier to understand.

What is changed and how it works?

Check the column type before checking the charset or other information.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Update the error message of "modify/change column"

@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #13457   +/-   ##
===========================================
  Coverage   80.1813%   80.1813%           
===========================================
  Files           473        473           
  Lines        117470     117470           
===========================================
  Hits          94189      94189           
  Misses        15878      15878           
  Partials       7403       7403

@zimulala zimulala closed this Nov 20, 2019
@zimulala zimulala deleted the err-msg branch November 20, 2019 08:42
@zimulala zimulala restored the err-msg branch November 21, 2019 06:49
@zimulala zimulala reopened this Nov 21, 2019
@zimulala
Copy link
Contributor Author

PTAL @jackysp @bb7133 @crazycs520 @tangenta

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

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

@tangenta tangenta added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 25, 2019
Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

LGTM

@AilinKid AilinKid added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 25, 2019
@zimulala zimulala added the status/can-merge Indicates a PR has been approved by a committer. label Nov 25, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 25, 2019

/run-all-tests

@sre-bot sre-bot merged commit 9cb05c7 into pingcap:master Nov 25, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 25, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 25, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 25, 2019

cherry pick to release-3.1 failed

zimulala added a commit to zimulala/tidb that referenced this pull request Nov 28, 2019
sre-bot pushed a commit that referenced this pull request Dec 3, 2019
zimulala added a commit that referenced this pull request Dec 3, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 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
sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants