fix: order -NaN below +NaN in Literal comparison - #861
Conversation
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.
|
cc @wgtmac FYI |
There was a problem hiding this comment.
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
CompareFloatto 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. |
| auto neg_nan = Literal::Float(-std::numeric_limits<float>::quiet_NaN()); | ||
| auto pos_nan = Literal::Float(std::numeric_limits<float>::quiet_NaN()); |
| // 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; |
There was a problem hiding this comment.
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.
std::numeric_limits<T>::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.
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.CompareFloatreturnslhs_is_negative <=> rhs_is_negativefor the both-NaN case, so-NaN <=> +NaNistrue <=> false=greater. The adjacent comment says "-NAN < NAN", andFloatSpecialValuesComparison/DoubleSpecialValuesComparisonassert-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/DoubleNaNComparisontests only cover same-sign NaN pairs (qNaN vs sNaN, which are equivalent), so the mixed-sign case was unexercised. AddedFloatSignedNaNComparisonandDoubleSignedNaNComparisonasserting-NaN < +NaNand the reverse. Verified fail-without (the new tests reportgreater/lessswapped) / pass-with. Fullexpression_testpasses (495 tests).