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: fix STR_TO_DATE handling for %h, %r (#18171) #18428

Merged
merged 4 commits into from
Jul 22, 2020
Merged

types: fix STR_TO_DATE handling for %h, %r (#18171) #18428

merged 4 commits into from
Jul 22, 2020

Conversation

dyzsr
Copy link
Contributor

@dyzsr dyzsr commented Jul 8, 2020

What problem does this PR solve?

Issue Number: close #18171

Problem Summary: Behavior of function STR_TO_DATE is not compatible with MySQL.

  • '%h:%i:%s' parses '12:mm:ss' into '12:mm:ss', which should be '00:mm:ss'.
  • '%r' parses '12:mm:ss AM' into '12:mm:ss' and parses '12:hh:ss PM' into 'NULL'.

What is changed and how it works?

What's Changed:

  • The hour part will change to 0 if '%h' matches 12 and no AM or PM occurs.
  • The hour part will subtract 12 if '%r' matches pattern '12:mm:ss AM' or '12:mm:ss PM'.

How it Works:

  • Function mysqlTimeFix inspects the occurrence of '%h', '%p' and whether the hour part is 12.
  • Function time12Hour now fixes the time when the hour part is 12.

Tests

  • Unit test
  • Manual test

Side effects

No.

Release note

  • types: fix STR_TO_DATE's handling for format token '%r', '%h'

+ str_to_date('12:30:17', '%h:%i:%s') -> '00:30:17'
+ str_to_date('12:30:17PM', '%r') -> '12:30:17'
  str_to_date('12:30:17AM', '%r') -> '00:30:17'
@dyzsr dyzsr requested a review from a team as a code owner July 8, 2020 01:59
@dyzsr dyzsr requested review from XuHuaiyu and removed request for a team July 8, 2020 01:59
@CLAassistant
Copy link

CLAassistant commented Jul 8, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jul 8, 2020

Codecov Report

Merging #18428 into master will increase coverage by 0.1955%.
The diff coverage is 100.0000%.

@@               Coverage Diff                @@
##             master     #18428        +/-   ##
================================================
+ Coverage   79.2508%   79.4463%   +0.1955%     
================================================
  Files           542        535         -7     
  Lines        146131     144237      -1894     
================================================
- Hits         115810     114591      -1219     
+ Misses        20994      20369       -625     
+ Partials       9327       9277        -50     

Copy link
Contributor

@lzmhhh123 lzmhhh123 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 Jul 20, 2020
@lzmhhh123 lzmhhh123 requested a review from qw4990 July 20, 2020 03:50
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.

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 21, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 21, 2020
@zz-jason
Copy link
Member

/merge

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

Your auto merge job has been accepted, waiting for:

  • 18568

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@zz-jason zz-jason merged commit cc0b6f1 into pingcap:master Jul 22, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 22, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #18725

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 22, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-3.1 in PR #18726

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 22, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #18727

dyzsr added a commit to ti-srebot/tidb that referenced this pull request Jul 22, 2020
ti-srebot added a commit that referenced this pull request Jul 24, 2020
ti-srebot added a commit that referenced this pull request Jul 28, 2020
@dyzsr dyzsr deleted the issue-18171 branch November 25, 2020 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StrToDate handling of %h(12) is not as expected
5 participants