fix(expr): exp follows PostgreSQL overflow/underflow behavior
#8309
Conversation
…ommit changes the tempdir to use my home directory. This is temporary to get tests working. Will make a proper fix after completing unit tests.
…rrectly creating temporary directories for testing should be consistent
…it tests would work on my computer
…r variants in ExprError
|
I did not see any instructions on running fuzz tests in Dev Guide, so I don't know how to verify if the Fuzz tests work locally. |
It seems the failure of fuzz tests has nothing to do with this PR. I will try Generally LGTM, thanks for your contribution! |
Have taken a look at it, failure is due to a separate issue. Have submitted a fix and added some docs too: #8312. Thanks for your contribution and for flagging this :) |
No problem. Since this PR has changes that would be user facing, what should I do about the last check box? Leaving it unchecked shows the PR as having 5/6 tasks done and I'm not sure if that's acceptable or if I should delete the last check box and just have the release notes I wrote. |
You can leave it unchecked, release notes look good. Thanks! |
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate::hummock::file_cache::test_utils::tempdir; |
There was a problem hiding this comment.
Seems these storage changes are included by accident?
There was a problem hiding this comment.
I noticed that the unit tests in cache.rs were using a tempdir function that would use /risingwave for temp directories if the env var RISINGWAVE_CI was set. Since, this is using a temp directory to store the same cache data files as the unit tests in cache.rs, I thought it would make sense to update this code to use the same tempdir function as cache.rs.
I can undo this change if desired.
| fn legal_input() { | ||
| let res = exp_f64(0.0.into()).unwrap(); | ||
| assert_eq!(res, OrderedF64::from(1.0)); | ||
| } |
There was a problem hiding this comment.
Also test for NaN, -Inf and Inf?
There was a problem hiding this comment.
Thanks. Added missing unit tests.
exp PG compatibility): if a very large or very small operand is used, then exp errorsexp follows PostgreSQL overflow/underflow behavior
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This addresses Issue #8025 .
With this change, the
expoperator will now return an Underflow error if the the operand is small enough that the result is0and it will return an Overflow error if the operand is large enough that the result isInf. This change is meant to create consistency between architectures which return0orInfwhen the operand is too small/big and architectures which would fault. It also mirrors the Postgres logic, improving the consistency between RisingWave and Postgres.The change adds a sequence of
ifchecks to determine ifexpreturning0orInfis valid or invalid. It's valid forexpto return 0 only when the operand is-Infand it's valid forexpto returnInfonly when the operand isInf. For any other operand, a result of0orInfindicates underflow or overflow, respectively. The logic in this PR checks the value of the operand to see if it is-InforInfand, if it's not, then it checks if the result is0orInfand returns an error if it is.This PR also adds two new error variants to
ExprError:NumericUnderflowandNumericOverflowwhich increases the precision of the error message returned to the user and better reflects the errors returned by Postgres. Theexpoperation uses these errors rather than the previousNumericOutOfRangeerror. I checked all uses of theNumericOutOfRangeerror to see if any can be changed to use eitherNumericUnderfloworNumericOverflow, but most of the situations they are used to not provide enough information to determine if an error was an underflow or overflow, AFAICT. One benchmark test, which checks specifically for overflow errors, was updated to use the newNumericOverflow.Checklist For Contributors
./risedev check(or alias,./risedev c)Checklist For Reviewers
Documentation
Click here for Documentation
Types of user-facing changes
Please keep the types that apply to your changes, and remove the others.
Release note
expoperator will now return an Overflow error if the operand is too large (e.g.exp(1000)) and it will return an Underflow error if the operand is too small (e.g.,exp(-1000)). These changes are intended for consistency between architectures that will return errors when the operand is too large or too small, and architectures which returnInfand0.