Skip to content

GH-50641: [C++][Compute] Fix correctness error in decimal round_binary kernel#50642

Open
dmsa-or wants to merge 1 commit into
apache:mainfrom
dmsa-or:fix_decimal_round_binary
Open

GH-50641: [C++][Compute] Fix correctness error in decimal round_binary kernel#50642
dmsa-or wants to merge 1 commit into
apache:mainfrom
dmsa-or:fix_decimal_round_binary

Conversation

@dmsa-or

@dmsa-or dmsa-or commented Jul 26, 2026

Copy link
Copy Markdown

Rationale for this change

There are major correctness bugs in the round_binary kernel for decimal input. #50641 describes them in more detail. Essentially, the midpoint half_pow10 of the rounding range is computed outside of the loop and assumes that ndigits == 0. The consequence is that for round-to-nearest modes, round_binary will frequently give wrong answers when ndigits != 0. Also as a result of assuming ndigits is always 0, the function does not attempt to round negative scale inputs.

There is also a correctness issue with the decimal implementation of the HALF_TO_ODD rounding mode, which applies both to the round and round_binary compute functions when the input is negative. In this case, the function does round to an odd, but rounds to the closest odd greater than the input rather than the nearest odd.

What changes are included in this PR?

The mentioned bugs are fixed. For round_binary, we make sure to compute pow and half_pow10 inside RoundBinary::Call where we know the row value of ndigits.

To fix the HALF_TO_ODD mode, we check whether the sign of the remainder is positive (1) instead of the truthy value of the sign (which turns out to be always true since remainder.Sign() is either 1 or -1. This matches the HALF_TO_EVEN implementation.

Added decimal round_binary unit tests to scalar_round_arithmetic_test.cc. They were absent up to this point which is why the bug wasn't caught previously. I did some refactoring and added a couple structs to make it easier to share test data between TestUnaryRoundArithmeticDecimal and the new TestBinaryRoundArithmeticDecimal. Those test cases use a constant ndigits for every row, but serve to verify that round_binary works as well as round if used in the same way. I also created an additional test TestBinaryRoundArithmeticDecimal.RoundNDigitsArray that covers most of the main edge cases with varying ndigits per row, since that is the unique functionality of the round_binary kernel vs. round.

Are these changes tested?

Yes. The new TestBinaryRoundArithmeticDecimal tests provide relatively extensive coverage of the round_binary kernel. I confirmed that they failed without the changes and passed with the fixes in place. There are a couple numbers included in the TestBinaryRoundArithmeticDecimal.RoundNDigitsArray test data that require the HALF_TO_ODD fix to be rounded correctly.

Besides that, I ran the main Arrow C++ unit tests and Arrow compute tests to ensure they pass.

Are there any user-facing changes?

No user-facing changes.

This PR contains a "Critical Fix". This PR fixes bugs in the decimal round_binary kernel and decimal HALF_TO_ODD rounding mode which cause incorrect data to be produced, even for the most common of inputs.

@dmsa-or
dmsa-or requested a review from pitrou as a code owner July 26, 2026 07:33
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50641 has been automatically assigned in GitHub to PR creator.

@Reranko05 Reranko05 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you run python -m pre_commit run clang-format --files cpp/src/arrow/compute/kernels/scalar_round.cc cpp/src/arrow/compute/kernels/scalar_round_arithmetic_test.cc to fix the formatting issues?

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants