Jefffrey commented on code in PR #10509:
URL: https://github.com/apache/arrow-rs/pull/10509#discussion_r4091520357
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -88,7 +96,102 @@ 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. 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:
why do we fallback to parsing through an f64?
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -88,7 +96,102 @@ 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. 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 {
Review Comment:
```suggestion
fn decimal_to_f64_rounded_once<D: DecimalType>(x: D::Native, scale: i8,
unscaled: f64) -> f64 {
```
try to maintain as i8 where possible to avoid needing to check it fits
(caller should verify this)
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -88,7 +96,102 @@ 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. 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.
+///
+/// The scale is the same for every value, so the divisor is computed once 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.
Review Comment:
so this is forcing all negative scales into the slower parsing path? that
doesnt seem ideal
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -88,7 +95,196 @@ where
D: DecimalType,
F: Fn(D::Native) -> f64,
{
- f(x) / 10_f64.powi(scale)
+ let unscaled = f(x);
+ // Fast path: below 2^53 the integer -> f64 conversion is exact, and
10^|scale|
+ // is exactly representable up to 22, so this rounds exactly once and
gives the
+ // double nearest to the decimal value. A negative scale has to multiply:
+ // `10^scale` is inexact there, while `10^-scale` is the exact power of
ten.
+ if (-22..=22).contains(&scale) && unscaled.abs() < F64_EXACT_INT_LIMIT {
+ return if scale >= 0 {
+ unscaled / 10_f64.powi(scale)
+ } else {
+ unscaled * 10_f64.powi(-scale)
+ };
+ }
+ decimal_to_f64_rounded_once::<D>(x, scale, unscaled)
+}
+
+/// The out-of-line half of [`single_decimal_to_float_lossy`], kept out of the
hot
+/// loop so the common case stays a comparison and a division.
+///
+/// `unscaled` and/or the power of ten are inexact here, so combining them
rounds
+/// twice and can land on the wrong double. Round once instead, via a correctly
+/// rounded decimal-string parse. The precision passed to `format_decimal` only
+/// bounds how many digits are kept, and values are not guaranteed to fit the
+/// declared precision, so it must not truncate here.
+#[cold]
+#[inline(never)]
+fn decimal_to_f64_rounded_once<D: DecimalType>(x: D::Native, scale: i32,
unscaled: f64) -> f64 {
+ D::format_decimal(x, u8::MAX, scale as i8)
+ .parse::<f64>()
+ .unwrap_or_else(|_| unscaled / 10_f64.powi(scale))
+}
+
+/// Lossy conversion from decimal to `f32`.
+///
+/// Returns the `f32` nearest to the decimal's exact value, rounding once.
+///
+/// Narrowing through [`single_decimal_to_float_lossy`] and then to `f32`
rounds
+/// twice, and the two steps disagree with a single rounding: a decimal just
+/// above an `f32` midpoint can collapse onto that midpoint in `f64`, and
+/// round-half-even then sends it the wrong way. So this narrows directly
rather
+/// than reusing the `f64` conversion.
+#[inline(always)]
+pub fn single_decimal_to_f32_lossy<D, F>(f: &F, x: D::Native, scale: i32) ->
f32
+where
+ D: DecimalType,
+ F: Fn(D::Native) -> f64,
+{
+ let unscaled = f(x);
+ // `10^k = 2^k * 5^k`, and `5^10` is the largest power of five that fits
the
+ // 24-bit significand, so the exactly representable powers of ten stop at
+ // `k = 10` -- much earlier than the `k = 22` that `f64` allows.
+ if (-10..=10).contains(&scale) && unscaled.abs() < F32_EXACT_INT_LIMIT {
+ return if scale >= 0 {
+ unscaled as f32 / 10_f32.powi(scale)
+ } else {
+ unscaled as f32 * 10_f32.powi(-scale)
+ };
+ }
+ decimal_to_f32_rounded_once::<D>(x, scale, unscaled)
+}
+
+/// The out-of-line half of [`single_decimal_to_f32_lossy`]; see
+/// [`decimal_to_f64_rounded_once`].
+#[cold]
+#[inline(never)]
+fn decimal_to_f32_rounded_once<D: DecimalType>(x: D::Native, scale: i32,
unscaled: f64) -> f32 {
Review Comment:
i suppose in a followup PR we can explore doing it their way to avoid
parsing through a string
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -88,7 +96,102 @@ 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. 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.
+///
+/// The scale is the same for every value, so the divisor is computed once here
+/// rather than inside the loop.
Review Comment:
```suggestion
```
this doesnt seem necessary to explicitly state, especially as its an
internal detail
##########
arrow-cast/src/cast/mod.rs:
##########
Review Comment:
will we target f16 in a followup?
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -88,7 +96,102 @@ 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. 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.
+///
+/// The scale is the same for every value, so the divisor is computed once 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);
+ let values = 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)
+ }
+ });
+ Ok(Arc::new(values))
+}
+
+/// Casts a decimal array to `Float32`, rounding each value once. Same shape as
+/// [`cast_decimal_to_f64`] with the bounds an `f32` allows: `10^k` is exact
only up
+/// to `k = 10`, since `5^10` is the largest power of five that fits the 24-bit
Review Comment:
why are we talking about powers of 5 here?
--
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]
