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 real number #5295

Closed
wants to merge 10 commits into from

Conversation

mccxj
Copy link
Contributor

@mccxj mccxj commented Dec 4, 2017

for #4477


This change is Reviewable

@sre-bot
Copy link
Contributor

sre-bot commented Dec 4, 2017

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@hanfei1991 hanfei1991 added the contribution This PR is from a community contributor. label Dec 4, 2017
@shenli
Copy link
Member

shenli commented Dec 4, 2017

/ok-to-test

@shenli
Copy link
Member

shenli commented Dec 4, 2017

/run-all-tests

@shenli
Copy link
Member

shenli commented Dec 5, 2017

/run-all-tests

@@ -594,6 +603,7 @@ func (c *arithmeticIntDivideFunctionClass) getFunction(ctx context.Context, args
}

type builtinArithmeticIntDivideIntSig struct{ baseBuiltinFunc }
type builtinArithmeticRealDivideRealSig struct{ baseBuiltinFunc }
Copy link
Member

@coocood coocood Dec 5, 2017

Choose a reason for hiding this comment

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

I think the IntDivide represents the DIV operator, so the name should be IntDivideRealSig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. PTAL.

return 0, isNull, errors.Trace(err)
}

if b == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

You may need to handle the case as follows:

mysql> select (1 div 1E-82);
+---------------+
| (1 div 1E-82) |
+---------------+
|          NULL |
+---------------+
1 row in set, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+---------------+
| Level   | Code | Message       |
+---------+------+---------------+
| Warning | 1365 | Division by 0 |
+---------+------+---------------+
1 row in set (0.00 sec)

Or you can add a TODO here.

@@ -594,6 +603,7 @@ func (c *arithmeticIntDivideFunctionClass) getFunction(ctx context.Context, args
}

type builtinArithmeticIntDivideIntSig struct{ baseBuiltinFunc }
type builtinArithmeticRealDivideRealSig struct{ baseBuiltinFunc }
Copy link
Contributor

Choose a reason for hiding this comment

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

s/builtinArithmeticRealDivideRealSig/builtinArithmeticIntDivideRealSig

@coocood
Copy link
Member

coocood commented Dec 5, 2017

LGTM

sc := s.ctx.GetSessionVars().StmtCtx
b, isNull, err := s.args[1].EvalReal(row, sc)
if isNull || err != nil {
return 0, isNull, errors.Trace(err)
Copy link
Member

Choose a reason for hiding this comment

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

return 0, true, errors.Trace(err) is more specific


a, isNull, err := s.args[0].EvalReal(row, sc)
if isNull || err != nil {
return 0, isNull, errors.Trace(err)
Copy link
Member

Choose a reason for hiding this comment

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

ditto

func DivFloat64(a float64, b float64) (int64, error) {
c := a / b
if c > math.MaxInt64 || c < math.MinInt64 {
return 0, ErrOverflow.GenByArgs("BIGINT", fmt.Sprintf("(%f, %f)", a, b))
Copy link
Member

Choose a reason for hiding this comment

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

add a test like "select (1 div 1E-20);" and check the error message.

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 6, 2017
@mccxj
Copy link
Contributor Author

mccxj commented Dec 7, 2017

maybe I will revert real to the origin decimal case, and test the overflow case of EvalDecimal. WIP.

@zimulala
Copy link
Contributor

zimulala commented Dec 9, 2017

@mccxj
I add the ‘WIP’ tag to your PR. If your PR is ready to be reviewed, let us know.

@zimulala zimulala added the WIP label Dec 9, 2017
@zz-jason zz-jason removed status/LGT1 Indicates that a PR has LGTM 1. all-tests-passed labels Mar 18, 2018
@zz-jason
Copy link
Member

@mccxj any update ?

@pingcap pingcap deleted a comment from CLAassistant Aug 20, 2018
@mccxj
Copy link
Contributor Author

mccxj commented Aug 22, 2018

@zz-jason I will open another PR to continue working.

@mccxj mccxj closed this Aug 22, 2018
@mccxj mccxj deleted the div_func branch August 22, 2018 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants