Skip to content

[Python][C++][Compute] Fix correctness of decimal round_binary kernel #50641

Description

@dmsa-or

Describe the bug, including details regarding any error messages, version, and platform.

The round_binary kernel for decimal input does not properly compute the halfway remainder point (half_pow10) elementwise based on the actual ndigits. Instead it is always computed as if ndigits is 0. This leads half_pow10 to be off by a factor of 10^(ndigits) and throws off the rounding logic.

import pyarrow as pa
import pyarrow.compute as pc
from decimal import Decimal

>>> pc.round_binary(Decimal("2.89"), 1)
<pyarrow.Decimal128Scalar: Decimal('2.80')>   # should be 2.9
>>> pc.round_binary(Decimal("-5.927"), 2)
<pyarrow.Decimal128Scalar: Decimal('-5.920')>   # should be -5.93
>>> pc.round_binary(Decimal("-134.2"), -1)
<pyarrow.Decimal128Scalar: Decimal('-140.0')>   # should be -130.0
>>> pc.round_binary(Decimal("211"), -2)
<pyarrow.Decimal128Scalar: Decimal('300')>    # should be 200

It also falls through to a no-op for any decimal input with negative scale.

negative_scale_input = pa.scalar(Decimal("3700"), type=pa.decimal128(4, -2))
>>> pc.round_binary(negative_scale_input, -3)
<pyarrow.Decimal128Scalar: Decimal('3.7E+3')>    # should be 4.0E+3

In summary, decimal round_binary returns the wrong answer when ndigits < scale (the standard case when rounding) AND one of the following are true:

  • Scale < 0
  • One of the rounding modes HALF_DOWN, HALF_UP, HALF_TOWARDS_ZERO, HALF_TOWARDS_INFINITY, HALF_TO_EVEN, or HALF_TO_ODD are used, AND:
    • ndigits > 0 and input is closer to ceil(input, ndigits) than floor(input, ndigits), OR
    • ndigits < 0, abs(input) % 10^(-ndigits) > 0.5 (true 90% of the time for ndigits == -1, and even more likely for ndigits < -1), and input is closer to ceil(input, ndigits) than floor(input, ndigits),

The only cases when decimal round_binary gets the right answer are when:

  • ndigits == 0,
  • ndigits >= scale (no rounding needed),
  • scale >= 0 and either of the following are true:
    • One of the rounding modes DOWN, UP, TOWARDS_ZERO, or TOWARDS_INFINITY are used
    • ndigits < 0 and abs(input) % 10^(-ndigits) <= 0.5, which is rare
  • OR a buggy round-to-nearest mode is used and happens to return the nearest number just by luck

Thus we conclude that decimal round_binary is completely unreliable at the moment for the most common rounding modes when ndigits != 0, only getting the correct answer for about 50% of inputs by chance. This is a major issue since the only reason one would want to use round_binary over round in the first place is if some elements should be rounded to nonzero ndigits of precision.

See the current implementation:

explicit RoundBinary(const DataType& out_ty)


Aside: there's also a bug in the decimal implementation of HALF_TO_ODD here:

scaled += remainder.Sign() ? 1 : -1;

This should be scaled += remainder.Sign() >= 0 ? 1 : -1; similar to what the HALF_TO_EVEN mode does.

Here is the effect of the bug:

>>> pc.round(Decimal("-52.5"), 0, "half_to_odd")
<pyarrow.Decimal128Scalar: Decimal('-51.0')>   # should be -53.0

Component(s)

C++, Python

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions