Refactor arrow-cast decimal casting to unify the rescale logic used in Parquet variant casts #8689

liamzwbao · 2025-10-22T23:04:48Z

Which issue does this PR close?

Closes Refactor arrow-cast decimal casting to unify the rescale logic used in Parquet variant casts #8670.

Rationale for this change

We currently have two separate code paths that both handle decimal casting between different (precision, scale) pairs. Without unifying the logic, a fix in one place often needs to be duplicated in the other (e.g., #8579 fixed the arrow-cast
and #8552 fixed the parquet-variant-compute), which can easily lead to divergence when contributors lack full context. This PR consolidates the decimal rescale logic for both arrow-cast and parquet-variant-compute.

What changes are included in this PR?

Extract the shared array-unary logic from convert_to_smaller_scale_decimal and convert_to_bigger_or_equal_scale_decimal into apply_decimal_cast
Move the rescale-closure creation into make_upscaler and make_downscaler so that they can be used in parquet-compute-variant
rework rescale_decimal in parquet-compute-variant to use the new make_upscaler and make_downscaler utilities.

One challenge is incorporating the large-scale reduction path (aka the delta_scale cannot fit into I::MAX_PRECISION) into make_downscaler without hurting performance. Returning 0 directly is usually cheaper than applying a unary operation to return zero. Therefore, make_downscaler may return None, and it is the caller’s responsibility to handle this case appropriately based on the documented behavior.

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

No

liamzwbao

This is ready for review. Please take a look when you get chance, @scovich @alamb.

Also, this PR may need a regression test for parse_decimal in arrow-cast. Thanks!

liamzwbao · 2025-10-22T23:08:22Z

arrow-cast/src/cast/decimal.rs

 }

