From 95350e988ebc1d8ad9cb28b7e33b753c86d5a55e Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Wed, 29 Jul 2026 20:54:28 +0800 Subject: [PATCH 1/3] fix: order -NaN below +NaN in Literal comparison (#860) CompareFloat returned lhs_is_negative <=> rhs_is_negative for the both-NaN case, so -NaN compared as greater than +NaN. That contradicts the adjacent "-NAN < NAN" comment and the FloatSpecialValuesComparison / DoubleSpecialValuesComparison tests, which assert the total ordering -NaN < -Infinity < ... < +Infinity < +NaN. Swap the operands so a negative sign bit sorts below a positive one. The existing NaN tests only covered same-sign pairs (qNaN vs sNaN), so the mixed-sign case was unexercised; add FloatSignedNaNComparison and DoubleSignedNaNComparison to cover it. --- src/iceberg/expression/literal.cc | 4 +++- src/iceberg/test/literal_test.cc | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/iceberg/expression/literal.cc b/src/iceberg/expression/literal.cc index d11ab2656..8d0e723c2 100644 --- a/src/iceberg/expression/literal.cc +++ b/src/iceberg/expression/literal.cc @@ -444,7 +444,9 @@ std::strong_ordering CompareFloat(T lhs, T rhs) { // and -NAN < NAN. bool lhs_is_negative = std::signbit(lhs); bool rhs_is_negative = std::signbit(rhs); - return lhs_is_negative <=> rhs_is_negative; + // A negative sign bit sorts below a positive one (-NaN < +NaN), so a + // negative operand must compare as less. + return rhs_is_negative <=> lhs_is_negative; } namespace { diff --git a/src/iceberg/test/literal_test.cc b/src/iceberg/test/literal_test.cc index 433c4fbed..ff2cb80a3 100644 --- a/src/iceberg/test/literal_test.cc +++ b/src/iceberg/test/literal_test.cc @@ -216,6 +216,16 @@ TEST(LiteralTest, FloatNaNComparison) { EXPECT_EQ(nan1 <=> signaling_nan, std::partial_ordering::equivalent); } +TEST(LiteralTest, FloatSignedNaNComparison) { + auto neg_nan = Literal::Float(-std::numeric_limits::quiet_NaN()); + auto pos_nan = Literal::Float(std::numeric_limits::quiet_NaN()); + + // Per the total ordering -NaN < ... < +NaN, a negative NaN sorts below a + // positive NaN. + EXPECT_EQ(neg_nan <=> pos_nan, std::partial_ordering::less); + EXPECT_EQ(pos_nan <=> neg_nan, std::partial_ordering::greater); +} + TEST(LiteralTest, FloatInfinityComparison) { auto neg_inf = Literal::Float(-std::numeric_limits::infinity()); auto pos_inf = Literal::Float(std::numeric_limits::infinity()); @@ -267,6 +277,16 @@ TEST(LiteralTest, DoubleNaNComparison) { EXPECT_EQ(nan1 <=> signaling_nan, std::partial_ordering::equivalent); } +TEST(LiteralTest, DoubleSignedNaNComparison) { + auto neg_nan = Literal::Double(-std::numeric_limits::quiet_NaN()); + auto pos_nan = Literal::Double(std::numeric_limits::quiet_NaN()); + + // Per the total ordering -NaN < ... < +NaN, a negative NaN sorts below a + // positive NaN. + EXPECT_EQ(neg_nan <=> pos_nan, std::partial_ordering::less); + EXPECT_EQ(pos_nan <=> neg_nan, std::partial_ordering::greater); +} + TEST(LiteralTest, DoubleInfinityComparison) { auto neg_inf = Literal::Double(-std::numeric_limits::infinity()); auto pos_inf = Literal::Double(std::numeric_limits::infinity()); From 76e7fd1188de469b23fd554ef74fb6c39d510d11 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Thu, 30 Jul 2026 00:06:33 +0800 Subject: [PATCH 2/3] test: construct signed NaNs with std::copysign std::numeric_limits::quiet_NaN() does not guarantee a sign bit, so build the mixed-sign NaN operands with std::copysign to keep the test deterministic across platforms. --- src/iceberg/test/literal_test.cc | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/iceberg/test/literal_test.cc b/src/iceberg/test/literal_test.cc index ff2cb80a3..bb0914b8d 100644 --- a/src/iceberg/test/literal_test.cc +++ b/src/iceberg/test/literal_test.cc @@ -19,6 +19,7 @@ #include "iceberg/expression/literal.h" +#include #include #include #include @@ -217,8 +218,10 @@ TEST(LiteralTest, FloatNaNComparison) { } TEST(LiteralTest, FloatSignedNaNComparison) { - auto neg_nan = Literal::Float(-std::numeric_limits::quiet_NaN()); - auto pos_nan = Literal::Float(std::numeric_limits::quiet_NaN()); + auto neg_nan = + Literal::Float(std::copysign(std::numeric_limits::quiet_NaN(), -1.0f)); + auto pos_nan = + Literal::Float(std::copysign(std::numeric_limits::quiet_NaN(), +1.0f)); // Per the total ordering -NaN < ... < +NaN, a negative NaN sorts below a // positive NaN. @@ -278,8 +281,10 @@ TEST(LiteralTest, DoubleNaNComparison) { } TEST(LiteralTest, DoubleSignedNaNComparison) { - auto neg_nan = Literal::Double(-std::numeric_limits::quiet_NaN()); - auto pos_nan = Literal::Double(std::numeric_limits::quiet_NaN()); + auto neg_nan = + Literal::Double(std::copysign(std::numeric_limits::quiet_NaN(), -1.0)); + auto pos_nan = + Literal::Double(std::copysign(std::numeric_limits::quiet_NaN(), +1.0)); // Per the total ordering -NaN < ... < +NaN, a negative NaN sorts below a // positive NaN. From 1b5b9f460f2d6dbbe0489b26993d20110146c96e Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Fri, 31 Jul 2026 23:21:32 +0800 Subject: [PATCH 3/3] refactor: use std::strong_order for float comparison std::strong_order implements the IEEE 754 totalOrder predicate on IEC 559 types, which is exactly the ordering CompareFloat hand-rolled. Replace the manual NaN sign handling with a direct call. This distinguishes NaNs by bit pattern (a signaling NaN sorts below a quiet NaN of the same sign) instead of collapsing same-sign NaNs to equivalent; update the NaN comparison tests accordingly. --- src/iceberg/expression/literal.cc | 20 +++++--------------- src/iceberg/test/literal_test.cc | 12 ++++++++---- 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/src/iceberg/expression/literal.cc b/src/iceberg/expression/literal.cc index 8d0e723c2..aeb51710b 100644 --- a/src/iceberg/expression/literal.cc +++ b/src/iceberg/expression/literal.cc @@ -430,23 +430,13 @@ Result Literal::CastTo(const std::shared_ptr& target_typ return LiteralCaster::CastTo(*this, target_type); } -// Template function for floating point comparison following Iceberg rules: -// -NaN < NaN, but all NaN values (qNaN, sNaN) are treated as equivalent within their sign +// Template function for floating point comparison following the Iceberg total +// ordering: -NaN < -Infinity < ... < +Infinity < +NaN. std::strong_order +// implements the IEEE 754 totalOrder predicate on IEC 559 types, which matches +// this requirement (and orders -0 below +0). template std::strong_ordering CompareFloat(T lhs, T rhs) { - // If both are NaN, check their signs - bool all_nan = std::isnan(lhs) && std::isnan(rhs); - if (!all_nan) { - // If not both NaN, use strong ordering - return std::strong_order(lhs, rhs); - } - // Same sign NaN values are equivalent (no qNaN vs sNaN distinction), - // and -NAN < NAN. - bool lhs_is_negative = std::signbit(lhs); - bool rhs_is_negative = std::signbit(rhs); - // A negative sign bit sorts below a positive one (-NaN < +NaN), so a - // negative operand must compare as less. - return rhs_is_negative <=> lhs_is_negative; + return std::strong_order(lhs, rhs); } namespace { diff --git a/src/iceberg/test/literal_test.cc b/src/iceberg/test/literal_test.cc index bb0914b8d..712887785 100644 --- a/src/iceberg/test/literal_test.cc +++ b/src/iceberg/test/literal_test.cc @@ -212,9 +212,11 @@ TEST(LiteralTest, FloatNaNComparison) { auto nan2 = Literal::Float(std::numeric_limits::quiet_NaN()); auto signaling_nan = Literal::Float(std::numeric_limits::signaling_NaN()); - // NaN should be equal to itself in strong ordering + // Identical NaN bit patterns are equivalent under the total ordering. EXPECT_EQ(nan1 <=> nan2, std::partial_ordering::equivalent); - EXPECT_EQ(nan1 <=> signaling_nan, std::partial_ordering::equivalent); + // Total ordering distinguishes NaNs by bit pattern; a signaling NaN sorts + // below a quiet NaN of the same sign. + EXPECT_EQ(signaling_nan <=> nan1, std::partial_ordering::less); } TEST(LiteralTest, FloatSignedNaNComparison) { @@ -275,9 +277,11 @@ TEST(LiteralTest, DoubleNaNComparison) { auto nan2 = Literal::Double(std::numeric_limits::quiet_NaN()); auto signaling_nan = Literal::Double(std::numeric_limits::signaling_NaN()); - // NaN should be equal to itself in strong ordering + // Identical NaN bit patterns are equivalent under the total ordering. EXPECT_EQ(nan1 <=> nan2, std::partial_ordering::equivalent); - EXPECT_EQ(nan1 <=> signaling_nan, std::partial_ordering::equivalent); + // Total ordering distinguishes NaNs by bit pattern; a signaling NaN sorts + // below a quiet NaN of the same sign. + EXPECT_EQ(signaling_nan <=> nan1, std::partial_ordering::less); } TEST(LiteralTest, DoubleSignedNaNComparison) {