Skip to content

expression: handle builtin add_date/sub_date func overflow#11472

Merged
wshwsh12 merged 2 commits intopingcap:masterfrom
AndrewDi:fix_issue_11256
Jul 27, 2019
Merged

expression: handle builtin add_date/sub_date func overflow#11472
wshwsh12 merged 2 commits intopingcap:masterfrom
AndrewDi:fix_issue_11256

Conversation

@AndrewDi
Copy link
Contributor

@AndrewDi AndrewDi commented Jul 26, 2019

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

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
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 .

@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

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
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

@wshwsh12
Copy link
Contributor

/run-all-tests

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

@sre-bot
Copy link
Contributor

sre-bot commented Jul 27, 2019

cherry pick to release-3.0 in PR #11476

@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
@AndrewDi AndrewDi deleted the fix_issue_11256 branch July 27, 2019 11:54
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/bugfix This PR fixes a bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants