Copilot commented on code in PR #861:
URL: https://github.com/apache/iceberg-cpp/pull/861#discussion_r3674558212
##########
src/iceberg/test/literal_test.cc:
##########
@@ -216,6 +216,16 @@ TEST(LiteralTest, FloatNaNComparison) {
EXPECT_EQ(nan1 <=> signaling_nan, std::partial_ordering::equivalent);
}
+TEST(LiteralTest, FloatSignedNaNComparison) {
+ auto neg_nan = Literal::Float(-std::numeric_limits<float>::quiet_NaN());
+ auto pos_nan = Literal::Float(std::numeric_limits<float>::quiet_NaN());
Review Comment:
`std::numeric_limits<T>::quiet_NaN()` does not guarantee the sign bit of the
produced NaN. On some platforms `quiet_NaN()` may already be negative, which
can make `pos_nan` negative and/or make the test flaky. To make the test
deterministic, construct NaNs with explicit sign bits (e.g., via
`std::copysign(quiet_NaN(), +1.0f/-1.0f)` or `std::bit_cast` from known
IEEE-754 NaN bit patterns).
##########
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:
The comment explains the desired ordering, but the implementation uses
reversed operands (`rhs <=> lhs`) which is easy to misread later. Consider
rewriting this branch to express the intent directly (e.g., by comparing
`lhs_is_negative`/`rhs_is_negative` with an explicit conditional or an explicit
expression that reads as “lhs negative sorts first”), so the code and comment
are self-evident and reduce the chance of future regressions.
--
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]
