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: year function can't handle some date string #26152

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

yuqi1129
Copy link
Contributor

@yuqi1129 yuqi1129 commented Jul 12, 2021

What problem does this PR solve?

Issue Number: close #26151

Check List

Tests

  • Unit test

Release note

  • No release note.

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

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 the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 12, 2021
@sre-bot
Copy link
Contributor

sre-bot commented Jul 12, 2021

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@yuqi1129 yuqi1129 changed the title Date string types: year function can't handle some date string Jul 12, 2021
@yuqi1129
Copy link
Contributor Author

/cc @wshwsh12 @lzmhhh123

@ti-chi-bot ti-chi-bot requested review from lzmhhh123 and wshwsh12 July 12, 2021 12:19
@yuqi1129
Copy link
Contributor Author

/cc @tisonkun

@ichn-hu ichn-hu mentioned this pull request Jul 13, 2021
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 14, 2021
@yuqi1129
Copy link
Contributor Author

@wshwsh12 @tisonkun , Can you review again ?

@tisonkun
Copy link
Contributor

@yuqi1129 can you give a brief analyze for the issue and your solution? Or comment on the original issue and refer from here.

This patch plays a lot low level manipulation which is theoretically brittle.

@yuqi1129
Copy link
Contributor Author

yuqi1129 commented Jul 15, 2021

@tisonkun
Take the date string 12:12:12:(length 9) and 12:12:12:::; (length 12), when come to the end of function ParseDateFormat, the value of start is 6(the last index of valid number split by ':') and 12(this means start come to end of date string), when the date string is normal like '2020-12-12'(start is 8), the algorithm currently used is OK but not some special case

My solution. as mention above, we should handle two case: start values equal to the length of string and substring from start to end contains invalid character.

for end = start; end < len(format) && !isValidSeparator(format[end], len(seps)); end++ {
}

I introduce variance end to represent the first invalid character from start index

if end != start

This is to handle the case when start equals to the length of date string

Another resolution: refactor the whole ParseDateFormat.

Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

@wshwsh12 is this function potentially pushed down to coprocessor? I'm afraid that TiKV or TiFlash implementation are different.

for end = start; end < len(format) && !isValidSeparator(format[end], len(seps)); end++ {
}

end = mathutil.Min(end, len(format))
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC golang builtin provides math.Min already, and github.com/cznic/mathutil is archived.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tisonkun You mean the built-in math.Min?, it only has double64 parameter method, correct me if i'am wrong

@tisonkun tisonkun requested review from wshwsh12 and lzmhhh123 July 30, 2021 02:39
@tisonkun
Copy link
Contributor

@wshwsh12 @lzmhhh123 PTAL

1 similar comment
@tisonkun
Copy link
Contributor

@wshwsh12 @lzmhhh123 PTAL

@wshwsh12
Copy link
Contributor

wshwsh12 commented Aug 5, 2021

Is this function potentially pushed down to coprocessor?

Yes. CaseXXXasTime maybe call the parse function. And the funtions have been push down to coprocessor.

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.

Seem some cases will also get wrong result.
For example.

[tidb]> select year('2011-11-11x');
ERROR 1105 (HY000): strconv.Atoi: parsing "11x": invalid syntax

But I don't have a good idea to cover all corner cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Year function can't handle some date string
6 participants