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 out of range error for intdiv #7492

Merged
merged 5 commits into from
Aug 28, 2018
Merged

Conversation

mccxj
Copy link
Contributor

@mccxj mccxj commented Aug 26, 2018

What problem does this PR solve?

#4477

What is changed and how it works?

see https://github.com/mysql/mysql-server/blob/913071c0b16cc03e703308250d795bc381627e37/sql/item_func.cc#L2232

mysql's behavior:

  • mysql's val_decimal will ignore truncate error and consider the others as waring.
  • similar error check for my_decimal_div.

Check List

Tests

  • Integration test

Code changes

Side effects

Related changes

@mccxj
Copy link
Contributor Author

mccxj commented Aug 26, 2018

@zz-jason PTAL

@jackysp
Copy link
Member

jackysp commented Aug 27, 2018

Thanks @mccxj !
/run-all-tests

@jackysp jackysp added contribution This PR is from a community contributor. component/expression labels Aug 27, 2018
func (s *builtinArithmeticIntDivideDecimalSig) evalInt(row chunk.Row) (ret int64, isNull bool, err error) {
sc := s.ctx.GetSessionVars().StmtCtx
var num [2]*types.MyDecimal
for i, arg := range s.args[0:2] {
Copy link
Member

Choose a reason for hiding this comment

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

Should we return an error if len(args) is not 2?

Copy link
Member

@zz-jason zz-jason Aug 27, 2018

Choose a reason for hiding this comment

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

argument validation should not be done in this phase.

Copy link
Member

Choose a reason for hiding this comment

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

So use s.args is also safe here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I forgot it. use s.args is enough.

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added type/compatibility status/LGT1 Indicates that a PR has LGTM 1. labels Aug 27, 2018
@zz-jason
Copy link
Member

@jackysp PTAL

@jackysp
Copy link
Member

jackysp commented Aug 28, 2018

/run-all-tests

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp merged commit 55565f1 into pingcap:master Aug 28, 2018
@mccxj mccxj deleted the intdiv branch August 28, 2018 03:58
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/LGT1 Indicates that a PR has LGTM 1. type/compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants