Copilot commented on code in PR #10509:
URL: https://github.com/apache/arrow-rs/pull/10509#discussion_r3990541045
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -88,7 +95,115 @@ where
D: DecimalType,
F: Fn(D::Native) -> f64,
{
- f(x) / 10_f64.powi(scale)
+ let unscaled = f(x);
+ // Both operands are exact in this range, so the division rounds once.
+ if (0..=22).contains(&scale) && unscaled.abs() < F64_EXACT_INT_LIMIT {
+ return unscaled / 10_f64.powi(scale);
+ }
+ decimal_to_f64_rounded_once::<D>(x, scale, unscaled)
+}
+
+/// Rounds once for the values the division cannot convert exactly, by parsing
the
+/// decimal's own text. `u8::MAX` because `format_decimal` truncates to the
+/// precision it is given, and values are not validated against the precision
they
+/// declare. A scale that does not fit the `i8` `format_decimal` takes falls
back to
+/// the division.
Review Comment:
This rationale is inconsistent with the current
`DecimalType::format_decimal` implementation, which ignores its precision
argument and always formats the full value for API compatibility. Update the
comment so it does not claim that passing `u8::MAX` prevents truncation.
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -88,7 +95,115 @@ where
D: DecimalType,
F: Fn(D::Native) -> f64,
{
- f(x) / 10_f64.powi(scale)
+ let unscaled = f(x);
+ // Both operands are exact in this range, so the division rounds once.
+ if (0..=22).contains(&scale) && unscaled.abs() < F64_EXACT_INT_LIMIT {
+ return unscaled / 10_f64.powi(scale);
+ }
+ decimal_to_f64_rounded_once::<D>(x, scale, unscaled)
+}
+
+/// Rounds once for the values the division cannot convert exactly, by parsing
the
+/// decimal's own text. `u8::MAX` because `format_decimal` truncates to the
+/// precision it is given, and values are not validated against the precision
they
+/// declare. A scale that does not fit the `i8` `format_decimal` takes falls
back to
+/// the division.
+#[cold]
+#[inline(never)]
+fn decimal_to_f64_rounded_once<D: DecimalType>(x: D::Native, scale: i32,
unscaled: f64) -> f64 {
+ i8::try_from(scale)
+ .ok()
+ .and_then(|scale| D::format_decimal(x, u8::MAX,
scale).parse::<f64>().ok())
+ .unwrap_or_else(|| unscaled / 10_f64.powi(scale))
+}
+
+/// As [`decimal_to_f64_rounded_once`], but narrowing to `f32` in one step.
+/// Rounding to `f64` first and then to `f32` rounds twice: a decimal just
above an
+/// `f32` midpoint can collapse onto that midpoint in `f64`, and
round-half-even
+/// then sends it the wrong way.
+#[cold]
+#[inline(never)]
+fn decimal_to_f32_rounded_once<D: DecimalType>(x: D::Native, scale: i32,
unscaled: f64) -> f32 {
+ i8::try_from(scale)
+ .ok()
+ .and_then(|scale| D::format_decimal(x, u8::MAX,
scale).parse::<f32>().ok())
+ .unwrap_or_else(|| decimal_to_f64_rounded_once::<D>(x, scale,
unscaled) as f32)
+}
+
+/// Casts a decimal array to `Float64`, rounding each value once.
+///
+/// Scale and precision are the same for every value, so the divisor and the
loop
+/// to run are picked here rather than inside the loop.
+fn cast_decimal_to_f64<D, F>(
+ array: &dyn Array,
+ as_float: &F,
+ scale: i32,
+) -> Result<ArrayRef, ArrowError>
+where
+ D: DecimalType + ArrowPrimitiveType,
+ F: Fn(D::Native) -> f64,
+{
+ let array = array.as_primitive::<D>();
+ // `10^scale` is only exactly representable in this range. A negative scale
+ // would have to multiply by `10^-scale` to stay exact, which is not worth
a
+ // second loop: it was not correctly rounded before this change either.
+ if !(0..=22).contains(&scale) {
+ let values = array
+ .unary::<_, Float64Type>(|x| decimal_to_f64_rounded_once::<D>(x,
scale, as_float(x)));
+ return Ok(Arc::new(values));
+ }
+ let pow = 10_f64.powi(scale);
+ // `10^15 < 2^53`: at this precision every value that fits the precision it
+ // declares is exactly representable, so the per-value test can go -- the
same
+ // reading of the declared precision `make_upscaler` uses to skip
validation.
+ let values = if array.precision() <= 15 {
+ array.unary::<_, Float64Type>(|x| as_float(x) / pow)
+ } else {
+ array.unary::<_, Float64Type>(|x| {
+ let unscaled = as_float(x);
+ if unscaled.abs() < F64_EXACT_INT_LIMIT {
+ unscaled / pow
+ } else {
+ decimal_to_f64_rounded_once::<D>(x, scale, unscaled)
+ }
+ })
+ };
Review Comment:
`array.precision()` is metadata and does not validate the stored values;
`with_precision_and_scale` permits a wider integer in a lower-precision array.
This unconditional division can therefore reintroduce the double-rounding bug
(for example, a 20-digit value stored with precision 15), despite the new
fallback handling values beyond their declared precision. Keep the per-value
exact-integer check for all precisions, or validate the values before taking
this shortcut.
This issue also appears on line 194 of the same file.
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -88,7 +95,115 @@ where
D: DecimalType,
F: Fn(D::Native) -> f64,
{
- f(x) / 10_f64.powi(scale)
+ let unscaled = f(x);
+ // Both operands are exact in this range, so the division rounds once.
+ if (0..=22).contains(&scale) && unscaled.abs() < F64_EXACT_INT_LIMIT {
+ return unscaled / 10_f64.powi(scale);
+ }
+ decimal_to_f64_rounded_once::<D>(x, scale, unscaled)
+}
+
+/// Rounds once for the values the division cannot convert exactly, by parsing
the
+/// decimal's own text. `u8::MAX` because `format_decimal` truncates to the
+/// precision it is given, and values are not validated against the precision
they
+/// declare. A scale that does not fit the `i8` `format_decimal` takes falls
back to
+/// the division.
+#[cold]
+#[inline(never)]
+fn decimal_to_f64_rounded_once<D: DecimalType>(x: D::Native, scale: i32,
unscaled: f64) -> f64 {
+ i8::try_from(scale)
+ .ok()
+ .and_then(|scale| D::format_decimal(x, u8::MAX,
scale).parse::<f64>().ok())
+ .unwrap_or_else(|| unscaled / 10_f64.powi(scale))
+}
+
+/// As [`decimal_to_f64_rounded_once`], but narrowing to `f32` in one step.
+/// Rounding to `f64` first and then to `f32` rounds twice: a decimal just
above an
+/// `f32` midpoint can collapse onto that midpoint in `f64`, and
round-half-even
+/// then sends it the wrong way.
+#[cold]
+#[inline(never)]
+fn decimal_to_f32_rounded_once<D: DecimalType>(x: D::Native, scale: i32,
unscaled: f64) -> f32 {
+ i8::try_from(scale)
+ .ok()
+ .and_then(|scale| D::format_decimal(x, u8::MAX,
scale).parse::<f32>().ok())
+ .unwrap_or_else(|| decimal_to_f64_rounded_once::<D>(x, scale,
unscaled) as f32)
Review Comment:
The PR description advertises `single_decimal_to_f32_lossy` as a new public
API, but this helper is private and no such public symbol is defined or
re-exported. As written, downstream callers cannot use the promised API; please
either expose the intended function or correct the user-facing contract.
--
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]
