From 95350e988ebc1d8ad9cb28b7e33b753c86d5a55e Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Wed, 29 Jul 2026 20:54:28 +0800 Subject: [PATCH 1/2] 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/2] 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.