Skip to content

Commit 72c7bc7

Browse files
liamzwbaoStanding-Man
liamzwbao
authored and
Standing-Man
committed
Fix next_up and next_down behavior for zero float values (apache#16745)
* Fix invalid intervals in `satisfy_greater` * Comparison between `None` and `Some` * Apply changes in `next_up` and `next_down` * Change impl * Revert format change
1 parent 82518ff commit 72c7bc7

File tree

2 files changed

+118
-2
lines changed

2 files changed

+118
-2
lines changed

‎datafusion/common/src/rounding.rs‎

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ pub trait FloatBits {
7777

7878
/// The integer value 0, used in bitwise operations.
7979
const ZERO: Self::Item;
80+
const NEG_ZERO: Self::Item;
8081

8182
/// Converts the floating-point value to its bitwise representation.
8283
fn to_bits(self) -> Self::Item;
@@ -101,6 +102,7 @@ impl FloatBits for f32 {
101102
const CLEAR_SIGN_MASK: u32 = 0x7fff_ffff;
102103
const ONE: Self::Item = 1;
103104
const ZERO: Self::Item = 0;
105+
const NEG_ZERO: Self::Item = 0x8000_0000;
104106

105107
fn to_bits(self) -> Self::Item {
106108
self.to_bits()
@@ -130,6 +132,7 @@ impl FloatBits for f64 {
130132
const CLEAR_SIGN_MASK: u64 = 0x7fff_ffff_ffff_ffff;
131133
const ONE: Self::Item = 1;
132134
const ZERO: Self::Item = 0;
135+
const NEG_ZERO: Self::Item = 0x8000_0000_0000_0000;
133136

134137
fn to_bits(self) -> Self::Item {
135138
self.to_bits()
@@ -175,8 +178,10 @@ pub fn next_up<F: FloatBits + Copy>(float: F) -> F {
175178
}
176179

177180
let abs = bits & F::CLEAR_SIGN_MASK;
178-
let next_bits = if abs == F::ZERO {
181+
let next_bits = if bits == F::ZERO {
179182
F::TINY_BITS
183+
} else if abs == F::ZERO {
184+
F::ZERO
180185
} else if bits == abs {
181186
bits + F::ONE
182187
} else {
@@ -206,8 +211,11 @@ pub fn next_down<F: FloatBits + Copy>(float: F) -> F {
206211
if float.float_is_nan() || bits == F::neg_infinity().to_bits() {
207212
return float;
208213
}
214+
209215
let abs = bits & F::CLEAR_SIGN_MASK;
210-
let next_bits = if abs == F::ZERO {
216+
let next_bits = if bits == F::ZERO {
217+
F::NEG_ZERO
218+
} else if abs == F::ZERO {
211219
F::NEG_TINY_BITS
212220
} else if bits == abs {
213221
bits - F::ONE
@@ -396,4 +404,32 @@ mod tests {
396404
let result = next_down(value);
397405
assert!(result.is_nan());
398406
}
407+
408+
#[test]
409+
fn test_next_up_neg_zero_f32() {
410+
let value: f32 = -0.0;
411+
let result = next_up(value);
412+
assert_eq!(result, 0.0);
413+
}
414+
415+
#[test]
416+
fn test_next_down_zero_f32() {
417+
let value: f32 = 0.0;
418+
let result = next_down(value);
419+
assert_eq!(result, -0.0);
420+
}
421+
422+
#[test]
423+
fn test_next_up_neg_zero_f64() {
424+
let value: f64 = -0.0;
425+
let result = next_up(value);
426+
assert_eq!(result, 0.0);
427+
}
428+
429+
#[test]
430+
fn test_next_down_zero_f64() {
431+
let value: f64 = 0.0;
432+
let result = next_down(value);
433+
assert_eq!(result, -0.0);
434+
}
399435
}

‎datafusion/expr-common/src/interval_arithmetic.rs‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3698,6 +3698,76 @@ mod tests {
36983698
Interval::make(Some(-500.0_f64), Some(1000.0_f64))?,
36993699
Interval::make(Some(-500.0_f64), Some(500.0_f64))?,
37003700
),
3701+
(
3702+
Interval::make(Some(0_i64), Some(0_i64))?,
3703+
Interval::make(Some(-0_i64), Some(0_i64))?,
3704+
true,
3705+
Interval::make(Some(0_i64), Some(0_i64))?,
3706+
Interval::make(Some(-0_i64), Some(0_i64))?,
3707+
),
3708+
(
3709+
Interval::make(Some(-0_i64), Some(0_i64))?,
3710+
Interval::make(Some(-0_i64), Some(-0_i64))?,
3711+
true,
3712+
Interval::make(Some(-0_i64), Some(0_i64))?,
3713+
Interval::make(Some(-0_i64), Some(-0_i64))?,
3714+
),
3715+
(
3716+
Interval::make(Some(0.0_f64), Some(0.0_f64))?,
3717+
Interval::make(Some(-0.0_f64), Some(0.0_f64))?,
3718+
true,
3719+
Interval::make(Some(0.0_f64), Some(0.0_f64))?,
3720+
Interval::make(Some(-0.0_f64), Some(0.0_f64))?,
3721+
),
3722+
(
3723+
Interval::make(Some(0.0_f64), Some(0.0_f64))?,
3724+
Interval::make(Some(-0.0_f64), Some(0.0_f64))?,
3725+
false,
3726+
Interval::make(Some(0.0_f64), Some(0.0_f64))?,
3727+
Interval::make(Some(-0.0_f64), Some(-0.0_f64))?,
3728+
),
3729+
(
3730+
Interval::make(Some(-0.0_f64), Some(0.0_f64))?,
3731+
Interval::make(Some(-0.0_f64), Some(-0.0_f64))?,
3732+
true,
3733+
Interval::make(Some(-0.0_f64), Some(0.0_f64))?,
3734+
Interval::make(Some(-0.0_f64), Some(-0.0_f64))?,
3735+
),
3736+
(
3737+
Interval::make(Some(-0.0_f64), Some(0.0_f64))?,
3738+
Interval::make(Some(-0.0_f64), Some(-0.0_f64))?,
3739+
false,
3740+
Interval::make(Some(0.0_f64), Some(0.0_f64))?,
3741+
Interval::make(Some(-0.0_f64), Some(-0.0_f64))?,
3742+
),
3743+
(
3744+
Interval::make(Some(0_i64), None)?,
3745+
Interval::make(Some(-0_i64), None)?,
3746+
true,
3747+
Interval::make(Some(0_i64), None)?,
3748+
Interval::make(Some(-0_i64), None)?,
3749+
),
3750+
(
3751+
Interval::make(Some(0_i64), None)?,
3752+
Interval::make(Some(-0_i64), None)?,
3753+
false,
3754+
Interval::make(Some(1_i64), None)?,
3755+
Interval::make(Some(-0_i64), None)?,
3756+
),
3757+
(
3758+
Interval::make(Some(0.0_f64), None)?,
3759+
Interval::make(Some(-0.0_f64), None)?,
3760+
true,
3761+
Interval::make(Some(0.0_f64), None)?,
3762+
Interval::make(Some(-0.0_f64), None)?,
3763+
),
3764+
(
3765+
Interval::make(Some(0.0_f64), None)?,
3766+
Interval::make(Some(-0.0_f64), None)?,
3767+
false,
3768+
Interval::make(Some(0.0_f64), None)?,
3769+
Interval::make(Some(-0.0_f64), None)?,
3770+
),
37013771
];
37023772
for (first, second, includes_endpoints, left_modified, right_modified) in cases {
37033773
assert_eq!(
@@ -3717,6 +3787,16 @@ mod tests {
37173787
Interval::make(Some(1500.0_f32), Some(2000.0_f32))?,
37183788
false,
37193789
),
3790+
(
3791+
Interval::make(Some(0_i64), Some(0_i64))?,
3792+
Interval::make(Some(-0_i64), Some(0_i64))?,
3793+
false,
3794+
),
3795+
(
3796+
Interval::make(Some(-0_i64), Some(0_i64))?,
3797+
Interval::make(Some(-0_i64), Some(-0_i64))?,
3798+
false,
3799+
),
37203800
];
37213801
for (first, second, includes_endpoints) in infeasible_cases {
37223802
assert_eq!(satisfy_greater(&first, &second, !includes_endpoints)?, None);

0 commit comments

Comments
 (0)