Skip to content

GH-55393: [C++][Compute] Fix precision and ties in floating round #55470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Ayush-CS-89112521 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GH-55393: [C++][Compute] Fix precision and ties in floating round #55470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions cpp/src/arrow/compute/kernels/scalar_round.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,16 @@ struct RoundUtil {
// Calculate powers of ten with arbitrary integer exponent
template <typename T>
static enable_if_floating_value<T> 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
Expand Down Expand Up @@ -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))) {
Expand Down Expand Up @@ -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))) {
Expand Down Expand Up @@ -887,7 +888,7 @@ struct RoundBinary {
double pow10 = RoundUtil::Pow10<double>(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))) {
Expand Down
150 changes: 150 additions & 0 deletions cpp/src/arrow/compute/kernels/scalar_round_arithmetic_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1321,6 +1321,128 @@ TYPED_TEST(TestUnaryRoundFloating, Round) {
}
}

TYPED_TEST(TestUnaryRoundFloating, RoundRegressionGH55393) {
this->SetNansEqual(true);

if constexpr (std::is_same_v<TypeParam, DoubleType>) {
// Powers of 10 must retain double precision for large negative ndigits (GH-55393)
std::vector<std::pair<int64_t, std::pair<std::string, std::string>>> 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<std::pair<RoundMode, std::string>> 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<TypeParam>::CType;
const CType half = static_cast<CType>(0.5);
const CType neg_half = static_cast<CType>(-0.5);
const CType neg_half_towards_zero = std::nextafter(neg_half, static_cast<CType>(0.0));
const CType neg_half_away_from_zero =
std::nextafter(neg_half, -std::numeric_limits<CType>::infinity());
const CType pos_half_below = std::nextafter(half, static_cast<CType>(0.0));
const CType pos_half_above =
std::nextafter(half, std::numeric_limits<CType>::infinity());

std::shared_ptr<Array> nextafter_inputs;
ArrayFromVector<TypeParam>(
std::vector<CType>{neg_half_towards_zero, neg_half_away_from_zero, pos_half_below,
pos_half_above, neg_half, half},
&nextafter_inputs);

std::shared_ptr<Array> nextafter_expected_even;
ArrayFromVector<TypeParam>(
std::vector<CType>{static_cast<CType>(-0.0), static_cast<CType>(-1.0),
static_cast<CType>(0.0), static_cast<CType>(1.0),
static_cast<CType>(-0.0), static_cast<CType>(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<const NumericScalar<TypeParam>&>(*res_datum.scalar()).value;
EXPECT_EQ(out_val, static_cast<CType>(0.0));
EXPECT_EQ(std::signbit(out_val), expect_negative);
};
this->SetRoundMode(RoundMode::HALF_TO_EVEN);
check_sign(static_cast<CType>(-0.0), true);
check_sign(static_cast<CType>(0.0), false);
check_sign(static_cast<CType>(-0.1), true);
check_sign(static_cast<CType>(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);
Expand Down Expand Up @@ -1487,6 +1609,19 @@ TYPED_TEST(TestBinaryRoundFloating, Round) {
}
}

TYPED_TEST(TestBinaryRoundFloating, RoundRegressionGH55393) {
this->SetNansEqual(true);

if constexpr (std::is_same_v<TypeParam, DoubleType>) {
// 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);
Expand Down Expand Up @@ -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<TypeParam, DoubleType>) {
// 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);
Expand Down
Loading