wombatu-kun commented on code in PR #16627:
URL: https://github.com/apache/iceberg/pull/16627#discussion_r3334943991
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/ArrowVectorAccessors.java:
##########
@@ -99,7 +99,9 @@ public BigDecimal ofLong(long value, int precision, int
scale) {
@Override
public BigDecimal ofBigDecimal(BigDecimal value, int precision, int scale)
{
- return BigDecimal.valueOf(value.unscaledValue().longValue(), scale);
+ // Return the value unchanged: it already has the correct unscaled value
and scale. The
+ // unscaled value can exceed the range of a long, so it must not be
narrowed to one.
Review Comment:
Yes. At every `ofBigDecimal` call site the value is built with exactly this
`scale`: the fixed-size-binary accessor uses `new BigDecimal(bigInteger,
scale)`, and the two `DecimalUtility.getBigDecimalFrom...` paths pass the same
`scale`, so `value.scale()` already equals the `scale` argument. The previous
`BigDecimal.valueOf(unscaledValue().longValue(), scale)` re-applied that
identical scale, so it never changed the scale - its only effect was narrowing
the unscaled value to a long, which is exactly the truncation this fixes.
`BigDecimal` carries no declared precision (its `precision()` is
value-dependent) and neither the old nor the new code sets one, so precision is
unaffected. This matches the Spark factory, whose `ofBigDecimal` calls
`Decimal.apply(value, precision, scale)` and likewise keeps the full unscaled
value.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
