Skip to content

Commit 587fb69

Browse files
authored
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).
1 parent 75b7d24 commit 587fb69

2 files changed

Lines changed: 32 additions & 19 deletions

File tree

src/iceberg/expression/literal.cc

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -430,21 +430,13 @@ Result<Literal> Literal::CastTo(const std::shared_ptr<PrimitiveType>& target_typ
430430
return LiteralCaster::CastTo(*this, target_type);
431431
}
432432

433-
// Template function for floating point comparison following Iceberg rules:
434-
// -NaN < NaN, but all NaN values (qNaN, sNaN) are treated as equivalent within their sign
433+
// Template function for floating point comparison following the Iceberg total
434+
// ordering: -NaN < -Infinity < ... < +Infinity < +NaN. std::strong_order
435+
// implements the IEEE 754 totalOrder predicate on IEC 559 types, which matches
436+
// this requirement (and orders -0 below +0).
435437
template <std::floating_point T>
436438
std::strong_ordering CompareFloat(T lhs, T rhs) {
437-
// If both are NaN, check their signs
438-
bool all_nan = std::isnan(lhs) && std::isnan(rhs);
439-
if (!all_nan) {
440-
// If not both NaN, use strong ordering
441-
return std::strong_order(lhs, rhs);
442-
}
443-
// Same sign NaN values are equivalent (no qNaN vs sNaN distinction),
444-
// and -NAN < NAN.
445-
bool lhs_is_negative = std::signbit(lhs);
446-
bool rhs_is_negative = std::signbit(rhs);
447-
return lhs_is_negative <=> rhs_is_negative;
439+
return std::strong_order(lhs, rhs);
448440
}
449441

450442
namespace {

src/iceberg/test/literal_test.cc

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
#include "iceberg/expression/literal.h"
2121

22+
#include <cmath>
2223
#include <limits>
2324
#include <numbers>
2425
#include <unordered_set>
@@ -209,11 +210,21 @@ TEST(LiteralTest, FloatSpecialValuesComparison) {
209210
TEST(LiteralTest, FloatNaNComparison) {
210211
auto nan1 = Literal::Float(std::numeric_limits<float>::quiet_NaN());
211212
auto nan2 = Literal::Float(std::numeric_limits<float>::quiet_NaN());
212-
auto signaling_nan = Literal::Float(std::numeric_limits<float>::signaling_NaN());
213213

214-
// NaN should be equal to itself in strong ordering
214+
// Identical NaN bit patterns are equivalent under the total ordering.
215215
EXPECT_EQ(nan1 <=> nan2, std::partial_ordering::equivalent);
216-
EXPECT_EQ(nan1 <=> signaling_nan, std::partial_ordering::equivalent);
216+
}
217+
218+
TEST(LiteralTest, FloatSignedNaNComparison) {
219+
auto neg_nan =
220+
Literal::Float(std::copysign(std::numeric_limits<float>::quiet_NaN(), -1.0f));
221+
auto pos_nan =
222+
Literal::Float(std::copysign(std::numeric_limits<float>::quiet_NaN(), +1.0f));
223+
224+
// Per the total ordering -NaN < ... < +NaN, a negative NaN sorts below a
225+
// positive NaN.
226+
EXPECT_EQ(neg_nan <=> pos_nan, std::partial_ordering::less);
227+
EXPECT_EQ(pos_nan <=> neg_nan, std::partial_ordering::greater);
217228
}
218229

219230
TEST(LiteralTest, FloatInfinityComparison) {
@@ -260,11 +271,21 @@ TEST(LiteralTest, DoubleSpecialValuesComparison) {
260271
TEST(LiteralTest, DoubleNaNComparison) {
261272
auto nan1 = Literal::Double(std::numeric_limits<double>::quiet_NaN());
262273
auto nan2 = Literal::Double(std::numeric_limits<double>::quiet_NaN());
263-
auto signaling_nan = Literal::Double(std::numeric_limits<double>::signaling_NaN());
264274

265-
// NaN should be equal to itself in strong ordering
275+
// Identical NaN bit patterns are equivalent under the total ordering.
266276
EXPECT_EQ(nan1 <=> nan2, std::partial_ordering::equivalent);
267-
EXPECT_EQ(nan1 <=> signaling_nan, std::partial_ordering::equivalent);
277+
}
278+
279+
TEST(LiteralTest, DoubleSignedNaNComparison) {
280+
auto neg_nan =
281+
Literal::Double(std::copysign(std::numeric_limits<double>::quiet_NaN(), -1.0));
282+
auto pos_nan =
283+
Literal::Double(std::copysign(std::numeric_limits<double>::quiet_NaN(), +1.0));
284+
285+
// Per the total ordering -NaN < ... < +NaN, a negative NaN sorts below a
286+
// positive NaN.
287+
EXPECT_EQ(neg_nan <=> pos_nan, std::partial_ordering::less);
288+
EXPECT_EQ(pos_nan <=> neg_nan, std::partial_ordering::greater);
268289
}
269290

270291
TEST(LiteralTest, DoubleInfinityComparison) {

0 commit comments

Comments
 (0)