zeroshade commented on code in PR #43957:
URL: https://github.com/apache/arrow/pull/43957#discussion_r1767126099
##########
cpp/src/arrow/util/decimal.cc:
##########
@@ -156,7 +162,9 @@ struct DecimalRealConversion : public
BaseDecimalRealConversion {
// NOTE: if `precision` is the full precision then the algorithm will
// lose the last digit. If `precision` is almost the full precision,
// there can be an off-by-one error due to rounding.
- const int mul_step = std::max(1, kMaxPrecision - precision);
+ constexpr int is_dec32_or_dec64 =
+ DecimalType::kByteWidth <= BasicDecimal64::kByteWidth;
+ const int mul_step = std::max(1, kMaxPrecision - precision -
is_dec32_or_dec64);
Review Comment:
It's not really that unexpected honestly. The algorithm explicitly states
that it can cause off-by-1 rounding errors because of the interleaved
multiplication and rounded division that we're doing.
> // NOTE: if `precision` is the full precision then the algorithm will
// lose the last digit. If `precision` is almost the full precision,
// there can be an off-by-one error due to rounding.
In the case where we need this, we using a precision of 16 for Decimal64
which is "almost" the full precision of 18 and thus hitting the aforementioned
"off-by-one error due to rounding" that is mentioned in the existing comment.
By forcing the multiplication step to 1 instead of 2 in this case, we eliminate
the off-by-one error (though we'd likely still run into it if we were using 17
or 18 precision which would already be using the minimum multiplication step of
1).
That said, the issue that @bkietz filed to possibly replace a lot of this
logic with fewer steps (using multiplications by 5 etc.) could potentially
alleviate this issue directly also.
If we really don't like this and are okay with the off-by-one rounding error
mentioned in the comment, then I can adjust the tests to simply not test this
case which has the off-by-one error for Decimal64.
What do you think?
--
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]
