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.

Sign up for GitHub

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

Jump to bottom

expression: Improve the compatibility of str_to_date #25386

Merged
merged 9 commits into from
Jun 25, 2021
Merged

expression: Improve the compatibility of str_to_date #25386

merged 9 commits into from
Jun 25, 2021

Conversation

JaySon-Huang
Copy link
Contributor

What problem does this PR solve?

Issue Number: related to #24928

Problem Summary:

  • '%b'/'%M' should be case insensitive
  • Improve the compatibility of parsing some input for "%r" and "%T"

What is changed and how it works?

  • Use strings.EqualFold for case insensitive prefix comparing in fullNameMonth
  • Make abbreviatedMonth case insensitive
  • Make time12Hour and time24Hour accept some irregular inputs

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

  • Performance regression
    • Consumes more CPU
  • Breaking backward compatibility

Release note

  • Improve the MySQL compatibility of str_to_date for %b/%M/%r/%T

@JaySon-Huang
%b/%M should be case insensitive
cf07daf 
Signed-off-by: JaySon-Huang <tshent@qq.com>
@JaySon-Huang JaySon-Huang requested a review from a team as a code owner June 13, 2021 16:10
@JaySon-Huang JaySon-Huang requested review from wshwsh12 and removed request for a team June 13, 2021 16:10
@ti-chi-bot ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 13, 2021
@JaySon-Huang JaySon-Huang force-pushed the improve_str_to_date branch 2 times, most recently from 2da73c5 to afc0923 Compare June 13, 2021 19:46
@JaySon-Huang JaySon-Huang mentioned this pull request Jun 13, 2021
Merged
@JaySon-Huang
improve compatibility for %r %T
291c392 
Signed-off-by: JaySon-Huang <tshent@qq.com>
@JaySon-Huang
More test cases
a95a749 
Signed-off-by: JaySon-Huang <tshent@qq.com>
@JaySon-Huang
Fix typo
acd6a7f 
Signed-off-by: JaySon-Huang <tshent@qq.com>
@JaySon-Huang
Add more test for AllowInvalidDate
6addc15 
Signed-off-by: JaySon-Huang <tshent@qq.com>
@JaySon-Huang JaySon-Huang mentioned this pull request Jun 15, 2021
Closed
wshwsh12
wshwsh12 reviewed Jun 21, 2021
View reviewed changes
types/time.go
Comment on lines +3009 to +3011
result := oneOrTwoDigitRegex.FindString(input) // 1..12
length := len(result)
hour, succ := parseDigits(input, length)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these lines extract a function?
I think extract a function like func parseOneOrTwoDigit(input string) (num int, succ bool, input string) can increase the readability of the code

Copy link
Contributor Author

@JaySon-Huang JaySon-Huang Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Actually finding the length by oneOrTwoDigitRegex is useless and costly, I combine them into one function in #25389 and improve the performance by about 50%.

I'd like to keep these code aligned with other parsing functions and will change them totally in that PR.

types/time.go Outdated
Comment on lines 3026 to 3028
if input = skipWhiteSpace(input); len(input) == 0 {
return input, parseStateEndOfLine
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think add skipWhiteSpace into parseSep when state is parseStateNormal is better. So the input can used by the next digit parse.

Copy link
Contributor

Choose a reason for hiding this comment

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

And why need three states? I think fail or succ is enough..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why need three states? I think fail or succ is enough..

Because MySQL accepts parsing some inputs like "11:13" with format "%r", which can be parsed into a datetime "11:13:00".

In parSep, we may run into three situations:

  1. meet an ":", then we should accept it and continue to parse string left
  2. meet a char that is not ":", then we should stop the parsing and return a "ZeroTime"
  3. meet the end of the input string, then we should stop the parsing, with some parsing result

There are three different following actions, so we need three different states.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add skipWhiteSpace into parseSep when state is parseStateNormal

Address

wshwsh12
wshwsh12 approved these changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 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-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 23, 2021
@wshwsh12 wshwsh12 requested a review from lzmhhh123 June 24, 2021 06:14
@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Jun 25, 2021 •
edited
Loading

@lzmhhh123 PTAL. plans to release it in v5.0.3

@wshwsh12 wshwsh12 requested a review from guo-shaoge June 25, 2021 06:23
@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • guo-shaoge
  • wshwsh12

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 25, 2021
@JaySon-Huang
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 7955cde

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 25, 2021
@JaySon-Huang
Copy link
Contributor Author

/label needs-cherry-pick-5.1

@JaySon-Huang
Copy link
Contributor Author

/run-check_dev_2

@JaySon-Huang
Copy link
Contributor Author 

[2021-06-25T10:33:36.123Z] panic: test timed out after 9m0s

Seems the failure is not related to the changes in this PR?

@JaySon-Huang
Copy link
Contributor Author

/run-check_dev_2

@JaySon-Huang
Copy link
Contributor Author

/merge

@ti-chi-bot ti-chi-bot merged commit 8434069 into pingcap:master Jun 25, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 25, 2021
@JaySon-Huang @ti-srebot
cherry pick pingcap#25386 to release-5.0
5ac3d52 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jun 25, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #25767

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 25, 2021
@JaySon-Huang @ti-srebot
cherry pick pingcap#25386 to release-5.1
3d2f0ab 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jun 25, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-5.1 in PR #25768

@JaySon-Huang JaySon-Huang deleted the improve_str_to_date branch June 25, 2021 12:04
jiajunhuang pushed a commit to jiajunhuang/tidb that referenced this pull request Jun 28, 2021
@JaySon-Huang @jiajunhuang
expression: Improve the compatibility of str_to_date (pingcap#25386)
20a9806
ti-chi-bot pushed a commit that referenced this pull request Jun 28, 2021
@ti-srebot
expression: Improve the compatibility of str_to_date (#25386) (#25767)
c8b2f25
ti-chi-bot pushed a commit that referenced this pull request Jul 21, 2021
@ti-srebot
expression: Improve the compatibility of str_to_date (#25386) (#25768)
810e2f9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guo-shaoge guo-shaoge guo-shaoge approved these changes

@wshwsh12 wshwsh12 wshwsh12 approved these changes

@lzmhhh123 lzmhhh123 Awaiting requested review from lzmhhh123

Assignees
No one assigned
Labels
component/expression needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JaySon-Huang @ti-chi-bot @ti-srebot @guo-shaoge @wshwsh12 @wjhuang2016