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

expression: fix TIMESTAMP func get wrong result with decimal (#15185) (#20088) #20469

Merged
merged 7 commits into from
Nov 11, 2020

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #20088 to release-3.0


What problem does this PR solve?

Issue Number: close #15185

Problem Summary:
The TIMESTAMP function deal with argument of decimal/float will get wrong result.
e.g.:
Run sql :
SELECT TIMESTAMP(11111.1111);
Expected:
2001-11-11 00:00:00.0000
Actual:
2011-11-01 11:11:00.0000

The reason is that the parse logic of datetime is different when dealing with strings and numbers.

In short, when dealing with numbers, if number length are not 6, 8, 12, 14, the parse logic should padded with leading zeros to the closest length.

When dealing with strings, the parse logic will interpreted from left to right to find year, month, day, hour... for as many parts as are present in the string.

For more details, see
https://dev.mysql.com/doc/refman/5.7/en/date-and-time-literals.html.
https://github.com/mysql/mysql-server/blob/5.7/sql-common/my_time.c

What is changed and how it works?

What's Changed:
The original code treat strings and numbers as all the same.
To spilt that logic, I redirect method call of parseDateTime() to ParseDatetimeFromNum() when the flag "isFloat" is true.

Check List

Tests

  • Unit test
  • Integration test

Release note

  • fix function timestamp() get wrong result when input argument is type of float/decimal

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ichn-hu
Copy link
Contributor

ichn-hu commented Oct 20, 2020

@LENSHOOD Hi, would you like to resolve the conflicts of this cherry-pick?

@LENSHOOD
Copy link
Contributor

@LENSHOOD Hi, would you like to resolve the conflicts of this cherry-pick?

sure, i'll give it a try.

@LENSHOOD
Copy link
Contributor

@LENSHOOD Hi, would you like to resolve the conflicts of this cherry-pick?

sure, i'll give it a try.

@ichn-hu PTAL ti-srebot#8

@LENSHOOD
Copy link
Contributor

@ti-srebot /run-all-tests

@LENSHOOD
Copy link
Contributor

@ti-srebot /run-sqllogic-test-1

Copy link
Contributor

@ichn-hu ichn-hu 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 Oct 21, 2020
@ichn-hu
Copy link
Contributor

ichn-hu commented Oct 21, 2020

@lzmhhh123 PTAL

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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 28, 2020
@bb7133
Copy link
Member

bb7133 commented Nov 11, 2020

/merge

@ti-srebot
Copy link
Contributor Author

Sorry @bb7133, this branch cannot be merged without an approval of release maintainers

@bb7133
Copy link
Member

bb7133 commented Nov 11, 2020

/run-all-tests

@bb7133 bb7133 merged commit b157b89 into pingcap:release-3.0 Nov 11, 2020
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/LGT2 Indicates that a PR has LGTM 2. type/3.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants