fix: order -NaN below +NaN in Literal comparison

[LuciferYang]

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:

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).

[LuciferYang]

cc @wgtmac FYI

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes NaN total-ordering behavior in Literal::operator<=> so that -NaN sorts below +NaN, aligning implementation with the documented and tested ordering and adding missing mixed-sign NaN coverage (Fixes #860).

Changes:

• Corrected NaN sign-bit comparison in CompareFloat to order -NaN < +NaN.
• Added new float/double tests covering mixed-sign NaN comparisons.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/iceberg/test/literal_test.cc	Adds regression tests for signed NaN ordering for float and double.
src/iceberg/expression/literal.cc	Fixes NaN ordering logic to sort negative-sign NaNs below positive-sign NaNs.

[LuciferYang]

Keeping rhs <=> lhs: it's the idiom for reversing the order relation, and the comment above states the intent. The SignedNaNComparison tests pin the direction either way.

[wgtmac]

Can we directly use function proposed by https://parquet.apache.org/blog/2026/05/29/taming-floating-point-statistics-in-apache-parquet-ieee-754-total-order-and-nan-counts/ for IEEE754 total order?

pub fn totalOrder(x: f64, y: f64) -> bool {
    let mut x_int = x.to_bits() as i64;
    let mut y_int = y.to_bits() as i64;
    x_int ^= (((x_int >> 63) as u64) >> 1) as i64;
    y_int ^= (((y_int >> 63) as u64) >> 1) as i64;
    return x_int <= y_int;
}

It is a piece of Rust code for f64 but would be easy to be adapted to f32.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/iceberg/expression/literal.cc:439

• The PR description/issue discussion describes fixing only the -NaN vs +NaN ordering by swapping sign-bit operands, but the implementation here switches to std::strong_order for all float comparisons, which also changes semantics for NaNs with different payloads/quiet-vs-signaling bits (and therefore Literal::operator==). Please confirm this broader behavior change is intended and align the PR description accordingly; also consider clarifying it in the comment to prevent future confusion.

// 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) {
  return std::strong_order(lhs, rhs);

src/iceberg/test/literal_test.cc:219

• FloatNaNComparison assumes (1) signaling NaNs are supported and (2) that a signaling NaN sorts below the quiet NaN returned by numeric_limits. has_signaling_NaN may be false on some platforms, and even when true the relative totalOrder between the library’s chosen signaling/quiet bit patterns is not guaranteed. This can make the test non-portable/flaky.

  // Identical NaN bit patterns are equivalent under the total ordering.
  EXPECT_EQ(nan1 <=> nan2, 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);

src/iceberg/test/literal_test.cc:284

• DoubleNaNComparison assumes signaling NaNs exist and that signaling_NaN() sorts below quiet_NaN() under the total ordering. has_signaling_NaN can be false, and the relative ordering of the implementation-chosen NaN payloads isn’t guaranteed, so the test may be non-portable.

  // Identical NaN bit patterns are equivalent under the total ordering.
  EXPECT_EQ(nan1 <=> nan2, 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);