Fix some simplification rules for floating-point arithmetic operations #7515
Conversation
github-actions
bot
added
optimizer
| Expr::BinaryExpr(BinaryExpr { | ||
| left, | ||
| op: Modulo, | ||
| right, | ||
| }) if !info.nullable(&left)? && is_zero(&right) => { | ||
| return Err(DataFusionError::ArrowError(ArrowError::DivideByZero)); | ||
| match info.get_data_type(&left)? { |
There was a problem hiding this comment.
I don't really understand the rationale for float % float --> NaN (rather than error)
But on the other hand, postgres doesn't seem to support % on floating point values:
select 1.0::float % 0::float;
operator does not exist: double precision % double precision LINE 1: select 1.0::float % 0::float; ^ HINT: No operator matches the given name and argument types. You might need to add explicit type casts.
There was a problem hiding this comment.
@alamb
DataFusion utilizes the rem() function from the arrow-rs to perform the Modulo operation.
The modification here ensures that it behaves the same as the rem() function in arrow-rs, i.e., float % 0. --> NAN.
https://github.com/apache/arrow-rs/blob/77455d48cd6609045a4728ba908123de9d0b62fd/arrow-arith/src/numeric.rs#L71-L77
And in the IEEE 754-2008 standard：
7.2 Invalid operation 7.2.0
For operations producing results in floating-point format, the default result of an operation that signals the
invalid operation exception shall be a quiet NaN...
These operations are:
...
f) remainder: remainder(x, y), when y is zero or x is infinite...
...
Ref: https://en.wikipedia.org/wiki/NaN#Operations_generating_NaN
|
Thanks for the clarification @jonahgao |
Which issue does this PR close?
N/A
Rationale for this change
Similar to #7503.
Because of the presence of
NaN, some simplification rules will no longer be applicable to floating-point types.What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No