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

improved the error message when encounter overflow. This is just a te… #3017

Merged
merged 1 commit into from
Apr 12, 2017
Merged

improved the error message when encounter overflow. This is just a te… #3017

merged 1 commit into from
Apr 12, 2017

Conversation

zhexuany
Copy link
Contributor

@zhexuany zhexuany commented Apr 8, 2017 •
edited
Loading

…mp commit. Rebase is needed

This PR aims to improve error handling in tidb. Once PR merged into master, tidb will have same behavior as mysql does.

@zhexuany
Copy link
Contributor Author

zhexuany commented Apr 8, 2017 •
edited
Loading

This pr addresses this issue. #2862

@zhexuany
Copy link
Contributor Author

zhexuany commented Apr 8, 2017 •
edited
Loading

The error message are generated in overflow.go which reside in util/type. These error message requires runtime information i.e. select 10000000000 + 10000000000. These two uint have to be shown in error message. There is no way that we can predefine a error message and use it at runtime. Hence, I propose creating error message at runtime and format the error message according to the runtime data.

If you guys agree this approach, I can continue to work on this. Otherwise, we can discuss and bring a new and better way to do this.

@coocood
Copy link
Member

coocood commented Apr 8, 2017

The error code is much more important than the error message.
Most of the applications only check error code.
Formatted error message is hard to compare.

@coocood
Copy link
Member

coocood commented Apr 8, 2017

@zhexuany
You can take a look at the package terror and how it is used.

@zhexuany
Copy link
Contributor Author

zhexuany commented Apr 8, 2017 •
edited
Loading

@coocood gotcha. I think I already know a better way to address this issue.

shenli
shenli reviewed Apr 9, 2017
View reviewed changes
util/types/overflow.go Outdated
)

const (
CodeUintAdditionOperationOverflow terror.ErrCode = 1
Copy link
Member

Choose a reason for hiding this comment

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

It is better to move this definition into util/types/errors.go. And if it is a mysql standard error, please also add the error code mapping in init(). Protocol layer will use this mapping to convert it to corresponding mysql error code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shenli this pr is not ready yet. I am still doing more work. It is my bad not saying this in this PR. As you can see, the todo list is still marked as todo.

@zhexuany
Copy link
Contributor Author

Will push all commits to this PR tonight for reviewing.

@shenli
Copy link
Member

shenli commented Apr 10, 2017

@zhexuany OK

@zimulala zimulala added the contribution This PR is from a community contributor. label Apr 10, 2017
@zhexuany zhexuany force-pushed the improve_error_msg_handleing_on_overflow branch 4 times, most recently from 37e302b to adda203 Compare April 10, 2017 09:16
@zhexuany
Copy link
Contributor Author

@coocood

select 10000000000000000000 + 10000000000000000000;

generates the following error message.

ERROR 1264 (22003): BIGINT UNSIGNED value is out of range in '(10000000000000000000, 10000000000000000000)'

Is this error message ok?

@coocood
Copy link
Member

coocood commented Apr 10, 2017

@zhexuany
MySQL returns error code 1690

@zhexuany
Copy link
Contributor Author

@coocood

 return 0, errors.Trace(mysql.NewErr(mysql.ErrDataOutOfRange, "BIGINT UNSIGNED", fmt.Sprintf("(%d, %d)", a, b)))

This snippet gives

ERROR 1105 (HY000): ERROR 1690 (22003): BIGINT UNSIGNED value is out of range in '(10000000000000000000, 10000000000000000000)'

The error code is correct but in a wired structure. Is this correct?

@coocood
Copy link
Member

coocood commented Apr 10, 2017

@zhexuany
typesMySQLErrCodes need to be updated.

@zhexuany
Copy link
Contributor Author

zhexuany commented Apr 10, 2017 •
edited
Loading

@coocood I already did that. Actually, my question is the error info's structure.

ERROR 1105 (HY000): ERROR 1690 (22003):

The first one indicates the error happens in types. the second one indicates the error happens in mysql overflow.

Is this understanding correct?

Update: It is just terror can not recognize MysqlError.

@zhexuany zhexuany force-pushed the improve_error_msg_handleing_on_overflow branch 4 times, most recently from 64414c2 to 3246ed5 Compare April 11, 2017 07:03
@zhexuany
Copy link
Contributor Author

@coocood @shenli PTAL

@zhexuany zhexuany force-pushed the improve_error_msg_handleing_on_overflow branch from 3246ed5 to fa17467 Compare April 11, 2017 09:45
@coocood
Copy link
Member

coocood commented Apr 11, 2017

LGTM
Thank you for your contribution!

@coocood coocood added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 11, 2017
zimulala
zimulala reviewed Apr 11, 2017
View reviewed changes
expression/builtin_math.go Outdated
@@ -24,6 +24,7 @@ import (
"strconv"
"strings"

"fmt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this line to line-26.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

util/types/overflow.go Outdated
@@ -16,16 +16,14 @@ package types
import (
"math"

"fmt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this line to line-18.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

zimulala
zimulala reviewed Apr 11, 2017
View reviewed changes
expression/builtin_math.go
d.SetFloat64(math.Pow(x, y))

power := math.Pow(x, y)
if math.IsInf(power, -1) || math.IsInf(power, 1) || math.IsNaN(power) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some tests for this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

@zhexuany zhexuany force-pushed the improve_error_msg_handleing_on_overflow branch from fa17467 to a090973 Compare April 12, 2017 03:10
@zhexuany zhexuany force-pushed the improve_error_msg_handleing_on_overflow branch from a090973 to 0e0cd9d Compare April 12, 2017 03:23
@zimulala
Copy link
Contributor

LGTM

@zimulala zimulala merged commit f5270ad into pingcap:master Apr 12, 2017
@zhexuany zhexuany deleted the improve_error_msg_handleing_on_overflow branch April 12, 2017 03:38
@coocood coocood mentioned this pull request Apr 18, 2017
Closed
@zhexuany zhexuany restored the improve_error_msg_handleing_on_overflow branch April 23, 2017 13:39
@zhexuany zhexuany deleted the improve_error_msg_handleing_on_overflow branch April 23, 2017 13:44
@zhexuany zhexuany restored the improve_error_msg_handleing_on_overflow branch April 24, 2017 03:18
@zhexuany zhexuany deleted the improve_error_msg_handleing_on_overflow branch April 24, 2017 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shenli shenli shenli left review comments

@zimulala zimulala zimulala approved these changes

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zhexuany @coocood @shenli @zimulala