dmsa-or opened a new pull request, #50642: URL: https://github.com/apache/arrow/pull/50642
### 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
