From 3e7a3bacb132aff3e0458d41e1b3e12635f51f02 Mon Sep 17 00:00:00 2001 From: Ayush Date: Tue, 10 Nov 2026 08:32:47 +0000 Subject: [PATCH] GH-55393: [C++][Compute] Fix precision and ties in floating round --- cpp/src/arrow/compute/kernels/scalar_round.cc | 15 +- .../kernels/scalar_round_arithmetic_test.cc | 150 ++++++++++++++++++ 2 files changed, 158 insertions(+), 7 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_round.cc b/cpp/src/arrow/compute/kernels/scalar_round.cc index f762f9e0fdd1..0ed322028400 100644 --- a/cpp/src/arrow/compute/kernels/scalar_round.cc +++ b/cpp/src/arrow/compute/kernels/scalar_round.cc @@ -90,15 +90,16 @@ struct RoundUtil { // Calculate powers of ten with arbitrary integer exponent template static enable_if_floating_value Pow10(int64_t power) { - static constexpr T lut[] = {1e0F, 1e1F, 1e2F, 1e3F, 1e4F, 1e5F, 1e6F, 1e7F, - 1e8F, 1e9F, 1e10F, 1e11F, 1e12F, 1e13F, 1e14F, 1e15F}; + static constexpr T lut[] = {T(1e0), T(1e1), T(1e2), T(1e3), T(1e4), T(1e5), + T(1e6), T(1e7), T(1e8), T(1e9), T(1e10), T(1e11), + T(1e12), T(1e13), T(1e14), T(1e15)}; int64_t lut_size = (sizeof(lut) / sizeof(*lut)); int64_t abs_power = std::abs(power); auto pow10 = lut[std::min(abs_power, lut_size - 1)]; while (abs_power-- >= lut_size) { - pow10 *= 1e1F; + pow10 *= T(1e1); } - return (power >= 0) ? pow10 : (1 / pow10); + return (power >= 0) ? pow10 : (T(1) / pow10); } // Calculate powers of ten with arbitrary integer exponent @@ -548,7 +549,7 @@ struct RoundToMultiple { return arg; } auto round_val = arg / multiple; - auto frac = round_val - std::floor(round_val); + auto frac = std::abs(round_val - std::trunc(round_val)); if (frac != T(0)) { // Use std::round() if in tie-breaking mode and scaled value is not 0.5. if ((kRoundMode >= RoundMode::HALF_DOWN) && (frac != T(0.5))) { @@ -741,7 +742,7 @@ struct Round { return arg; } auto round_val = ndigits >= 0 ? (arg * pow10) : (arg / pow10); - auto frac = round_val - std::floor(round_val); + auto frac = std::abs(round_val - std::trunc(round_val)); if (frac != T(0)) { // Use std::round() if in tie-breaking mode and scaled value is not 0.5. if ((RndMode >= RoundMode::HALF_DOWN) && (frac != T(0.5))) { @@ -887,7 +888,7 @@ struct RoundBinary { double pow10 = RoundUtil::Pow10(std::abs(arg1)); auto round_val = arg1 >= 0 ? (arg0 * pow10) : (arg0 / pow10); - auto frac = round_val - std::floor(round_val); + auto frac = std::abs(round_val - std::trunc(round_val)); if (frac != T(0)) { // Use std::round() if in tie-breaking mode and scaled value is not 0.5. if ((RndMode >= RoundMode::HALF_DOWN) && (frac != T(0.5))) { diff --git a/cpp/src/arrow/compute/kernels/scalar_round_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_round_arithmetic_test.cc index 1e9e0c9d50cf..066972b76cb9 100644 --- a/cpp/src/arrow/compute/kernels/scalar_round_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_round_arithmetic_test.cc @@ -1321,6 +1321,128 @@ TYPED_TEST(TestUnaryRoundFloating, Round) { } } +TYPED_TEST(TestUnaryRoundFloating, RoundRegressionGH55393) { + this->SetNansEqual(true); + + if constexpr (std::is_same_v) { + // Powers of 10 must retain double precision for large negative ndigits (GH-55393) + std::vector>> pow10_tests = { + {-1, {"[25.0]", "[20.0]"}}, + {-2, {"[250.0]", "[200.0]"}}, + {-6, {"[2500000.0]", "[2000000.0]"}}, + {-8, {"[250000000.0]", "[200000000.0]"}}, + {-10, {"[25000000000.0]", "[20000000000.0]"}}, + {-11, {"[250000000000.0]", "[200000000000.0]"}}, + {-12, {"[2500000000000.0]", "[2000000000000.0]"}}, + {-14, {"[250000000000000.0]", "[200000000000000.0]"}}, + {-15, {"[2500000000000000.0]", "[2000000000000000.0]"}}, + {-18, {"[2.5e18]", "[2.0e18]"}}, + {-20, {"[2.5e20]", "[2.0e20]"}}, + {-22, {"[2.5e22]", "[2.0e22]"}}, + }; + this->SetRoundMode(RoundMode::HALF_TO_EVEN); + for (const auto& test : pow10_tests) { + this->SetRoundNdigits(test.first); + this->AssertUnaryOp(Round, test.second.first, test.second.second); + } + + // Negative values with negative ndigits under different tie-breaking modes + this->SetRoundNdigits(-11); + this->SetRoundMode(RoundMode::HALF_UP); + this->AssertUnaryOp(Round, "[250000000000.0, -250000000000.0]", + "[300000000000.0, -200000000000.0]"); + this->SetRoundMode(RoundMode::HALF_DOWN); + this->AssertUnaryOp(Round, "[250000000000.0, -250000000000.0]", + "[200000000000.0, -300000000000.0]"); + this->SetRoundMode(RoundMode::HALF_TO_ODD); + this->AssertUnaryOp(Round, "[250000000000.0, -250000000000.0]", + "[300000000000.0, -300000000000.0]"); + + // Negative inputs near halfway points must not falsely trigger tie-breaking + this->SetRoundNdigits(-14); + for (const auto mode : {RoundMode::HALF_DOWN, RoundMode::HALF_UP, + RoundMode::HALF_TO_EVEN, RoundMode::HALF_TOWARDS_ZERO, + RoundMode::HALF_TOWARDS_INFINITY, RoundMode::HALF_TO_ODD}) { + this->SetRoundMode(mode); + this->AssertUnaryOp(Round, "[-49999999999999.9921875]", "[-0.0]"); + } + } + + // Exact ties across all 10 rounding modes + std::string ties("[-2.5, -1.5, -0.5, 0.5, 1.5, 2.5]"); + std::vector> tie_expected{{ + {RoundMode::DOWN, "[-3.0, -2.0, -1.0, 0.0, 1.0, 2.0]"}, + {RoundMode::UP, "[-2.0, -1.0, -0.0, 1.0, 2.0, 3.0]"}, + {RoundMode::TOWARDS_ZERO, "[-2.0, -1.0, -0.0, 0.0, 1.0, 2.0]"}, + {RoundMode::TOWARDS_INFINITY, "[-3.0, -2.0, -1.0, 1.0, 2.0, 3.0]"}, + {RoundMode::HALF_DOWN, "[-3.0, -2.0, -1.0, 0.0, 1.0, 2.0]"}, + {RoundMode::HALF_UP, "[-2.0, -1.0, -0.0, 1.0, 2.0, 3.0]"}, + {RoundMode::HALF_TOWARDS_ZERO, "[-2.0, -1.0, -0.0, 0.0, 1.0, 2.0]"}, + {RoundMode::HALF_TOWARDS_INFINITY, "[-3.0, -2.0, -1.0, 1.0, 2.0, 3.0]"}, + {RoundMode::HALF_TO_EVEN, "[-2.0, -2.0, -0.0, 0.0, 2.0, 2.0]"}, + {RoundMode::HALF_TO_ODD, "[-3.0, -1.0, -1.0, 1.0, 1.0, 3.0]"}, + }}; + this->SetRoundNdigits(0); + for (const auto& pair : tie_expected) { + this->SetRoundMode(pair.first); + this->AssertUnaryOp(Round, ties, pair.second); + } + + // Non-ties near halfway boundaries + std::string non_ties("[-0.49, 0.49, -0.51, 0.51, -1.49, 1.49, -1.51, 1.51]"); + this->SetRoundMode(RoundMode::HALF_TO_EVEN); + this->AssertUnaryOp(Round, non_ties, "[-0.0, 0.0, -1.0, 1.0, -1.0, 1.0, -2.0, 2.0]"); + this->SetRoundMode(RoundMode::HALF_UP); + this->AssertUnaryOp(Round, non_ties, "[-0.0, 0.0, -1.0, 1.0, -1.0, 1.0, -2.0, 2.0]"); + this->SetRoundMode(RoundMode::HALF_DOWN); + this->AssertUnaryOp(Round, non_ties, "[-0.0, 0.0, -1.0, 1.0, -1.0, 1.0, -2.0, 2.0]"); + + // Adjacent floating-point neighbors of halfway points via std::nextafter + using CType = typename TypeTraits::CType; + const CType half = static_cast(0.5); + const CType neg_half = static_cast(-0.5); + const CType neg_half_towards_zero = std::nextafter(neg_half, static_cast(0.0)); + const CType neg_half_away_from_zero = + std::nextafter(neg_half, -std::numeric_limits::infinity()); + const CType pos_half_below = std::nextafter(half, static_cast(0.0)); + const CType pos_half_above = + std::nextafter(half, std::numeric_limits::infinity()); + + std::shared_ptr nextafter_inputs; + ArrayFromVector( + std::vector{neg_half_towards_zero, neg_half_away_from_zero, pos_half_below, + pos_half_above, neg_half, half}, + &nextafter_inputs); + + std::shared_ptr nextafter_expected_even; + ArrayFromVector( + std::vector{static_cast(-0.0), static_cast(-1.0), + static_cast(0.0), static_cast(1.0), + static_cast(-0.0), static_cast(0.0)}, + &nextafter_expected_even); + + this->SetRoundNdigits(0); + this->SetRoundMode(RoundMode::HALF_TO_EVEN); + this->AssertUnaryOp(Round, nextafter_inputs, nextafter_expected_even); + + // Preserve signed zero for floating-point values + auto check_sign = [&](CType in_val, bool expect_negative) { + auto in_scalar = this->MakeScalar(in_val); + ASSERT_OK_AND_ASSIGN(auto res_datum, Round(in_scalar, this->options_, nullptr)); + CType out_val = + checked_cast&>(*res_datum.scalar()).value; + EXPECT_EQ(out_val, static_cast(0.0)); + EXPECT_EQ(std::signbit(out_val), expect_negative); + }; + this->SetRoundMode(RoundMode::HALF_TO_EVEN); + check_sign(static_cast(-0.0), true); + check_sign(static_cast(0.0), false); + check_sign(static_cast(-0.1), true); + check_sign(static_cast(0.1), false); + check_sign(neg_half, true); + check_sign(half, false); +} + TYPED_TEST_SUITE(TestBinaryRoundIntegral, IntegralTypes); TYPED_TEST_SUITE(TestBinaryRoundSigned, SignedIntegerTypes); TYPED_TEST_SUITE(TestBinaryRoundUnsigned, UnsignedIntegerTypes); @@ -1487,6 +1609,19 @@ TYPED_TEST(TestBinaryRoundFloating, Round) { } } +TYPED_TEST(TestBinaryRoundFloating, RoundRegressionGH55393) { + this->SetNansEqual(true); + + if constexpr (std::is_same_v) { + // GH-55393: Power-of-ten precision and halfway-tie classification in RoundBinary + this->SetRoundMode(RoundMode::HALF_TO_EVEN); + this->AssertBinaryOp(RoundBinary, "[250000000000.0]", -11, "[200000000000.0]"); + + this->SetRoundMode(RoundMode::HALF_DOWN); + this->AssertBinaryOp(RoundBinary, "[-49999999999999.9921875]", -14, "[-0.0]"); + } +} + TYPED_TEST_SUITE(TestUnaryRoundToMultipleIntegral, IntegralTypes); TYPED_TEST_SUITE(TestUnaryRoundToMultipleSigned, SignedIntegerTypes); TYPED_TEST_SUITE(TestUnaryRoundToMultipleUnsigned, UnsignedIntegerTypes); @@ -1692,6 +1827,21 @@ TYPED_TEST(TestUnaryRoundToMultipleFloating, RoundToMultiple) { "Rounding multiple must be positive"); } +TYPED_TEST(TestUnaryRoundToMultipleFloating, RoundToMultipleRegressionGH55393) { + this->SetNansEqual(true); + + if constexpr (std::is_same_v) { + // GH-55393: Halfway-tie classification with large multiples in RoundToMultiple + this->SetRoundMultiple(100000000000.0); + this->SetRoundMode(RoundMode::HALF_TO_EVEN); + this->AssertUnaryOp(RoundToMultiple, "[250000000000.0]", "[200000000000.0]"); + + this->SetRoundMultiple(100000000000000.0); + this->SetRoundMode(RoundMode::HALF_DOWN); + this->AssertUnaryOp(RoundToMultiple, "[-49999999999999.9921875]", "[-0.0]"); + } +} + TYPED_TEST(TestUnaryRoundArithmeticSigned, Floor) { auto floor = [](const Datum& arg, const ArithmeticOptions&, ExecContext* ctx) { return Floor(arg, ctx);