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 incorrect date arithmitical with negative interval #8523

Merged
merged 4 commits into from Dec 4, 2018

Conversation

@lerencao
Copy link
Contributor

lerencao commented Nov 29, 2018

What problem does this PR solve?

#8497

What is changed and how it works?

Round the float interval, so that the date arithmetical can behave like mysql:

1.2  => 1
1.5  => 2
-1.2 => -1
-1.5 => -2

Check List

Tests

  • Unit test

Related changes

This bug affects almost all functions related to date arithmetical functions. May be it should

  • Need to cherry-pick to the release branch
  • Need to be included in the release note ?

This change is Reviewable

@sre-bot

This comment has been minimized.

Copy link

sre-bot commented Nov 29, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

{date[0], fcSub, -0.5, date[1]},
{date[0], fcSub, -1.4, date[1]},
}
for _, test := range tests {

This comment has been minimized.

Copy link
@ngaut

ngaut Nov 30, 2018

Member

s/test/t/

This comment has been minimized.

Copy link
@lerencao

lerencao Nov 30, 2018

Author Contributor

Using test is consistent with other usages in this file.

@@ -1527,7 +1527,7 @@ func extractSingleTimeValue(unit string, format string) (int64, int64, int64, fl
if err != nil {
return 0, 0, 0, 0, ErrIncorrectDatetimeValue.GenWithStackByArgs(format)
}
iv := int64(fv + 0.5)
iv := int64(math.Round(fv))

This comment has been minimized.

Copy link
@eurekaka

eurekaka Nov 30, 2018

Contributor

Should we use floatStrToIntStr instead of Round? floatStrToIntStr does round operation also and it covers corner cases.

This comment has been minimized.

Copy link
@lerencao

lerencao Nov 30, 2018

Author Contributor

The float value is also needed.
iv is used in cases that float value should be rounded, like DAY, WEEK.

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Nov 30, 2018

Contributor

You mean inputs like 1.23a4 DAY ? 1.23a can pass strconv.ParseFloat @eurekaka
or something like 1e9223372036854775807 ?

I think the change here is ok, a sane person will not use those illegal input, so
strconv.ParseFloat + math.Round is suitable for most of the cases

This comment has been minimized.

Copy link
@zz-jason

zz-jason Dec 3, 2018

Member
MySQL(root@127.0.0.1:test) > select adddate("2018-12-09", interval "1e1" DAY);
+-------------------------------------------+
| adddate("2018-12-09", interval "1e1" DAY) |
+-------------------------------------------+
| 2018-12-10                                |
+-------------------------------------------+
1 row in set, 1 warning (0.00 sec)

MySQL(root@127.0.0.1:test) > select adddate("2018-12-09", interval "10" DAY);
+------------------------------------------+
| adddate("2018-12-09", interval "10" DAY) |
+------------------------------------------+
| 2018-12-19                               |
+------------------------------------------+
1 row in set (0.00 sec)

MySQL(root@127.0.0.1:test) > select adddate("2018-12-09", interval "9223372036854775807" DAY);
+-----------------------------------------------------------+
| adddate("2018-12-09", interval "9223372036854775807" DAY) |
+-----------------------------------------------------------+
| NULL                                                      |
+-----------------------------------------------------------+
1 row in set, 1 warning (0.00 sec)

MySQL(root@127.0.0.1:test) > show warnings;
+---------+------+--------------------------------------------+
| Level   | Code | Message                                    |
+---------+------+--------------------------------------------+
| Warning | 1441 | Datetime function: datetime field overflow |
+---------+------+--------------------------------------------+
1 row in set (0.01 sec)

We should use strconv.ParseFloat() in this PR. floatStrToIntStr converts 1e1 to 10, which is not in MySQL. Another discovery is that TiDB does not properly handle the "Datetime function: datetime field overflow" warning:

TiDB(root@127.0.0.1:test) > select adddate("2018-12-09", interval "9223372036854775807" DAY);
+-----------------------------------------------------------+
| adddate("2018-12-09", interval "9223372036854775807" DAY) |
+-----------------------------------------------------------+
| 2018-12-09                                                |
+-----------------------------------------------------------+
1 row in set (0.00 sec)

This comment has been minimized.

Copy link
@zz-jason

zz-jason Dec 3, 2018

Member

You can do it in the next PR, this PR LGTM

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

tiancaiamao commented Nov 30, 2018

LGTM

@lerencao

This comment has been minimized.

Copy link
Contributor Author

lerencao commented Dec 3, 2018

Ping

@eurekaka

This comment has been minimized.

Copy link
Contributor

eurekaka commented Dec 3, 2018

@zz-jason PTAL

@zz-jason
Copy link
Member

zz-jason left a comment

LGTM

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Dec 3, 2018

/run-all-tests

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Dec 3, 2018

@zz-jason zz-jason merged commit 6e27de2 into pingcap:master Dec 4, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Dec 4, 2018

@lerencao Please cherry-pick this commit to release-2.1 and release-2.0 branch, thanks.

@lerencao

This comment has been minimized.

Copy link
Contributor Author

lerencao commented Dec 4, 2018

@zz-jason

  • cherry-pick to release-2.1 is done. see #8574
  • This issue is introduced in a commit after v2.0.0 was released, and doesn't exist in release-2.0, so no need to do cherry pick.
@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Dec 4, 2018

Got it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.