LuciferYang commented on code in PR #861:
URL: https://github.com/apache/iceberg-cpp/pull/861#discussion_r3696047613
##########
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;
Review Comment:
Thanks for the pointer. There are two separate questions here and I'd rather
ask than guess on the second one.
On the mechanics: `std::strong_order` already implements the IEEE 754
totalOrder predicate on IEC 559 types, so we don't need to port the bit trick.
The bit trick is a bitwise totalOrder and the standard requires
`std::strong_order` to be consistent with totalOrder when `is_iec559` is true,
so they're equivalent by construction. I also sanity-checked ±0, ±Inf and mixed
NaN bit patterns locally and they agree. That part is just a spelling choice.
The semantics are what I'd like your call on. totalOrder distinguishes NaNs
by bit pattern, so NaNs differing in the signaling bit or payload are no longer
equal to each other. That's finer than the spec asks for: the sorting section
only requires `-NaN` < `-Infinity` < ... < `Infinity` < `NaN`, i.e. sign only,
and NaNs aren't permitted as lower or upper bounds anyway. And because
`Literal::operator==` is `(*this <=> other) == 0`, this changes NaN equality
for `Literal`, not just ordering.
Java isn't a usable reference point here. `Comparators` uses
`Comparator.naturalOrder()` for float/double, i.e. `Double.compare`, which
canonicalizes every NaN through `doubleToLongBits`. On JDK 17 I get
`Double.compare(-NaN, +NaN) == 0` and `Double.compare(-NaN, -Infinity) == 1`:
all NaNs equal, sorting above `+Infinity`. That contradicts the spec's own
`-NaN` < `-Infinity` ordering even though the spec claims to align with Java,
and our `FloatSpecialValuesComparison` follows the spec rather than Java.
One thing that does argue for (a): it keeps `Literal` equality consistent
with `LiteralValueHash`, which hashes floats by bit pattern. Under (b) two
same-sign NaNs with different payloads compare equal but hash differently,
which breaks the `unordered_set` invariant behind IN/NOT IN literal sets unless
we canonicalize NaN in the hash too. The `std::vector<Literal>` overload of
`UnboundPredicateImpl::Make` doesn't reject NaN, so such a set is constructible
today.
So which do you prefer: (a) full totalOrder via `std::strong_order`, or (b)
keep the NaN special case and only fix the sign inversion, which matches the
sorting requirement exactly and leaves NaN equality alone? I've pushed (a) so
CI covers it. Switching to (b) is just reverting the last two commits and
restoring the same-sign NaN equivalence assertions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
