-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix divide by zero not throwing proper error for decimal #3517
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
Conversation
f64 also doesn't have the precision to do Decimal128 math. I'll add another issue around fixing the math for Decimal128 so it uses i128 instead of floats. I first attempted to fix that here but I think fixing that might be more complicated than it looks. I think we can handle that separately and just focus this fix on the divide by zero error. Note also that the full precision of decimal literals is not available from SQL because the SQL planner converts all literals to floats. (not a problem in this case obviously, but would be for literals using the full 38 digits.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kmitchener -- the code looks good -- I think the test needs a tweak but otherwise this is looking quite good 👍
let _result = modulus_decimal(&left_decimal_array, &right_decimal_array) | ||
.expect_err("expecting DivideByZero error"); | ||
let _result = modulus_decimal_scalar(&left_decimal_array, 0) | ||
.expect_err("expecting DivideByZero error"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these tests would pass even on master without your code change
Perhaps we could add a check for the actual error ? With code similar to https://github.com/apache/arrow-datafusion/blob/331922036a23265084ab091fa2ab5c57da42c841/datafusion/core/tests/sql/information_schema.rs#L35-L42 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, excellent! Thanks for the review, as always. Have updated with these changes.
Codecov Report
@@ Coverage Diff @@
## master #3517 +/- ##
=======================================
Coverage 85.75% 85.76%
=======================================
Files 299 299
Lines 55311 55334 +23
=======================================
+ Hits 47432 47455 +23
Misses 7879 7879
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks great. Thanks @kmitchener
Benchmark runs are scheduled for baseline = c6ebf7b and contender = b106b02. b106b02 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3498 .
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?
Proper error