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

Merged
merged 5 commits into from
Oct 15, 2020

Conversation

LENSHOOD
Copy link
Contributor

@LENSHOOD LENSHOOD commented Sep 18, 2020

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

@LENSHOOD LENSHOOD requested a review from a team as a code owner September 18, 2020 03:20
@LENSHOOD LENSHOOD requested review from lzmhhh123 and removed request for a team September 18, 2020 03:20
@ti-srebot ti-srebot added the contribution This PR is from a community contributor. label Sep 18, 2020
@LENSHOOD
Copy link
Contributor Author

/run-all-tests

@LENSHOOD
Copy link
Contributor Author

/run-all-tests

ichn-hu
ichn-hu previously approved these changes Sep 18, 2020
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 Sep 18, 2020
Copy link
Contributor

@Patrick0308 Patrick0308 left a comment

Choose a reason for hiding this comment

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

This pr may be a part of #11178. @LENSHOOD

@LENSHOOD
Copy link
Contributor Author

This pr may be a part of #11178. @LENSHOOD

It's not, this PR is for fix wrong cast logic of timestamp() dealing with "11111.1111" or 11111.1111.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
mmarchini mary marchini
…#15185)

The TIMESTAMP function deal with argument of decimal/float will get
wrong result. This commit fix that issue by redirect method call of
parseDateTime() to ParseDatetimeFromNum() when the flag "isFloat" is
true.

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. But 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.
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 removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 21, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 21, 2020
@lzmhhh123
Copy link
Contributor

/merge

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

/run-all-tests

@ti-srebot
Copy link
Contributor

@LENSHOOD merge failed.

@LENSHOOD
Copy link
Contributor Author

/run-integration-copr-test

@Patrick0308
Copy link
Contributor

@LENSHOOD It's work for dayofmonth(120.00), because the expression of dayofmonth use ParseTimeFromFloatString.

@LENSHOOD
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

@LENSHOOD Oops! auto merge is restricted to Committers of the SIG.See the corresponding SIG page for more information. Related SIG: execution(slack).

@LENSHOOD
Copy link
Contributor Author

/run-all-tests

@LENSHOOD
Copy link
Contributor Author

@ti-srebot /run-all-tests

@LENSHOOD
Copy link
Contributor Author

LENSHOOD commented Oct 4, 2020

@SunRunAway @lzmhhh123
hi guys, I've proposed a tikv PR(tikv/tikv#8737) to fix the copr-test failed.
Now the run all test with "tikv=pr/8737" can get all pass, please take a look.
I'm wondering what's the next step?

@LENSHOOD let's wait for the review of tikv/tikv#8737. These two PRs need to be merged nearly at the same time in order not to break the integration test.

Got it. Thx!

@breezewish
Copy link
Member

/run-integration-copr-test tikv=pr/8737

Verified

This commit was signed with the committer’s verified signature. The key has expired.
mmarchini mary marchini
@SunRunAway
Copy link
Contributor

/reward 300

@ti-challenge-bot
Copy link

You are not the mentor for the linked issue.

@breezewish
Copy link
Member

/run-all-tests tikv=pr/8737

@SunRunAway
Copy link
Contributor

/reward 300

@ti-challenge-bot
Copy link

Reward success.

@LENSHOOD
Copy link
Contributor Author

@ti-srebot /run-all-tests tikv=pr/8737

@ti-srebot
Copy link
Contributor

/run-all-tests tikv=pr/8737

@zhouqiang-cl
Copy link
Contributor

@ti-srebot /run-all-tests tikv=pr/8737

tics test is not working for pr/xxx format, it need sometime to ensure it, you can ignore the tics test for a while

@LENSHOOD
Copy link
Contributor Author

@ti-srebot /run-all-tests tikv=pr/8737

tics test is not working for pr/xxx format, it need sometime to ensure it, you can ignore the tics test for a while

Thx, so is this PR need to manually merge due to the tests are not fully passed? (cannot use /merge command)

@ti-srebot
Copy link
Contributor

/run-all-tests tikv=pr/8737

tics test is not working for pr/xxx format, it need sometime to ensure it, you can ignore the tics test for a while

Thx, so is this PR need to manually merge due to the tests are not fully passed? (cannot use command)

@zhouqiang-cl
Copy link
Contributor

@ti-srebot /run-all-tests tikv=pr/8737

tics test is not working for pr/xxx format, it need sometime to ensure it, you can ignore the tics test for a while

Thx, so is this PR need to manually merge due to the tests are not fully passed? (cannot use /merge command)

Yes. Now it need

LENSHOOD and others added 2 commits October 15, 2020 09:16

Unverified

This user has not yet uploaded their public signing key.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@SunRunAway
Copy link
Contributor

/run-all-tests

@SunRunAway SunRunAway merged commit 8ed9a6f into pingcap:master Oct 15, 2020
@ti-challenge-bot
Copy link

@LENSHOOD, Congratulations, you get 300 in this PR, and your total score is 300 in high-performance challenge program.

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Oct 15, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #20469

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Oct 15, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #20470

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/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

different results of function TIMESTAMP with float/decimal argument
9 participants