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 interval month,year diffs from mysql #8988

Merged
merged 9 commits into from
Feb 11, 2019

Conversation

haplone
Copy link
Contributor

@haplone haplone commented Jan 9, 2019

What problem does this PR solve?

date_add interval month diffs from mysql

when we execute select date_add('2018-01-31',interval 1 month) in mysql we got 2018-02-28

but in tidb we got 2018-03-03

What is changed and how it works?

  1. fix the gap between golang api time.DateDate and mysql date_sub with days in expression/builtin_time.go
  2. add GetLastDay in types/mytime.go to get last day in month
  3. add test for above methods

Check List

Tests

  • Unit test

@haplone
Copy link
Contributor Author

haplone commented Jan 9, 2019

@zz-jason @zyguan PTAL
I've moved PR[8534] here , to merge into master first , as shenli suggest in wechat.
Please help me close 8534, thanks.

@codecov-io
Copy link

codecov-io commented Jan 9, 2019

Codecov Report

Merging #8988 into master will decrease coverage by <.01%.
The diff coverage is 41.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8988      +/-   ##
==========================================
- Coverage   67.26%   67.26%   -0.01%     
==========================================
  Files         371      371              
  Lines       77161    77182      +21     
==========================================
+ Hits        51905    51916      +11     
- Misses      20620    20633      +13     
+ Partials     4636     4633       -3
Impacted Files Coverage Δ
expression/builtin_time.go 65% <100%> (-0.07%) ⬇️
types/mytime.go 85.19% <35.71%> (-5.61%) ⬇️
util/systimemon/systime_mon.go 80% <0%> (-20%) ⬇️
executor/join.go 77.86% <0%> (-0.53%) ⬇️
executor/executor.go 67.03% <0%> (+0.13%) ⬆️
util/filesort/filesort.go 76.48% <0%> (+0.31%) ⬆️
expression/schema.go 94.53% <0%> (+0.78%) ⬆️
store/tikv/lock_resolver.go 42.65% <0%> (+0.94%) ⬆️
ddl/delete_range.go 79.36% <0%> (+4.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcecc91...50768b3. Read the comment docs.

@imtbkcat
Copy link

/run-all-tests

Copy link

@imtbkcat imtbkcat left a comment

Choose a reason for hiding this comment

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

Rest LGTM

goTime = goTime.AddDate(int(year), int(month), int(day))
// when we execute select date_add('2018-01-31',interval 1 month) in mysql we got 2018-02-28
// but in tidb we got 2018-03-03
// dig it and we found it's caused by golang api time.DateDate(year int, month Month, day, hour, min, sec, nsec int, loc *Location) Time ,

Choose a reason for hiding this comment

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

Please uppercase the first letter of each comment sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@imtbkcat PTAL

@imtbkcat imtbkcat requested review from XuHuaiyu and qw4990 January 14, 2019 09:09
Copy link

@imtbkcat imtbkcat left a comment

Choose a reason for hiding this comment

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

LGTM

goTime = goTime.AddDate(int(year), int(month), int(day))
// When we execute select date_add('2018-01-31',interval 1 month) in mysql we got 2018-02-28
// but in tidb we got 2018-03-03.
// Dig it and we found it's caused by golang api time.DateDate(year int, month Month, day, hour, min, sec, nsec int, loc *Location) Time ,
Copy link
Contributor

Choose a reason for hiding this comment

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

s/time.DateDate/time.Date

// Dig it and we found it's caused by golang api time.DateDate(year int, month Month, day, hour, min, sec, nsec int, loc *Location) Time ,
// it says October 32 converts to November 1 ,it conflits with mysql.
// See https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_date-add
df := getFixDays(int(year), int(month), int(day), goTime)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to wrap these to a more easy-to-use function like dateAdd(goTime, year, month, day) goTime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've modified as you suggested.
PTAL @qw4990

@qw4990
Copy link
Contributor

qw4990 commented Jan 31, 2019

LGTM

@qw4990 qw4990 added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 31, 2019
@qw4990 qw4990 merged commit eb2578b into pingcap:master Feb 11, 2019
@qw4990
Copy link
Contributor

qw4990 commented Feb 11, 2019

@haplone Thanks for your contribution~ Can you help us to cherry pick this commit to release-2.1 and release-2.0?

@haplone
Copy link
Contributor Author

haplone commented Feb 12, 2019

@haplone Thanks for your contribution~ Can you help us to cherry pick this commit to release-2.1 and release-2.0?

@qw4990
I've maked PR in 9284 and 9287

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/compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants