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: use original column name in duplicate column error #9291

Merged
merged 4 commits into from Feb 14, 2019

Conversation

u5surf
Copy link
Contributor

@u5surf u5surf commented Feb 13, 2019

What problem does this PR solve?

fix #9285

What is changed and how it works?

Check List

$mysql -u root -P 4000 -h 127.0.0.1
Welcome to the MySQL monitor.  Commands end with ; or \g.
Your MySQL connection id is 1
Server version: 5.7.10-TiDB-v3.0.0-beta-52-g55da8ad9f MySQL Community Server (Apache License 2.0)

Copyright (c) 2000, 2017, Oracle and/or its affiliates. All rights reserved.

Oracle is a registered trademark of Oracle Corporation and/or its
affiliates. Other names may be trademarks of their respective
owners.

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

mysql> show databases;
+--------------------+
| Database           |
+--------------------+
| INFORMATION_SCHEMA |
| PERFORMANCE_SCHEMA |
| mysql              |
| test               |
+--------------------+
4 rows in set (0.00 sec)

mysql> use test;
Database changed
mysql> show tables;
Empty set (0.00 sec)

mysql> create table tt( ID123 int , ID123 int );
ERROR 1060 (42S21): Duplicate column name 'ID123'
mysql> create table tt( charID varchar(10) , charID varchar(10) charset utf8mb4);
ERROR 1060 (42S21): Duplicate column name 'charID'
mysql> create table tt( WHYNOTUPPER char , WHYNOTUPPER int);
ERROR 1060 (42S21): Duplicate column name 'WHYNOTUPPER'
mysql>

Code changes

  • Has exported function/method change
    no
  • Has exported variable/fields change
    no
  • Has interface methods change
    no
  • Has persistent data change
    no

Side effects

  • Possible performance regression
    probably no
  • Increased code complexity
    no
  • Breaking backward compatibility
    no

Related changes

  • Need to cherry-pick to the release branch
    nothing
  • Need to update the documentation
    nothing
  • Need to update the tidb-ansible repository
    ?
  • Need to be included in the release note
    probably nothing??

@CLAassistant
Copy link

CLAassistant commented Feb 13, 2019

CLA assistant check
All committers have signed the CLA.

@xiekeyi98 xiekeyi98 added type/compatibility contribution This PR is from a community contributor. labels Feb 13, 2019
Copy link
Contributor

@xiekeyi98 xiekeyi98 left a comment

Choose a reason for hiding this comment

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

Thanks for your contirbution.
I was wondering if you could add some unit test to avoid this error next time?

@codecov-io
Copy link

codecov-io commented Feb 13, 2019

Codecov Report

Merging #9291 into master will increase coverage by <.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9291      +/-   ##
==========================================
+ Coverage   67.16%   67.16%   +<.01%     
==========================================
  Files         371      371              
  Lines       77505    77505              
==========================================
+ Hits        52057    52058       +1     
+ Misses      20794    20793       -1     
  Partials     4654     4654
Impacted Files Coverage Δ
ddl/ddl_api.go 74.64% <71.42%> (ø) ⬆️
util/systimemon/systime_mon.go 80% <0%> (-20%) ⬇️
executor/distsql.go 73% <0%> (+0.46%) ⬆️
store/tikv/lock_resolver.go 42.65% <0%> (+0.94%) ⬆️

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 65066c8...6838186. Read the comment docs.

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

Please add some UT to cover this bug.

ddl/ddl_api.go Outdated Show resolved Hide resolved
@zz-jason zz-jason changed the title Fix error meessage about duplicate column name always lower case #9285 ddl: use original column name in duplicate column error message Feb 14, 2019
@zz-jason zz-jason changed the title ddl: use original column name in duplicate column error message ddl: use original column name in duplicate column error Feb 14, 2019
@u5surf
Copy link
Contributor Author

u5surf commented Feb 14, 2019

i fixed TestFieldCase to check column name case strictly.

before:(if column name includes bigcase character as Field)

PASS: column_test.go:844: testColumnSuite.TestDropColumn	0.192s

----------------------------------------------------------------------
FAIL: column_test.go:960: testColumnSuite.TestFieldCase

column_test.go:975:
    c.Assert(err.Error(), Equals, infoschema.ErrColumnExists.GenWithStackByArgs("Field").Error())
... obtained string = "[schema:1060]Duplicate column name 'field'"
... expected string = "[schema:1060]Duplicate column name 'Field'"


----------------------------------------------------------------------

after: fixed

ok  	github.com/pingcap/tidb/config	0.022s	coverage: 18.9% of statements
ok  	github.com/pingcap/tidb/ddl	48.576s	coverage: 78.6% of statements

@zhouqiang-cl
Copy link
Contributor

/rebuild

@zz-jason
Copy link
Member

LGTM

@zz-jason
Copy link
Member

/run-all-tests

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 14, 2019
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 14, 2019
@zz-jason zz-jason merged commit 6a98441 into pingcap:master Feb 14, 2019
@zz-jason
Copy link
Member

@u5surf Please cherry pick this commit to release 2.1 and release 2.0 branches.

@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/LGT2 Indicates that a PR has LGTM 2. type/compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error message about "create table" is not same with MYSQL
8 participants