-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
expression: Improve the performance of str_to_date
#25389
expression: Improve the performance of str_to_date
#25389
Conversation
b6f9fdb
to
ded724f
Compare
/run-check_dev_2 |
This pr is depending on #25386, so I cancel the review request now. After that pr merged, feel free to re-request review again. |
cce42b2
to
6af9e3a
Compare
@wshwsh12 PTAL if you are free |
6af9e3a
to
9cc92e5
Compare
} | ||
|
||
func dayOfYearThreeDigits(t *CoreTime, input string, ctx map[string]int) (string, bool) { | ||
v, succ := parseDigits(input, 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems you have changed the behavior of dayOfYearThreeDigits
.
Please add some test case that len(input) < 3, and compare the result with mysql.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MySQL declares that "%j" should be "Day of year (001..366)"[1]. But actually, it accepts a number that is up to three digits[2]. (The same with "%d", "Day of the month, numeric (00..31)" but still accept one digit as the day).
TiDB add parsing function dayOfYearThreeDigits
for '%j', but actually it doesn't support parsing format with '%j' [3]. So it is not easy to add cases for testing this function by testing StrToDate.
Instead of adding cases for 'dayOfYearThreeDigits', I think I will add some test cases for parseNDigits
, later.
- [1] https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_date-format
- [2] Some result for '%j' in MySQL 5.7
> select date, format, str_to_date(date, format) as str_to_date from a; +-----------+--------+----------------------------+ | date | format | str_to_date | +-----------+--------+----------------------------+ | 015 2021 | %j %Y | 2021-01-15 00:00:00.000000 | | 15 2021 | %j %Y | 2021-01-15 00:00:00.000000 | | 9 2021 | %j %Y | 2021-01-09 00:00:00.000000 | | 900 2021 | %j %Y | 2023-06-19 00:00:00.000000 | | 999 2021 | %j %Y | 2023-09-26 00:00:00.000000 | | 1000 2021 | %j %Y | 2000-04-09 00:00:00.000000 | -- don't know why, need further investigation | 9999 2021 | %j %Y | 2011-09-26 00:00:00.000000 | -- don't know why, need further investigation +-----------+--------+----------------------------+ > select version(); +-------------------------+ | version() | +-------------------------+ | 5.7.33-0ubuntu0.16.04.1 | +-------------------------+
- [3]
Lines 2752 to 2756 in 2093349
// Key of the ctx is the format char, such as `%j` `%p` and so on. if yearOfDay, ok := ctx["%j"]; ok { // TODO: Implement the function that converts day of year to yy:mm:dd. _ = yearOfDay }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding cases for 'dayOfYearThreeDigits', I think I will add some test cases for
parseNDigits
Find it hard to add test cases for the private method parseNDigits
, add more test cases for StrToDate
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
be7c42d
to
4c9490b
Compare
@wshwsh12 Could you find another reviewer to continue to review this PR when they have spare time? It has not been active for several weeks. |
/cc @guo-shaoge |
Will review this today. |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: f7f33e8
|
@JaySon-Huang: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
/run-unit-test |
/run_check_dev_2 |
/run-check_dev_2 |
What problem does this PR solve?
Issue Number: related to #24928
Problem Summary:
While parsing parts for
StrToDate
, we first match the length of the digit by regex then parsing the number bystrconv.ParseUint
. The regex matching is costly and somehow useless, we can rewriteparseDigits
to reduce the cost of parsing.What is changed and how it works?
Depending on #25386, should merge that PR first.
Use
parseNDigits
instead of matching length byoneOrTwoDigitRegex
then parsing number bystrconv.ParseUint
.Test the performance by
go test -run=^$ -bench=BenchmarkStrToDate -count=5 -v
Related changes
pingcap/docs
/pingcap/docs-cn
:Check List
Tests
Side effects
Release note
str_to_date