-pub(crate) fn cast_decimal_to_decimal_error<I, O>(
+fn cast_decimal_to_decimal_error<I, O>(


downgrade the visibility since it's only used in this file

liamzwbao · 2025-10-22T23:31:37Z

arrow-cast/src/cast/decimal.rs

+    let is_infallible_cast = (input_precision as i8) + delta_scale <= (output_precision as i8);
+    let f_infallible = is_infallible_cast
+        .then_some(move |x| O::Native::from_decimal(x).unwrap().mul_wrapping(mul));
+    Some((f, f_infallible))


Chose to return f_infallible instead of is_infallible_cast because, unlike make_downscaler, we cannot derive an infallible closure from f. So to keep the interface consistent, I applied the same approach to make_downscaler to return (f, f_infallible) as well.

liamzwbao · 2025-10-22T23:36:24Z

arrow-cast/src/cast/decimal.rs


-    Ok(if is_infallible_cast {
-        // make sure we don't perform calculations that don't make sense w/o validation
-        validate_decimal_precision_and_scale::<O>(output_precision, output_scale)?;


Removed this check because I feel like it is redundant, the validation is already handled by array.with_precision_and_scale(p, s) later. Plus, if we were to keep this validation, it should be applied consistently across all three branches to avoid unnecessary computation.

now that this function is pub it means it can be called from anywhere (including outside this crate/repo) so I think more error handling is actually required

If we add it, it will be inside apply_decimal_cast, so it remains an internal implementation detail and will not cross the code path with the public API

liamzwbao · 2025-10-23T00:27:15Z

Benchmarked locally on an M3 Max Mac, and the results show no performance improvement or regression against the main branch.

command used:

git checkout a7572eb6
cargo bench -p arrow-cast --bench parse_decimal -- --save-baseline main

git checkout issue-8670-decimal-cast-refactor
cargo bench -p arrow-cast --bench parse_decimal -- --baseline main

alamb · 2025-10-23T17:16:49Z

🤖 ./gh_compare_arrow.sh Benchmark Script Running
Linux aal-dev 6.14.0-1017-gcp #18~24.04.1-Ubuntu SMP Tue Sep 23 17:51:44 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing issue-8670-decimal-cast-refactor (e32483c) to a7572eb diff
BENCH_NAME=parse_decimal
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench parse_decimal
BENCH_FILTER=
BENCH_BRANCH_NAME=issue-8670-decimal-cast-refactor
Results will be posted here when complete

alamb · 2025-10-23T17:25:25Z

🤖: Benchmark completed

Details

group                      issue-8670-decimal-cast-refactor       main
-----                      --------------------------------       ----
-.123                      1.00     20.8±0.06ns        ? ?/sec    1.00     20.8±0.12ns        ? ?/sec
-00.1                      1.00     31.5±0.05ns        ? ?/sec    1.00     31.5±0.22ns        ? ?/sec
-12.                       1.00     36.5±0.04ns        ? ?/sec    1.00     36.5±0.07ns        ? ?/sec
-123                       1.00     36.2±0.09ns        ? ?/sec    1.00     36.2±0.06ns        ? ?/sec
-123.                      1.00     38.9±0.05ns        ? ?/sec    1.00     38.9±0.10ns        ? ?/sec
-123.1                     1.00     37.6±0.07ns        ? ?/sec    1.00     37.6±0.06ns        ? ?/sec
-123.123                   1.00     28.8±0.06ns        ? ?/sec    1.00     28.8±0.04ns        ? ?/sec
-123.1234                  1.00     30.2±0.21ns        ? ?/sec    1.00     30.2±0.04ns        ? ?/sec
-12345678912345678.1234    1.00     67.7±0.11ns        ? ?/sec    1.00     67.7±0.33ns        ? ?/sec
-99999999999999999.999     1.00     66.1±0.14ns        ? ?/sec    1.00     66.1±0.36ns        ? ?/sec
.123                       1.00     19.7±0.02ns        ? ?/sec    1.00     19.7±0.11ns        ? ?/sec
0.0000123                  1.00     24.7±0.05ns        ? ?/sec    1.00     24.7±0.02ns        ? ?/sec
00.1                       1.00     30.4±0.04ns        ? ?/sec    1.00     30.4±0.03ns        ? ?/sec
12.                        1.00     35.2±0.16ns        ? ?/sec    1.00     35.1±0.06ns        ? ?/sec
123                        1.00     34.9±0.05ns        ? ?/sec    1.00     34.9±0.05ns        ? ?/sec
123.                       1.00     37.8±0.03ns        ? ?/sec    1.00     37.8±0.26ns        ? ?/sec
123.1                      1.00     36.5±0.04ns        ? ?/sec    1.00     36.5±0.04ns        ? ?/sec
123.123                    1.00     27.4±0.06ns        ? ?/sec    1.00     27.4±0.03ns        ? ?/sec
123.1234                   1.00     29.0±0.05ns        ? ?/sec    1.00     29.0±0.14ns        ? ?/sec
12345678912345678.1234     1.00     65.9±0.23ns        ? ?/sec    1.00     65.9±0.39ns        ? ?/sec
99999999999999999.999      1.00     64.2±0.14ns        ? ?/sec    1.00     64.3±0.40ns        ? ?/sec

alamb

Thank you @liamzwbao -- I ran benchmarks and agree with your assesment that this PR does not change the performance

However, I am somewhat concerned with the APIs that are proposed to make pub -- they implementations seem to rely on the caller correctly comparing / calling make_upscaler / make_downscaler which seems error prone to me (and the code does not really validate any of the inputs)

What if you left make_upscaler / make_downscaler private, and instead added a single new API like this, which validated the input and then called make_upscaler / make_downscaler appropriately?

pub fn make_scaler<I: DecimalType, O: DecimalType>(
    input_precision: u8,
    input_scale: i8,
    output_precision: u8,
    output_scale: i8,
) -> Result<Option<(
    impl Fn(I::Native) -> Option<O::Native>,
    Option<impl Fn(I::Native) -> O::Native>,
)>>
{
}

alamb · 2025-10-23T17:31:37Z

arrow-cast/src/cast/decimal.rs

+    // then an increase of scale by 3 will have the following effect on the representation:
+    // [xxxxx] -> [xxxxx000], so for the cast to be infallible, the output type
+    // needs to provide at least 8 digits precision
+    let is_infallible_cast = (input_precision as i8) + delta_scale <= (output_precision as i8);


I see the old code did this too, but it seems like the cast as i8 could porentially convert a number larger than 128 to a negative number : -- maybe that is ok

Yeah, this becomes an issue once we expose the API. Normally it isn’t a concern, since the precision is guaranteed to stay under 128. Let me redesign the public interface

alamb · 2025-10-23T17:33:56Z

arrow-cast/src/cast/decimal.rs

+/// Construct closures to upscale decimals from `(input_precision, input_scale)` to
+/// `(output_precision, output_scale)`.


It was not immediately clear to me what the two closures where (I think it is fallable and infallable)

So how about making that clearer in the docs like

Suggested change

/// Construct closures to upscale decimals from `(input_precision, input_scale)` to

/// `(output_precision, output_scale)`.

/// Construct closures to upscale decimals from `(input_precision, input_scale)` to

/// `(output_precision, output_scale)`.

///

/// Returns `(infallable_fn, fallable_fn)` where:

/// * `infallable_fn` will panic where the requested cast can not be performed

/// * `fallable_fn` will return None when the requested cast can not be performaned

I added docs for the return types. Specifically, f_infallible is guaranteed to be valid when provided; otherwise it will be None, and the caller should fall back to f_fallible for casting.

alamb · 2025-10-23T17:34:46Z

arrow-cast/src/cast/decimal.rs

+    Some((f, f_infallible))
+}
+
+/// Construct closures to downscale decimals from `(input_precision, input_scale)` to


same comment above

alamb · 2025-10-23T17:35:55Z

arrow-cast/src/cast/decimal.rs

+    I::Native: DecimalCast + ArrowNativeTypeOp,
+    O::Native: DecimalCast + ArrowNativeTypeOp,
+{
+    let delta_scale = input_scale - output_scale;


how do we ensure delta_scale is not negative? Given that this method is now pub it seems like we maybe need to do error checking on the scales more proactively

It can absolutely be negative. It corresponds to removing significant digits from the answer. The code is specifically designed to handle that (including cases where the final scale is negative, e.g. 123 with scale -3 is interpreted as 123000).

clearly I don't understand this code well enough

ah right, since this is now public, callers may not be aware of it. How about moving rescale_decimal into arrow_cast and making that function public to reduce the risk? It does not make much sense to expose this function on its own.

alamb · 2025-10-23T17:36:48Z

arrow-cast/src/cast/decimal.rs


-    Ok(if is_infallible_cast {
-        // make sure we don't perform calculations that don't make sense w/o validation
-        validate_decimal_precision_and_scale::<O>(output_precision, output_scale)?;


now that this function is pub it means it can be called from anywhere (including outside this crate/repo) so I think more error handling is actually required

liamzwbao

Hi @alamb, I attempted to unify the API, but the return types of make_upscaler and make_downscaler are closures with incompatible types unless we box them. Because of that, I decided to move rescale_decimal from parquet-variant-compute into arrow-cast and expose it for use in the variant conversion.

I’ll add some tests for the new rescale_decimal API next. WDYT?

liamzwbao added 6 commits October 20, 2025 19:42

Optimize

3f792b4

Extract apply_decimal_cast

a5fcc37

Rework convert scale fn

9a88258

Move fns and add docs

9b9800f

Use scaler fns in variant decimal rescaling

04505a0

Merge branch 'main' into issue-8670-decimal-cast-refactor

e32483c

github-actions bot added arrow Changes to the arrow crate parquet-variant parquet-variant* crates labels Oct 22, 2025

liamzwbao commented Oct 22, 2025

View reviewed changes

liamzwbao marked this pull request as ready for review October 22, 2025 23:38

alamb reviewed Oct 23, 2025

View reviewed changes

Move rescale_decimal to arrow-cast

5900bd4

liamzwbao force-pushed the issue-8670-decimal-cast-refactor branch from 4fe7d19 to 252eee7 Compare October 24, 2025 01:53

liamzwbao commented Oct 24, 2025

View reviewed changes

Rename f_fallible and add docs

49e72cd

liamzwbao force-pushed the issue-8670-decimal-cast-refactor branch from 252eee7 to 49e72cd Compare October 24, 2025 02:01

		/// Construct closures to upscale decimals from `(input_precision, input_scale)` to
		/// `(output_precision, output_scale)`.

Refactor arrow-cast decimal casting to unify the rescale logic used in Parquet variant casts #8689

Are you sure you want to change the base?

Refactor arrow-cast decimal casting to unify the rescale logic used in Parquet variant casts #8689

Uh oh!

Conversation

liamzwbao commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

liamzwbao left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamzwbao Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamzwbao commented Oct 23, 2025

Uh oh!

alamb commented Oct 23, 2025

Uh oh!

alamb commented Oct 23, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamzwbao left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

liamzwbao commented Oct 22, 2025 •

edited

Loading

liamzwbao left a comment •

edited

Loading

liamzwbao Oct 23, 2025 •

edited

Loading