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: validate fsp for datetime/timestamp if they are used as defaultvalue of datetime/timestamp columns #9327

Merged
merged 14 commits into from Mar 4, 2019

Conversation

@spongedu
Copy link
Contributor

spongedu commented Feb 16, 2019

What problem does this PR solve?

As is described in MySQL Reference and pingcap/parser#186:

If a TIMESTAMP or DATETIME column definition includes an explicit fractional seconds precision value anywhere, the same value must be used throughout the column definition.

mysql> create table tbl_so_q23671222_1( ts timestamp(3) default now() );
ERROR 1067 (42000): Invalid default value for 'ts'
mysql> create table tbl_so_q23671222_1( ts timestamp(3) default now(3) );
Query OK, 0 rows affected (0.59 sec)
mysql> create table tbl_so_q23671222_2( ts timestamp(3) default current_timestamp );
ERROR 1067 (42000): Invalid default value for 'ts'
mysql> create table tbl_so_q23671222_2( ts timestamp(3) default current_timestamp(3) );
Query OK, 0 rows affected (0.38 sec)

These checks are missing now in TiDB. This PR fixes this.

What is changed and how it works?

Add related check in ddl_api.go

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

This pr depends on pingcap/parser#211

@spongedu

This comment has been minimized.

Copy link
Contributor Author

spongedu commented Feb 17, 2019

/run-all-tests

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

tiancaiamao commented Feb 18, 2019

CI fails @spongedu

@morgo morgo added the contribution label Feb 23, 2019

@winkyao

This comment has been minimized.

Copy link
Member

winkyao commented Feb 26, 2019

@spongedu please fix ci.

@spongedu

This comment has been minimized.

Copy link
Contributor Author

spongedu commented Feb 26, 2019

/run-all-tests

@spongedu

This comment has been minimized.

Copy link
Contributor Author

spongedu commented Feb 27, 2019

/run-all-tests

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

zhouqiang-cl commented Feb 27, 2019

/run-all-tests

@spongedu

This comment has been minimized.

Copy link
Contributor Author

spongedu commented Feb 27, 2019

@winkyao @tiancaiamao PTAL :) It seems that the test cases need update.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 28, 2019

Codecov Report

Merging #9327 into master will increase coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9327      +/-   ##
==========================================
+ Coverage   67.38%   67.39%   +0.01%     
==========================================
  Files         374      374              
  Lines       78728    78738      +10     
==========================================
+ Hits        53049    53064      +15     
+ Misses      20905    20901       -4     
+ Partials     4774     4773       -1
Impacted Files Coverage Δ
ddl/ddl.go 89.66% <100%> (+0.04%) ⬆️
ddl/ddl_api.go 76.97% <46.15%> (-0.17%) ⬇️
executor/distsql.go 71.72% <0%> (-0.46%) ⬇️
store/tikv/scan.go 77.31% <0%> (+3.36%) ⬆️
ddl/delete_range.go 79.36% <0%> (+4.23%) ⬆️

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 0b371ea...bd77251. Read the comment docs.

@xiekeyi98

This comment has been minimized.

Copy link
Member

xiekeyi98 commented Feb 28, 2019

/run-all-tests

@xiekeyi98

This comment has been minimized.

Copy link
Member

xiekeyi98 commented Feb 28, 2019

/run-common-test tidb-test=pr/747

@xiekeyi98

This comment has been minimized.

Copy link
Member

xiekeyi98 commented Feb 28, 2019

/run-all-tests tidb-test=pr/747

@xiekeyi98

This comment has been minimized.

Copy link
Member

xiekeyi98 commented Feb 28, 2019

/run-all-tests

@xiekeyi98

This comment has been minimized.

Copy link
Member

xiekeyi98 commented Feb 28, 2019

/run-common-test tidb-test=pr/747
/run-intergration-common-test tidb-test=pr/747

@xiekeyi98

This comment has been minimized.

Copy link
Member

xiekeyi98 commented Feb 28, 2019

/run-integration-common-test tidb-test=pr/747

@xiekeyi98
Copy link
Member

xiekeyi98 left a comment

LGTM
@winkyao @crazycs520 PTAL_(:з」∠)_

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

tiancaiamao commented Mar 1, 2019

LGTM

@tiancaiamao tiancaiamao added status/LGT2 and removed status/LGT1 labels Mar 1, 2019

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

tiancaiamao commented Mar 1, 2019

@zimulala
Copy link
Member

zimulala left a comment

LGTM

@zimulala zimulala added status/LGT3 and removed status/LGT2 labels Mar 1, 2019

@spongedu

This comment has been minimized.

Copy link
Contributor Author

spongedu commented Mar 1, 2019

conflicts resolved. PTAL :) ~ @zimulala @tiancaiamao

spongedu added some commits Mar 2, 2019

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

tiancaiamao commented Mar 4, 2019

LGTM

@tiancaiamao tiancaiamao merged commit 0267a7f into pingcap:master Mar 4, 2019

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.