Skip to content

Commit 99ca196

Browse files
fix: follow IEEE 754 totalOrder for float and double (#1959)
## Which issue does this PR close? - Closes #1951. ## What changes are included in this PR? For `F32` and `F64`, we use the `total_cmp` method for comparison. ## Are these changes tested? Yes, I added test cases to verify whether the comparison follows the IEEE 754 rules. Signed-off-by: StandingMan <[email protected]> Co-authored-by: Renjie Liu <[email protected]>
1 parent 5dc5764 commit 99ca196

File tree

2 files changed

+33
-28
lines changed

2 files changed

+33
-28
lines changed

crates/iceberg/src/spec/values/datum.rs

Lines changed: 8 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -166,36 +166,16 @@ impl<'de> Deserialize<'de> for Datum {
166166

167167
// Compare following iceberg float ordering rules:
168168
// -NaN < -Infinity < -value < -0 < 0 < value < Infinity < NaN
169-
fn iceberg_float_cmp<T: Float>(a: T, b: T) -> Option<Ordering> {
170-
if a.is_nan() && b.is_nan() {
171-
return match (a.is_sign_negative(), b.is_sign_negative()) {
172-
(true, false) => Some(Ordering::Less),
173-
(false, true) => Some(Ordering::Greater),
174-
_ => Some(Ordering::Equal),
175-
};
176-
}
177-
178-
if a.is_nan() {
179-
return Some(if a.is_sign_negative() {
180-
Ordering::Less
181-
} else {
182-
Ordering::Greater
183-
});
184-
}
185-
186-
if b.is_nan() {
187-
return Some(if b.is_sign_negative() {
188-
Ordering::Greater
189-
} else {
190-
Ordering::Less
191-
});
192-
}
169+
fn iceberg_float_cmp_f32(a: OrderedFloat<f32>, b: OrderedFloat<f32>) -> Option<Ordering> {
170+
Some(a.total_cmp(&b))
171+
}
193172

194-
a.partial_cmp(&b)
173+
fn iceberg_float_cmp_f64(a: OrderedFloat<f64>, b: OrderedFloat<f64>) -> Option<Ordering> {
174+
Some(a.total_cmp(&b))
195175
}
196176

197177
impl PartialOrd for Datum {
198-
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
178+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
199179
match (&self.literal, &other.literal, &self.r#type, &other.r#type) {
200180
// generate the arm with same type and same literal
201181
(
@@ -221,13 +201,13 @@ impl PartialOrd for Datum {
221201
PrimitiveLiteral::Float(other_val),
222202
PrimitiveType::Float,
223203
PrimitiveType::Float,
224-
) => iceberg_float_cmp(*val, *other_val),
204+
) => iceberg_float_cmp_f32(*val, *other_val),
225205
(
226206
PrimitiveLiteral::Double(val),
227207
PrimitiveLiteral::Double(other_val),
228208
PrimitiveType::Double,
229209
PrimitiveType::Double,
230-
) => iceberg_float_cmp(*val, *other_val),
210+
) => iceberg_float_cmp_f64(*val, *other_val),
231211
(
232212
PrimitiveLiteral::Int(val),
233213
PrimitiveLiteral::Int(other_val),

crates/iceberg/src/spec/values/tests.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1293,6 +1293,31 @@ fn test_iceberg_float_order() {
12931293
assert_eq!(double_sorted, double_expected);
12941294
}
12951295

1296+
#[test]
1297+
fn test_negative_zero_less_than_positive_zero() {
1298+
{
1299+
let neg_zero = Datum::float(-0.0);
1300+
let pos_zero = Datum::float(0.0);
1301+
1302+
assert_eq!(
1303+
neg_zero.partial_cmp(&pos_zero),
1304+
Some(std::cmp::Ordering::Less),
1305+
"IEEE 754 totalOrder requires -0.0 < +0.0 on F32"
1306+
);
1307+
}
1308+
1309+
{
1310+
let neg_zero = Datum::double(-0.0);
1311+
let pos_zero = Datum::double(0.0);
1312+
1313+
assert_eq!(
1314+
neg_zero.partial_cmp(&pos_zero),
1315+
Some(std::cmp::Ordering::Less),
1316+
"IEEE 754 totalOrder requires -0.0 < +0.0 on F64"
1317+
);
1318+
}
1319+
}
1320+
12961321
/// Test Date deserialization from JSON as number (days since epoch).
12971322
///
12981323
/// This reproduces the scenario from Iceberg Java's TestAddFilesProcedure where:

0 commit comments

Comments
 (0)