Skip to content

expression: fix getIntervalFromDecimal in DATE_ADD()#11297

Merged
SunRunAway merged 6 commits into
pingcap:masterfrom
wshwsh12:fix_date_microsecond
Jul 18, 2019
Merged

expression: fix getIntervalFromDecimal in DATE_ADD()#11297
SunRunAway merged 6 commits into
pingcap:masterfrom
wshwsh12:fix_date_microsecond

Conversation

@wshwsh12
Copy link
Copy Markdown
Contributor

@wshwsh12 wshwsh12 commented Jul 17, 2019

What problem does this PR solve?

#11286
Before this pr:

mysql> select DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 MINUTE_MICROSECOND);
+------------------------------------------------------------------+
| DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 MINUTE_MICROSECOND) |
+------------------------------------------------------------------+
| 2007-03-28 22:08:30.200000                                       |
+------------------------------------------------------------------+
1 row in set (0.00 sec)

mysql> select DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 HOUR_MICROSECOND);
+----------------------------------------------------------------+
| DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 HOUR_MICROSECOND) |
+----------------------------------------------------------------+
| 2007-03-28 22:08:30.200000                                     |
+----------------------------------------------------------------+
1 row in set (0.01 sec)

mysql> select DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 DAY_MICROSECOND);
+---------------------------------------------------------------+
| DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 DAY_MICROSECOND) |
+---------------------------------------------------------------+
| 2007-03-28 22:08:30.200000                                    |
+---------------------------------------------------------------+
1 row in set (0.00 sec)

This pr:

mysql> select DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 MINUTE_MICROSECOND);
+------------------------------------------------------------------+
| DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 MINUTE_MICROSECOND) |
+------------------------------------------------------------------+
| 2007-03-28 22:08:25.800000                                       |
+------------------------------------------------------------------+
1 row in set (0.00 sec)

mysql> select DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 HOUR_MICROSECOND);
+----------------------------------------------------------------+
| DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 HOUR_MICROSECOND) |
+----------------------------------------------------------------+
| 2007-03-28 22:08:25.800000                                     |
+----------------------------------------------------------------+
1 row in set (0.00 sec)

mysql> select DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 DAY_MICROSECOND);
+---------------------------------------------------------------+
| DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 DAY_MICROSECOND) |
+---------------------------------------------------------------+
| 2007-03-28 22:08:25.800000                                    |
+---------------------------------------------------------------+
1 row in set (0.00 sec)

What is changed and how it works?

When use getIntervalFromDecimal to get interval, It will convert the Decimal to Interval(the type is string).
Before the pr, the func don't consider that the decimal may be a negative number. It get a wrong result.
e.g.
1 DAY_MICROSECOND => 0 00:00:1
-2.2 DAY_MICROSECOND => 0 00:00:-2.2 (wrong)

This pr, before the converting, I judge whether the decimal is negative first.
-2.2 DAY_MICROSECOND => -0 00:00:2.2

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Code changes

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

@wshwsh12
Copy link
Copy Markdown
Contributor Author

wshwsh12 commented Jul 17, 2019

/run-all-tests

@zz-jason zz-jason requested a review from SunRunAway July 17, 2019 12:16
zz-jason
zz-jason reviewed Jul 17, 2019
View reviewed changes
Comment thread expression/builtin_time.go Outdated
@qw4990 qw4990 changed the title *: fix getIntervalFromDecimal in DATE_ADD() expression: fix getIntervalFromDecimal in DATE_ADD() Jul 18, 2019
qw4990
qw4990 reviewed Jul 18, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

SunRunAway
SunRunAway previously requested changes Jul 18, 2019
View reviewed changes
Comment thread expression/builtin_time.go Outdated
Copy link
Copy Markdown
Contributor

@SunRunAway SunRunAway Jul 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It will be an incorrect result in case "SECOND" and default parts in the switch statement above.

@wshwsh12 wshwsh12 force-pushed the fix_date_microsecond branch from 4919bda to 5cb495c Compare July 18, 2019 06:57
@wshwsh12 wshwsh12 requested a review from SunRunAway July 18, 2019 06:59
@wshwsh12
Copy link
Copy Markdown
Contributor Author

wshwsh12 commented Jul 18, 2019

@SunRunAway PTAL

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 18, 2019 •
edited
Loading

Codecov Report

Merging #11297 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11297   +/-   ##
===========================================
  Coverage   81.6571%   81.6571%           
===========================================
  Files           423        423           
  Lines         91818      91818           
===========================================
  Hits          74976      74976           
  Misses        11531      11531           
  Partials       5311       5311

SunRunAway
SunRunAway reviewed Jul 18, 2019
View reviewed changes
Comment thread expression/builtin_time.go Outdated
Copy link
Copy Markdown
Contributor

@SunRunAway SunRunAway Jul 18, 2019

Choose a reason for hiding this comment

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

It would be better not to do the extra revise code in these paths.
Also, would you like to add a unit test to cover it?

@wshwsh12
Copy link
Copy Markdown
Contributor Author

wshwsh12 commented Jul 18, 2019

/run-all-tests

@wshwsh12
Copy link
Copy Markdown
Contributor Author

wshwsh12 commented Jul 18, 2019

@SunRunAway PTAL

@wshwsh12 wshwsh12 requested a review from SunRunAway July 18, 2019 11:01
@wshwsh12 wshwsh12 force-pushed the fix_date_microsecond branch from cbf8901 to ff0eb51 Compare July 18, 2019 11:13
@SunRunAway SunRunAway mentioned this pull request Jul 18, 2019
Merged
SunRunAway
SunRunAway reviewed Jul 18, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@SunRunAway SunRunAway requested review from qw4990 and zz-jason July 18, 2019 13:27
zz-jason
zz-jason previously approved these changes Jul 18, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Copy Markdown
Member

zz-jason commented Jul 18, 2019

/run-all-tests

@zz-jason zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 18, 2019
@wshwsh12 wshwsh12 force-pushed the fix_date_microsecond branch from e5a3e2b to 34496d0 Compare July 18, 2019 14:44
@wshwsh12
Copy link
Copy Markdown
Contributor Author

wshwsh12 commented Jul 18, 2019

/run-all-tests

@SunRunAway SunRunAway merged commit 5aef053 into pingcap:master Jul 18, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Jul 18, 2019

cherry pick to release-2.1 failed

@sre-bot sre-bot mentioned this pull request Jul 18, 2019
Merged
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Jul 18, 2019

cherry pick to release-3.0 in PR #11325

wshwsh12 added a commit to wshwsh12/tidb that referenced this pull request Jul 18, 2019
@wshwsh12
expression: fix getIntervalFromDecimal in DATE_ADD() (pingcap#11297)
e0b40e6
@wshwsh12 wshwsh12 mentioned this pull request Jul 18, 2019
Merged
zz-jason pushed a commit that referenced this pull request Jul 18, 2019
@sre-bot @zz-jason
expression: fix getIntervalFromDecimal in DATE_ADD() (#11297) (#11325)
13aa832
zz-jason pushed a commit that referenced this pull request Jul 18, 2019
@wshwsh12 @zz-jason
expression: fix getIntervalFromDecimal in DATE_ADD() (#11297) (#11326)
2fe5636
@wshwsh12 wshwsh12 deleted the fix_date_microsecond branch July 25, 2019 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zz-jason zz-jason zz-jason approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

+1 more reviewer

@SunRunAway SunRunAway SunRunAway approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component/expression status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wshwsh12 @zz-jason @sre-bot @SunRunAway @qw4990