mbutrovich commented on code in PR #12049:
URL: https://github.com/apache/arrow-rs/pull/12049#discussion_r4457490204
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -760,26 +859,226 @@ impl<D: DecimalType> FloatToDecimalError<D> {
}
}
-// `mul` is precomputed once for array casts. Precision must already be
validated.
-#[inline]
+// Precision and scale must already be validated.
+#[inline(always)]
fn float_to_decimal_checked<D>(
input: f64,
- mul: f64,
+ scale: FloatScale,
precision: u8,
) -> Result<D::Native, FloatToDecimalError<D>>
where
D: DecimalType,
D::Native: DecimalCast,
{
let value =
- D::Native::from_f64((mul *
input).round()).ok_or(FloatToDecimalError::NativeOverflow)?;
+ float_to_decimal_native::<D>(input,
scale).ok_or(FloatToDecimalError::NativeOverflow)?;
if D::is_valid_decimal_precision(value, precision) {
Ok(value)
} else {
Err(FloatToDecimalError::PrecisionOverflow(value))
}
}
+/// What [`float_to_decimal_native`] needs to know about a scale, computed
once per array.
+#[derive(Clone, Copy)]
+struct FloatScale {
+ scale: i8,
+ /// `10^scale` as an `f64`
+ mul: f64,
+}
+
+impl FloatScale {
+ #[inline]
+ fn new(scale: i8) -> Self {
+ Self {
+ scale,
+ mul: 10_f64.powi(scale as i32),
+ }
+ }
+}
+
+/// `input * 10^scale` rounded half away from zero, or `None` if `input` is
not finite or the
+/// result does not fit `D::Native`.
+///
+/// The product is computed in floating point while it is below 2^53. Above
that an `f64` no
+/// longer holds every integer, and the floating-point product is off by up to
half its ulp:
+/// `295161.0` at scale 15 would give `295160999999999967232`. There the
shortest decimal
+/// representation of `input` (the digits arrow displays it with) is scaled
exactly instead.
+#[inline(always)]
+fn float_to_decimal_native<D>(input: f64, scale: FloatScale) ->
Option<D::Native>
+where
+ D: DecimalType,
+ D::Native: DecimalCast,
+{
+ let p = scale.mul * input;
+ // false for NaN and infinities
+ if p.abs() < 9_007_199_254_740_992.0 {
+ // |p.round()| <= 2^53: exact as i64, and cheaper than converting f64
to i128
+ return D::Native::from_decimal(p.round() as i64);
+ }
+ shortest_decimal_scaled(input,
scale.scale).and_then(D::Native::from_decimal)
+}
Review Comment:
Should the cast round the double's exact binary value at every magnitude, as
I asked [on
#12048](https://github.com/apache/arrow-rs/issues/12048#issuecomment-6609413607),
instead of switching to the shortest representation at 2^53?
With the switch, the result of a cast depends on the target scale. On the
head commit, `1.005` cast to `Decimal128(38, 2)` gives `1.00`, while casting to
`Decimal128(38, 16)` and then to `Decimal128(38, 2)` gives `1.01`. On `main`
both give `1.00`.
The branch below 2^53 also isn't correctly rounded under either definition.
On the head commit, `float_to_decimal_opt::<Decimal128Type>(26.657349498176934,
38, 14)` returns `2665734949817694`, and both definitions give
`2665734949817693`. [My comment on
#12048](https://github.com/apache/arrow-rs/issues/12048#issuecomment-6609413607)
has the sample this came from.
Rounding the exact value needs no formatting. With `x = m * 2^e` and `m <
2^53`, the result is `m * 10^scale * 2^e` rounded half away from zero, which is
one integer multiply and a shift. This sketch matches Rust's exact `{:.N}`
formatting on 300,000 random doubles at scales 0 to 21 (except exact ties,
where Rust rounds to even), and is exact for integers:
```rust
fn exact(x: f64, s: u32) -> Option<i128> {
if !x.is_finite() { return None; }
let bits = x.to_bits();
let biased = ((bits >> 52) & 0x7ff) as i32;
let frac = bits & ((1u64 << 52) - 1);
let (m, e) = if biased == 0 { (frac, -1074) } else { (frac | (1 << 52),
biased - 1075) };
// m < 2^53 and 10^21 < 2^70, so the product fits
let p = m as i128 * 10i128.pow(s);
let mag = if e >= 0 {
if p != 0 && (p.leading_zeros() as i32) <= e { return None; }
p << e
} else {
let k = -e as u32;
if k > 124 { 0 } else { (p + (1i128 << (k - 1))) >> k }
};
Some(if bits >> 63 == 1 { -mag } else { mag })
}
```
Wider scales would do the same in `i256`. It also handles `Float32` input
without a separate path, because widening to `f64` is exact. With the shortest
representation, `0.1f32` at `Decimal128(38, 20)` gives `0.10000000149011612000`
on the head commit, which is not the digits arrow displays (`0.1`), so the doc
comment's description doesn't hold for `Float32`.
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -760,26 +859,226 @@ impl<D: DecimalType> FloatToDecimalError<D> {
}
}
-// `mul` is precomputed once for array casts. Precision must already be
validated.
-#[inline]
+// Precision and scale must already be validated.
+#[inline(always)]
fn float_to_decimal_checked<D>(
input: f64,
- mul: f64,
+ scale: FloatScale,
precision: u8,
) -> Result<D::Native, FloatToDecimalError<D>>
where
D: DecimalType,
D::Native: DecimalCast,
{
let value =
- D::Native::from_f64((mul *
input).round()).ok_or(FloatToDecimalError::NativeOverflow)?;
+ float_to_decimal_native::<D>(input,
scale).ok_or(FloatToDecimalError::NativeOverflow)?;
if D::is_valid_decimal_precision(value, precision) {
Ok(value)
} else {
Err(FloatToDecimalError::PrecisionOverflow(value))
}
}
+/// What [`float_to_decimal_native`] needs to know about a scale, computed
once per array.
+#[derive(Clone, Copy)]
+struct FloatScale {
+ scale: i8,
+ /// `10^scale` as an `f64`
+ mul: f64,
+}
+
+impl FloatScale {
+ #[inline]
+ fn new(scale: i8) -> Self {
+ Self {
+ scale,
+ mul: 10_f64.powi(scale as i32),
+ }
+ }
+}
+
+/// `input * 10^scale` rounded half away from zero, or `None` if `input` is
not finite or the
+/// result does not fit `D::Native`.
+///
+/// The product is computed in floating point while it is below 2^53. Above
that an `f64` no
+/// longer holds every integer, and the floating-point product is off by up to
half its ulp:
+/// `295161.0` at scale 15 would give `295160999999999967232`. There the
shortest decimal
+/// representation of `input` (the digits arrow displays it with) is scaled
exactly instead.
+#[inline(always)]
+fn float_to_decimal_native<D>(input: f64, scale: FloatScale) ->
Option<D::Native>
+where
+ D: DecimalType,
+ D::Native: DecimalCast,
+{
+ let p = scale.mul * input;
+ // false for NaN and infinities
+ if p.abs() < 9_007_199_254_740_992.0 {
+ // |p.round()| <= 2^53: exact as i64, and cheaper than converting f64
to i128
+ return D::Native::from_decimal(p.round() as i64);
+ }
+ shortest_decimal_scaled(input,
scale.scale).and_then(D::Native::from_decimal)
+}
+
+/// A decimal native from an `i64` without a range check: the array cast's
first pass converts
+/// every slot, valid or not, and masks the invalid ones.
+pub(crate) trait NativeFromI64 {
+ fn from_i64_wrapping(v: i64) -> Self;
+}
+
+impl NativeFromI64 for i32 {
+ fn from_i64_wrapping(v: i64) -> Self {
+ v as i32
+ }
+}
+
+impl NativeFromI64 for i64 {
+ fn from_i64_wrapping(v: i64) -> Self {
+ v
+ }
+}
+
+impl NativeFromI64 for i128 {
+ fn from_i64_wrapping(v: i64) -> Self {
+ v as i128
+ }
+}
+
+impl NativeFromI64 for i256 {
+ fn from_i64_wrapping(v: i64) -> Self {
+ i256::from_i128(v as i128)
+ }
+}
+
+/// Pass 1 of the array cast: `kernel` on every input and its product with
10^scale
+/// (`x * mul`), 64 at a time, into `values` and one word per 64 inputs of the
`valid` and
+/// `exact` bitmaps; the kernel returns the value, `valid` and `not exact`. A
separate function
+/// over plain slices, so that the loop holds its invariants and bit
accumulators in registers;
+/// kernels without branches vectorize.
+#[inline(never)]
+fn fast_pass<T, N, K>(
Review Comment:
Could the two-pass array kernel (`fast_pass`, `NativeFromI64`, the `valid`
and `exact` bitmaps) move to its own PR? It changes the common casts below 2^53
that the title doesn't cover, and it makes this fix hard to review on its own.
A separate PR would let us review its speedups and its `from_i64_wrapping` and
`.expect("a failing row")` paths separately, with the new benchmark cases in
their own commit first so the benchmark bot can compare.
--
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]
