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.

Sign up for GitHub

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

Jump to bottom

expression: handle builtin add_date/sub_date func overflow #11472

Merged
merged 2 commits into from
Jul 27, 2019
Merged

expression: handle builtin add_date/sub_date func overflow #11472

merged 2 commits into from
Jul 27, 2019

Conversation

AndrewDi
Copy link
Contributor

@AndrewDi AndrewDi commented Jul 26, 2019 •
edited
Loading

What problem does this PR solve?

Part of issue #11223, fix issue #11256

What is changed and how it works?

if Year less then 0 or greater then max uint16
then return null value

if goTime.Year() < 0 || goTime.Year() > (1<<16-1) {
	return types.Time{}, true, nil
}

Check List

Tests

  • Unit test

bb7133
bb7133 reviewed Jul 26, 2019
View reviewed changes
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133
Copy link
Member

bb7133 commented Jul 26, 2019

hi @AndrewDi , thanks for contributing!

@wshwsh12 wshwsh12 added component/expression type/bugfix This PR fixes a bug. contribution This PR is from a community contributor. labels Jul 26, 2019
wshwsh12
wshwsh12 reviewed Jul 26, 2019
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

I think you should add the same code in function sub to keep the behaviour of ADD_DATE and SUB_DATE same. @AndrewDi .

expression/builtin_time.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 27, 2019

Codecov Report

Merging #11472 into master will increase coverage by 0.0217%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #11472        +/-   ##
================================================
+ Coverage   81.3426%   81.3643%   +0.0217%     
================================================
  Files           424        424                
  Lines         90913      90917         +4     
================================================
+ Hits          73951      73974        +23     
+ Misses        11639      11624        -15     
+ Partials       5323       5319         -4

@codecov
Copy link

codecov bot commented Jul 27, 2019 •
edited
Loading

Codecov Report

Merging #11472 into master will increase coverage by 0.0195%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #11472        +/-   ##
================================================
+ Coverage   81.3426%   81.3621%   +0.0195%     
================================================
  Files           424        424                
  Lines         90913      90917         +4     
================================================
+ Hits          73951      73972        +21     
+ Misses        11639      11628        -11     
+ Partials       5323       5317         -6

@wshwsh12
Copy link
Contributor

Please include some test cases that cover the function sub showing now the function is right.
@AndrewDi
Rest LGTM, and thanks for contributing!

@wshwsh12 wshwsh12 changed the title expression: handle builtin add_date func overflow expression: handle builtin add_date/sub_date func overflow Jul 27, 2019
wshwsh12
wshwsh12 reviewed Jul 27, 2019
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

@AndrewDi PTAL

expression/integration_test.go Outdated Show resolved Hide resolved
@wshwsh12
Copy link
Contributor

/run-all-tests

wshwsh12
wshwsh12 approved these changes Jul 27, 2019
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

LGTM

@wshwsh12 wshwsh12 added status/all tests passed status/LGT2 Indicates that a PR has LGTM 2. labels Jul 27, 2019
@wshwsh12 wshwsh12 merged commit 5c35082 into pingcap:master Jul 27, 2019
@sre-bot sre-bot mentioned this pull request Jul 27, 2019
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Jul 27, 2019

cherry pick to release-3.0 in PR #11476

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

sre-bot commented Jul 27, 2019

cherry pick to release-2.1 in PR #11477

sre-bot added a commit that referenced this pull request Jul 27, 2019
@sre-bot
expression: handle builtin add_date/sub_date func overflow (#11472) (#…
6c3920c 
…11477)

All tests passed, auto merged by Bot
@AndrewDi AndrewDi deleted the fix_issue_11256 branch July 27, 2019 11:54
@SunRunAway SunRunAway mentioned this pull request Jul 29, 2019
Open
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bb7133 bb7133 bb7133 left review comments

@wshwsh12 wshwsh12 wshwsh12 approved these changes

Assignees
No one assigned
Labels
component/expression contribution This PR is from a community contributor. 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.
4 participants
@AndrewDi @bb7133 @wshwsh12 @sre-bot