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

types: match MySQL behavior with datetime delimiters #17376

Merged
merged 4 commits into from
May 29, 2020

Conversation

smola
Copy link
Contributor

@smola smola commented May 23, 2020

What problem does this PR solve?

Issue Number: close #17277

Problem Summary: select date '2020-02--1' fails

What is changed and how it works?

  • types.ParseDateFormat changed datetime splitting to match MySQL 5.7.
  • Any ASCII punctuation character (i.e. printable non-alphanumeric) is a valid delimiter.
  • Additionally, (space) and T are valid delimiters if they appear between time and date.
  • Multiple consecutive delimiters are accepted, and handled as a single delimiter.
  • Consecutive delimiters can be mixed (e.g. -./ or T/TT).
  • This PR does not apply to types.ParseDuration, since it has additional validation.

Check List

  • Unit tests

Backwards compatibility

  • In previous TiDB versions, T and (space) were valid delimiters anywhere in a datetime string. After this change, using T or (space) as a delimiter is an error except between date and time.

Release note

  • Datetime parsing now matches MySQL 5.7 behavior for delimiters.

@smola smola requested a review from a team as a code owner May 23, 2020 16:25
@CLAassistant
Copy link

CLAassistant commented May 23, 2020

CLA assistant check
All committers have signed the CLA.

@sre-bot sre-bot added the contribution This PR is from a community contributor. label May 23, 2020
@ghost ghost requested review from qw4990 and removed request for a team May 23, 2020 16:26
@Reminiscent
Copy link
Contributor

The unit-test is failed. Please fix them. Thanks!

[2020-05-25T07:44:53.516Z] FAIL: integration_test.go:1174: testIntegrationSuite2.TestTimeBuiltin

[2020-05-25T07:44:53.516Z] 

[2020-05-25T07:44:53.516Z] integration_test.go:1446:

[2020-05-25T07:44:53.516Z]     result = tk.MustQuery("select addtime(a, b), addtime(cast(a as date), b), addtime(b,a), addtime(a,c), addtime(b," +

[2020-05-25T07:44:53.516Z]         "c), addtime(c,a), addtime(c,b)" +

[2020-05-25T07:44:53.516Z]         " from t;")

[2020-05-25T07:44:53.516Z] /home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:288:

[2020-05-25T07:44:53.516Z]     tk.c.Check(err, check.IsNil, comment)

[2020-05-25T07:44:53.516Z] ... value *errors.withStack = [types:1292]Incorrect time value: '{0 0 0 1 1 1 0}' ("[types:1292]Incorrect time value: '{0 0 0 1 1 1 0}'")

[2020-05-25T07:44:53.516Z] ... sql:select addtime(a, b), addtime(cast(a as date), b), addtime(b,a), addtime(a,c), addtime(b,c), addtime(c,a), addtime(c,b) from t;, args:[]

[2020-05-25T07:44:53.516Z] 

[2020-05-25T07:44:53.516Z] integration_test.go:1449:

[2020-05-25T07:44:53.516Z]     result.Check(testkit.Rows("<nil> <nil> <nil> 2017-01-01 13:31:32 2017-01-01 13:31:32 <nil> <nil>"))

[2020-05-25T07:44:53.516Z] /home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:62:

[2020-05-25T07:44:53.516Z]     res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)

[2020-05-25T07:44:53.516Z] ... obtained string = ""

[2020-05-25T07:44:53.516Z] ... expected string = "[<nil> <nil> <nil> 2017-01-01 13:31:32 2017-01-01 13:31:32 <nil> <nil>]\n"

[2020-05-25T07:44:53.516Z] ... sql:select addtime(a, b), addtime(cast(a as date), b), addtime(b,a), addtime(a,c), addtime(b,c), addtime(c,a), addtime(c,b) from t;, args:[]

@smola
Copy link
Contributor Author

smola commented May 26, 2020

@Reminiscent Oops. I missed that one. I have fixed the integration test, it used 2017 01-01 12:30:31 as a date, which produces an error in MySQL (see the initial notes about backwards compatibility). So I changed the test to 2017-01-01 12:30:31.

Just for the record, I tested on MySQL 8 with the following:

create database if not exists test;
use test;
drop table if exists t;
create table t(a datetime, b timestamp, c time);
insert into t values("2017 01-01 12:30:31", "2017 01-01 12:30:31", "01:01:01");

This produces:

Error Code: 1292. Incorrect datetime value: '2017 01-01 12:30:31' for column 'a' at row 1

I still have 4 unit tests failing locally, but they fail on master too. They are probably related to #17013

@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

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

@@             Coverage Diff             @@
##             master     #17376   +/-   ##
===========================================
  Coverage   79.9031%   79.9031%           
===========================================
  Files           520        520           
  Lines        140385     140385           
===========================================
  Hits         112172     112172           
  Misses        19248      19248           
  Partials       8965       8965           

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 @Reminiscent PTAL

Copy link
Contributor

@Reminiscent Reminiscent left a comment

Choose a reason for hiding this comment

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

LGTM. Great work!

@Reminiscent Reminiscent added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels May 29, 2020
@Reminiscent Reminiscent removed the request for review from qw4990 May 29, 2020 02:55
@sre-bot
Copy link
Contributor

sre-bot commented May 29, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 29, 2020

@smola merge failed.

@Reminiscent
Copy link
Contributor

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 29, 2020

/run-all-tests

@sre-bot sre-bot merged commit f1b21b5 into pingcap:master May 29, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request May 29, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented May 29, 2020

cherry pick to release-3.0 in PR #17499

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request May 29, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented May 29, 2020

cherry pick to release-3.1 in PR #17500

@sre-bot
Copy link
Contributor

sre-bot commented May 29, 2020

cherry pick to release-4.0 in PR #17501

@smola smola deleted the issue-17277 branch May 31, 2020 15:14
ti-srebot added a commit that referenced this pull request Jun 28, 2020
* cherry pick #17376 to release-3.0

Signed-off-by: sre-bot <sre-bot@pingcap.com>

* solve conflicts

* address comments

Co-authored-by: Santiago M. Mola <santi@mola.io>
Co-authored-by: Reminiscent <y870414423@gmail.com>
Co-authored-by: ti-srebot <66930949+ti-srebot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

select date '2020-02--1' fails
5 participants