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 date_add func in SECOND INTERVAL #11312

Merged
merged 3 commits into from Aug 5, 2019

Conversation

@yangwenmai
Copy link
Contributor

commented Jul 18, 2019

What problem does this PR solve?

Fixes #11319

What is changed and how it works?

The results of this PR like MySQL 5.7 has same result.

SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 MINUTE_MICROSECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 SECOND_MICROSECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 HOUR_MICROSECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 DAY_MICROSECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 SECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 HOUR_SECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 DAY_SECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 MINUTE_SECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 MINUTE);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 DAY_MINUTE);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 HOUR_MINUTE);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 DAY_HOUR);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 YEAR_MONTH);

SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 MINUTE_MICROSECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 SECOND_MICROSECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 HOUR_MICROSECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 DAY_MICROSECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 SECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 HOUR_SECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 DAY_SECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 MINUTE_SECOND);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 MINUTE);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 DAY_MINUTE);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 HOUR_MINUTE);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 DAY_HOUR);
SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL 2.2 YEAR_MONTH);

@yangwenmai yangwenmai changed the title Fix date add neg Fix(expression): fix date_add func in neg INTERVAL Jul 18, 2019

@zz-jason zz-jason requested review from XuHuaiyu and qw4990 Jul 18, 2019

@zz-jason

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

@yangwenmai Thanks for your contribution, it would be nice if you can:

  1. follow the rules described in CONTRIBUTING.md to refine your PR title.
  2. follow the rules described in Closing issues using keywords to refine the PR description so that the issue can be automated closed once this PR is merged.

@yangwenmai yangwenmai force-pushed the yangwenmai:fix_date_add_neg branch from 200fc50 to 5614270 Jul 18, 2019

@SunRunAway

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

Is it duplicate to pr #11297?

@SunRunAway

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

Consider fixing issue #11319? @yangwenmai

@yangwenmai

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

I fixed the #11286 and #11319 in this PR.

@wshwsh12

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

I add some test cases in #11319 . You can have a look.

@yangwenmai yangwenmai changed the title Fix(expression): fix date_add func in neg INTERVAL expression: fix date_add func in neg INTERVAL Jul 18, 2019

@SunRunAway

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

I fixed the #11286 and #11319 in this PR.

@yangwenmai
#11286 is already fixed by another pull request. Will you consider modifying your code and pull request description to fix #11319 only?

@yangwenmai

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

I fixed the #11286 and #11319 in this PR.

@yangwenmai
#11286 is already fixed by another pull request. Will you consider modifying your code and pull request description to fix #11319 only?

OK

@qw4990 qw4990 requested review from wshwsh12 and SunRunAway Jul 22, 2019

@yangwenmai yangwenmai force-pushed the yangwenmai:fix_date_add_neg branch 2 times, most recently from 8dfbe7d to 1ea8ed1 Jul 23, 2019

@qw4990 qw4990 removed their request for review Jul 23, 2019

tk.MustQuery(`SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 SECOND_MICROSECOND)`).Check(testkit.Rows("2007-03-28 22:08:25.800000"))
tk.MustQuery(`SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 HOUR_MICROSECOND)`).Check(testkit.Rows("2007-03-28 22:08:25.800000"))
tk.MustQuery(`SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 DAY_MICROSECOND)`).Check(testkit.Rows("2007-03-28 22:08:25.800000"))
tk.MustQuery(`SELECT DATE_ADD('2007-03-28 22:08:28',INTERVAL -2.2 SECOND)`).Check(testkit.Rows("2007-03-28 22:08:25.800000"))

This comment has been minimized.

Copy link
@SunRunAway

SunRunAway Jul 24, 2019

Member

Please take a look at the code commented out above and it seems that something is duplicate.

if sign == int64(-1) {
return 0, 0, dayCount, -(iv*int64(gotime.Second) + dv*int64(gotime.Microsecond)), err
}
return 0, 0, dayCount, (iv*int64(gotime.Second) + dv*int64(gotime.Microsecond)), err

This comment has been minimized.

Copy link
@SunRunAway

SunRunAway Jul 24, 2019

Member

Please remove the redundant brackets.

This comment has been minimized.

Copy link
@SunRunAway

SunRunAway Jul 24, 2019

Member

If you prefer to just fix the SECOND unit problem, please change your pull request title more precisely and do not use neg for abbreviation.

if err != nil {
return 0, 0, 0, 0, ErrIncorrectDatetimeValue.GenWithStackByArgs(format)
}
}

This comment has been minimized.

Copy link
@SunRunAway

SunRunAway Jul 24, 2019

Member

After another look, I think the problem occurs because of the incorrectness of dv.
Is it more reasonable to add a line dv *= sign below to solve this problem?

@SunRunAway

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@yangwenmai
Hi, please take a look again when you're free.

@@ -1628,6 +1628,12 @@ func parseSingleTimeValue(unit string, format string, strictCheck bool) (int64,
return 0, 0, 0, 0, ErrIncorrectDatetimeValue.GenWithStackByArgs(format)
}
riv := iv // Rounded integer value
if sign == int64(-1) {

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Jul 29, 2019

Contributor

Can we use iv *= -1 here?

@zz-jason

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

@yangwenmai friendly ping, any update?

@sre-bot

This comment has been minimized.

Copy link

commented Aug 3, 2019

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.

@yangwenmai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@SunRunAway
Copy link
Member

left a comment

reset LGTM

@SunRunAway SunRunAway requested a review from XuHuaiyu Aug 5, 2019

@@ -1664,6 +1664,7 @@ func parseSingleTimeValue(unit string, format string, strictCheck bool) (int64,
}
dayCount := iv / int64(GoDurationDay/gotime.Second)
iv %= int64(GoDurationDay / gotime.Second)
dv *= sign

This comment has been minimized.

Copy link
@SunRunAway

SunRunAway Aug 5, 2019

Member

Should we put this line into if decimalPointPos < lf { section?

This comment has been minimized.

Copy link
@yangwenmai

yangwenmai Aug 5, 2019

Author Contributor

OK, It's much better.

@yangwenmai yangwenmai force-pushed the yangwenmai:fix_date_add_neg branch from 66f41ea to 6a2a950 Aug 5, 2019

@SunRunAway
Copy link
Member

left a comment

LGTM && @XuHuaiyu @wshwsh12 PTAL

@wshwsh12
Copy link
Contributor

left a comment

LGTM

@wshwsh12

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

/run-all-tests

1 similar comment
@sre-bot

This comment has been minimized.

Copy link

commented Aug 5, 2019

/run-all-tests

@sre-bot sre-bot merged commit ddafdff into pingcap:master Aug 5, 2019

14 checks passed

ci/circleci Your tests passed on CircleCI!
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@wshwsh12

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

/run-cherry-picker

@sre-bot

This comment has been minimized.

Copy link

commented Aug 5, 2019

cherry pick to release-2.1 failed

@you06

This comment has been minimized.

Copy link

commented Aug 5, 2019

/run-cherry-picker

@sre-bot

This comment has been minimized.

Copy link

commented Aug 5, 2019

cherry pick to release-3.0 in PR #11615

sre-bot added a commit that referenced this pull request Aug 5, 2019

zz-jason added a commit to SunRunAway/tidb that referenced this pull request Aug 5, 2019

SunRunAway added a commit to SunRunAway/tidb that referenced this pull request Aug 6, 2019

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.