fix(arrow-cast): round decimal to float conversions once - #10509
fix(arrow-cast): round decimal to float conversions once#10509davidlghellin wants to merge 5 commits into
Conversation
the nearest one to the decimal's exact value, because both paths rounded twice. For Float64, `unscaled / 10f64.powi(scale)` rounds the unscaled integer when it exceeds 2^53 and uses a `powi` result that is 1 ULP off for some scales. Keep that arithmetic as a fast path only where both operands are exact (-22 <= scale <= 22 and |unscaled| < 2^53), multiplying by 10^-scale when the scale is negative since only that is an exact power of ten, and otherwise round once via a correctly rounded decimal-string parse. For Float32, computing in f64 and narrowing with `as f32` rounds twice again, independently of the above: a decimal just above an f32 midpoint can collapse onto that midpoint in f64, and round-half-even then sends it the wrong way. Add `single_decimal_to_f32_lossy`, which narrows in one step; its exact range is smaller, since 10^k is exact in an f32 only up to k = 10. Float16 is unchanged and now consumes the corrected f64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
run benchmark cast_kernels |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing fix-decimal-to-float-double-rounding (270bd2a) to fd8ead5 (merge-base) diff Run configurationrun benchmark cast_kernels
env:
BENCH_FILTER: "decimal.*to.*float"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Thanks for running it! The slowdown is real and I can reproduce it locally. Looking into it now, |
|
Pushed the fix for the regression. The guard around the new string path was landing in the hot loop; it now decides once per array instead of once per value. |
|
run benchmark cast_kernels |
|
@Jefffrey I pushed the fix for the regression — could you run the benchmark again? |
|
run benchmark cast_kernels |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing fix-decimal-to-float-double-rounding (271f5fb) to ed92960 (merge-base) diff Run configurationrun benchmark cast_kernels
env:
BENCH_FILTER: "decimal.*to.*float"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
the comments explaining the logic are nice, but i feel this PR is quite overly verbose both in terms of comments and code organization that it makes it hard to follow
| // 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) |
There was a problem hiding this 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:
- from
cast_from_decimal(), callcast_decimal_array_to_f64() - if we pass this check, we call
cast_decimal_to_float()which callssingle_decimal_to_float_lossy()for each element - but
single_decimal_to_float_lossy()also checks the scale again and may calldecimal_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?
| /// [`decimal_to_f64_rounded_once`]. | ||
| #[cold] | ||
| #[inline(never)] | ||
| fn decimal_to_f32_rounded_once<D: DecimalType>(x: D::Native, scale: i32, unscaled: f64) -> f32 { |
There was a problem hiding this comment.
it seems duckdb also did a fix for this recently: duckdb/duckdb#22874
notably they dont parse through a string, but seemingly manually parse the digits mathematically to avoid heap allocation
| /// 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 |
There was a problem hiding this comment.
| pub fn single_decimal_to_f32_lossy<D, F>(f: &F, x: D::Native, scale: i32) -> f32 | |
| fn single_decimal_to_f32_lossy<D, F>(f: &F, x: D::Native, scale: i32) -> f32 |
this doesn't need to be public
3 participants
Which issue does this PR close?
Rationale for this change
cast(decimal, Float64)andcast(decimal, Float32)could return a float that is not the nearestone to the decimal's exact value — see the issue for the full analysis and the failing values.
The short version is two independent double roundings.
single_decimal_to_float_lossycomputedf(x) / 10_f64.powi(scale), wheref(x)rounds the unscaled integer once it exceeds 2^53 and10_f64.powi(scale)is itself 1 ULP off for some scales aDECIMAL(38,s)can declare; dividing twowrongly rounded operands does not give the correctly rounded quotient. Separately, the
Float32target computed the
f64result and narrowed it withas f32, rounding a second time — a decimaljust above an
f32midpoint can collapse onto that midpoint inf64, and round-half-even then sendsit the wrong way, so fixing the
f64path alone leaves those cases unchanged.The expected result is the float nearest the decimal's exact value: what parsing the decimal's own
text gives, and what Java's
BigDecimal.doubleValue()/floatValue()give.What changes are included in this PR?
single_decimal_to_float_lossykeeps the existing arithmetic as a fast path, but only where bothoperands are exact:
-22 <= scale <= 22and|unscaled| < 2^53. A negative scale multiplies by10^-scalerather than dividing by10^scale, since only the former is an exact power of ten.Everything outside that range goes through a correctly rounded decimal-string parse.
single_decimal_to_f32_lossy, used by theFloat32target, narrowing in one step. Same shape,smaller exact range:
10^kis exact in anf32only up tok = 10(5^10is the largest powerof five that fits the 24-bit significand), and the integer bound is 2^24.
Float16is unchanged and now consumes the correctedf64. It is still not correctly rounded —half::f16::from_strisf32::from_str(..).map(f16::from_f32), so there is no correctly roundeddecimal →
f16conversion to compare against. Noted as a remaining limitation.Are these changes tested?
Yes, six tests in
arrow-cast/src/cast/mod.rs:Float64andFloat32against expected constants, each including control cases that alreadypassed before this change.
Decimal256, where thei256 -> f64step is least likely to leave the value intact.correctly rounded, and it needs no expected constants. It sweeps both sides of each fast-path
boundary: the 2^53 / 2^24 significand limits and the scale ±22 / ±10 limits on exact powers of ten.
or 76.
Also checked out of tree against randomized sweeps of
(unscaled, scale)pairs, comparing everyresult to an independently built decimal string, for
Decimal128to bothf32andf64: 0mismatches, where the same sweep failed on a large share of the wide cases before the change.
Are there any user-facing changes?
Yes. Casts from decimal to
Float32/Float64can now return a different — correctly rounded — valuefor wide decimals, and
single_decimal_to_f32_lossyis new public API.AI disclosure
This change was developed with AI assistance (Claude).