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 proto fields and add missing overflow handling for arithmatic functions #12858

Merged
merged 42 commits into from
Nov 6, 2019

Conversation

H-ZeX
Copy link
Contributor

@H-ZeX H-ZeX commented Oct 21, 2019

Signed-off-by: H-ZeX [email protected]

What problem does this PR solve?

expression: Fix some bug of cast that detected by tikv/copr-test#4

What is changed and how it works?

  • There are some cast function that forget to handle the err using ctx, fix them by handle the err with ctx.
  • This sql select 1 / '2007' div 1; will fail with this err msg
    For float(M,D), double(M,D) or decimal(M,D), M must be >= D (column '').
    because in arithmeticDivideFunctionClass.setType4DivReal, it set the decimal to 31 while flen is 23. Then after builtinCastRealAsDecimalSig.evalDecimal is called , it will fail in ProduceDecWithSpecifiedTp
  • change in expression/builtin_arithmetic.go: Without this change, the errMsg is like
    BIGINT UNSIGNED value is out of range in '(Column#1 - Column#2) , has no concrete number.
  • change in server/conn.go: Many error (such as errors.withStack)'s err field is of terror.Error type, but themeself are not of terror.Error type. So origin code will hide the origin err code. Same in store/mockstore/mocktikv/cop_handler_dag.go's toPBError.

Check List

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Write release note for bug-fix or new feature.
    TODO

Signed-off-by: H-ZeX <[email protected]>
@H-ZeX H-ZeX requested a review from a team as a code owner October 21, 2019 16:04
@ghost ghost requested review from wshwsh12 and removed request for a team October 21, 2019 16:04
@zyxbest
Copy link

zyxbest commented Oct 21, 2019

/build

@zyxbest
Copy link

zyxbest commented Oct 21, 2019

/rebuild

@zyxbest
Copy link

zyxbest commented Oct 21, 2019

/run-all-tests

@H-ZeX
Copy link
Contributor Author

H-ZeX commented Oct 22, 2019

/tests

@H-ZeX
Copy link
Contributor Author

H-ZeX commented Oct 22, 2019

/run-all-tests

Signed-off-by: H-ZeX <[email protected]>
@H-ZeX
Copy link
Contributor Author

H-ZeX commented Oct 22, 2019

/run-all-tests

@codecov
Copy link

codecov bot commented Oct 23, 2019

Codecov Report

Merging #12858 into master will decrease coverage by 0.0206%.
The diff coverage is 72.7272%.

@@               Coverage Diff                @@
##             master     #12858        +/-   ##
================================================
- Coverage   80.1454%   80.1247%   -0.0207%     
================================================
  Files           469        469                
  Lines        111239     111239                
================================================
- Hits          89153      89130        -23     
- Misses        15200      15224        +24     
+ Partials       6886       6885         -1

@lonng
Copy link
Contributor

lonng commented Oct 23, 2019

@qw4990 PTAL

Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Deardrops Deardrops left a comment

Choose a reason for hiding this comment

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

LGTM

@lonng
Copy link
Contributor

lonng commented Nov 6, 2019

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 6, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 6, 2019

Your auto merge job has been accepted, waiting for 13198, 13200

@sre-bot
Copy link
Contributor

sre-bot commented Nov 6, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Nov 6, 2019

@H-ZeX merge failed.

@lonng
Copy link
Contributor

lonng commented Nov 6, 2019

/run-unit-test

@lonng lonng merged commit c01006a into pingcap:master Nov 6, 2019
@lonng lonng deleted the fix_cast_bug-2 branch November 6, 2019 09:39
@sre-bot
Copy link
Contributor

sre-bot commented Nov 6, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 6, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 6, 2019

cherry pick to release-3.1 failed

@lonng
Copy link
Contributor

lonng commented Nov 6, 2019

@H-ZeX Please cherry-pick this PR to 2.1/3.0/3.1.

XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-2.1 release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @lonng PTAL.

@reafans
Copy link
Contributor

reafans commented Apr 9, 2020

/run-cherry-picker

@sre-bot
Copy link
Contributor

sre-bot commented Apr 9, 2020

cherry pick to release-2.1 in PR #16245

@sre-bot
Copy link
Contributor

sre-bot commented Apr 9, 2020

cherry pick to release-3.0 in PR #16246

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/can-merge Indicates a PR has been approved by a committer. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants