GH-50641: [C++][Compute] Fix correctness error in decimal round_binary kernel#50642
Open
dmsa-or wants to merge 1 commit into
Open
GH-50641: [C++][Compute] Fix correctness error in decimal round_binary kernel#50642dmsa-or wants to merge 1 commit into
dmsa-or wants to merge 1 commit into
Conversation
|
|
Reranko05
reviewed
Jul 26, 2026
Reranko05
left a comment
Contributor
There was a problem hiding this comment.
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?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
There are major correctness bugs in the
round_binarykernel for decimal input. #50641 describes them in more detail. Essentially, the midpointhalf_pow10of the rounding range is computed outside of the loop and assumes thatndigits== 0. The consequence is that for round-to-nearest modes,round_binarywill frequently give wrong answers whenndigits!= 0. Also as a result of assumingndigitsis 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
roundandround_binarycompute 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 computepowandhalf_pow10insideRoundBinary::Callwhere we know the row value ofndigits.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
truesinceremainder.Sign()is either 1 or -1. This matches the HALF_TO_EVEN implementation.Added decimal
round_binaryunit tests toscalar_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 constantndigitsfor every row, but serve to verify thatround_binaryworks as well asroundif used in the same way. I also created an additional testTestBinaryRoundArithmeticDecimal.RoundNDigitsArraythat covers most of the main edge cases with varyingndigitsper row, since that is the unique functionality of theround_binarykernel vs.round.Are these changes tested?
Yes. The new TestBinaryRoundArithmeticDecimal tests provide relatively extensive coverage of the
round_binarykernel. I confirmed that they failed without the changes and passed with the fixes in place. There are a couple numbers included in theTestBinaryRoundArithmeticDecimal.RoundNDigitsArraytest 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_binarykernel and decimal HALF_TO_ODD rounding mode which cause incorrect data to be produced, even for the most common of inputs.