Jefffrey commented on code in PR #10509:
URL: https://github.com/apache/arrow-rs/pull/10509#discussion_r3702319564
##########
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 {
+ D::format_decimal(x, u8::MAX, scale as i8)
+ .parse::<f32>()
+ .unwrap_or_else(|_| decimal_to_f64_rounded_once::<D>(x, scale,
unscaled) as f32)
+}
+
+/// Casts a whole decimal array to `Float64`, rounding each value once.
+///
+/// The scale and the declared precision are fixed for the whole array, so the
+/// power of ten and the shape of the loop are chosen once here rather than per
+/// value: a test inside the loop costs more than the conversion it guards.
+/// `cast_floating_point_to_decimal` hoists the same multiplier in the other
+/// direction.
+fn cast_decimal_array_to_f64<D, F>(
+ array: &dyn Array,
+ as_float: &F,
+ scale: i32,
+) -> Result<ArrayRef, ArrowError>
+where
+ D: DecimalType + ArrowPrimitiveType,
+ F: Fn(D::Native) -> f64,
+{
+ // Outside this range no power of ten is exactly representable, so every
value
+ // has to go through the string path.
+ if !(-22..=22).contains(&scale) {
+ return cast_decimal_to_float::<D, Float64Type, _>(array, |x| {
+ single_decimal_to_float_lossy::<D, F>(as_float, x, scale)
Review Comment:
i feel like we can simplify/inline a lot of this code, as there is a lot of
indirection going on here
for example im trying to read this but im getting a flow like:
1. from `cast_from_decimal()`, call `cast_decimal_array_to_f64()`
2. if we pass this check, we call `cast_decimal_to_float()` which calls
`single_decimal_to_float_lossy()` for each element
3. but `single_decimal_to_float_lossy()` also checks the scale again and may
call `decimal_to_f64_rounded_once()`
we can cut out `cast_decimal_to_float()` here since if we look at it, its a
very simple function that can be inlined. and perhaps find some way not to
check the scale again?
##########
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
Review Comment:
```suggestion
fn single_decimal_to_f32_lossy<D, F>(f: &F, x: D::Native, scale: i32) -> f32
```
this doesn't need to be public
##########
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:
it seems duckdb also did a fix for this recently:
https://github.com/duckdb/duckdb/pull/22874
notably they dont parse through a string, but seemingly manually parse the
digits mathematically to avoid heap allocation
--
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]
