Skip to content

Commit

Permalink
Normalize NaNs in to_ieee754_32 and to_ieee754_64
Browse files Browse the repository at this point in the history
Summary:
We noticed a case in the fuzzer where different code paths or builds
(specifically ASAN) generated NaNs with different binary
representations before being fed to either of the aforementioned
UDFs. To be more specific, with ASAN builds we noticed that the exact
same input when added returned different results in different
iterations. For example, NaN + (-NaN) sometimes returned NaN and
sometimes -NAN. When this was fed into to_ieee754_32, the binary
representation came out different between fuzzer runs and the results
mismatched.
Therefore, this change ensures that the output of to_ieee754_32/64
for NaNs is consistent across different binary representations so
that to_ieee754_32(NaN) = to_ieee754_32(NaN) and NaN=NaN remain
true (in velox and presto all NaNs are considered equal).

Differential Revision: D65238915
  • Loading branch information
Bikramjeet Vig authored and facebook-github-bot committed Oct 30, 2024
1 parent 7377bc8 commit bcdc77c
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 3 deletions.
12 changes: 10 additions & 2 deletions velox/functions/prestosql/BinaryFunctions.h
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,11 @@ struct ToIEEE754Bits64 {
out_type<Varbinary>& result,
const arg_type<double>& input) {
static constexpr auto kTypeLength = sizeof(int64_t);
auto value = folly::Endian::big(input);
// Since we consider NaNs with different binary representation as equal, we
// normalize them to a single value to ensure the output is equal too.
auto value = std::isnan(input)
? folly::Endian::big(std::numeric_limits<double>::quiet_NaN())
: folly::Endian::big(input);
result.setNoCopy(
StringView(reinterpret_cast<const char*>(&value), kTypeLength));
}
Expand Down Expand Up @@ -416,7 +420,11 @@ struct ToIEEE754Bits32 {
out_type<Varbinary>& result,
const arg_type<float>& input) {
static constexpr auto kTypeLength = sizeof(int32_t);
auto value = folly::Endian::big(input);
// Since we consider NaNs with different binary representation as equal, we
// normalize them to a single value to ensure the output is equal too.
auto value = std::isnan(input)
? folly::Endian::big(std::numeric_limits<float>::quiet_NaN())
: folly::Endian::big(input);
result.setNoCopy(
StringView(reinterpret_cast<const char*>(&value), kTypeLength));
}
Expand Down
9 changes: 8 additions & 1 deletion velox/functions/prestosql/tests/BinaryFunctionsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -635,8 +635,10 @@ TEST_F(BinaryFunctionsTest, toIEEE754Bits64) {
EXPECT_EQ(
hexToDec("7FF8000000000000"),
toIEEE754Bits64(std::numeric_limits<double>::quiet_NaN()));
// NaNs are normalized when generating output to ensure they are equal as all
// NaNs are considered equal
EXPECT_EQ(
hexToDec("7FF4000000000000"),
hexToDec("7FF8000000000000"),
toIEEE754Bits64(std::numeric_limits<double>::signaling_NaN()));
EXPECT_EQ(
hexToDec("FFF0000000000000"),
Expand Down Expand Up @@ -698,6 +700,11 @@ TEST_F(BinaryFunctionsTest, toIEEE754Bits32) {
EXPECT_EQ(
hexToDec("7FC00000"),
toIEEE754Bits32(std::numeric_limits<float>::quiet_NaN()));
// NaNs are normalized when generating output to ensure they are equal as all
// NaNs are considered equal
EXPECT_EQ(
hexToDec("7FC00000"),
toIEEE754Bits32(std::numeric_limits<float>::signaling_NaN()));
EXPECT_EQ(
hexToDec("7F800000"),
toIEEE754Bits32(std::numeric_limits<float>::infinity()));
Expand Down

0 comments on commit bcdc77c

Please sign in to comment.