refactor: use arity helper for Int to Decimal128 reinterpretation

[0lai0]

Which issue does this PR close?

Part of #5091.

This is the first of a few small PRs splitting up #5091.
It covers the twoInt to Decimal128 reinterpretation sites
the remaining items (numeric.rs,array_insert.rs, temporal.rs, pow.rs, covariance.rs) will follow in separate PRs

Rationale for this change

Two sites hand-roll a per-row loop to turn an integer array into a Decimal128 array:

• native/core/src/execution/columnar_to_row.rs (maybe_cast_to_schema_type):
  Int32/Int64 to Decimal128, via iter().map().collect().
• native/spark-expr/src/math_funcs/internal/make_decimal.rs
  (spark_make_decimal): Int64 → Decimal128, via a Decimal128Builder loop.

What changes are included in this PR?

Replace the hand-rolled per-row loops with arity helpers:

• columnar_to_row.rs (maybe_cast_to_schema_type): Int32/Int64 → Decimal128 via arity::unary.
• make_decimal.rs (spark_make_decimal): Int64 → Decimal128 via an infallible unary widen followed by a single short-circuiting is_valid_decimal_precision scan for overflow, the same idiom already used by the sibling checkoverflow.rs. Non-ANSI nulls the first offending row, ANSI errors on it. (Earlier revisions used unary_opt / try_unary; switched to the scan shape after benchmarking, see below.)

How are these changes tested?

• cargo test -p datafusion-comet-spark-expr and cargo test -p datafusion-comet (all green).
• Added benches/make_decimal.rs (mirroring benches/unscaled_value.rs / benches/check_overflow.rs)
  so the choice of idiom is measurable. On 8192-row Int64 batches at Decimal128(18, 2)(Criterion median):

case	main (per-row loop)	unary + scan	speedup
no nulls	32.38 µs	5.90 µs	5.49x
sparse nulls	35.84 µs	6.87 µs	5.22x
dense nulls	35.44 µs	6.19 µs	5.72x
ansi no nulls	31.81 µs	5.90 µs	5.39x
ansi sparse nulls	34.68 µs	6.95 µs	4.99x
ansi dense nulls	35.39 µs	6.11 µs	5.79x

[andygrove]

Thanks for splitting #5091 into reviewable pieces, and for being careful with the ANSI error path. I checked out the head commit and ran things rather than reading the diff alone. Recording what I verified so it doesn't have to be re-done:

• cargo test -p datafusion-comet-spark-expr make_decimal (5 passed) and cargo test -p datafusion-comet columnar_to_row (18 passed).
• cargo fmt --all -- --check and cargo clippy -p datafusion-comet-spark-expr -p datafusion-comet --all-targets -- -D warnings are both clean.
• The ANSI error is preserved exactly. The ArrowError::ExternalError unwrap does what the comment claims. A downcast test confirms the payload is still a SparkError, and the message is unchanged: External error: [NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION] 123456 cannot be represented as Decimal(3, 0).... This was the most likely thing to break silently and it doesn't.
• The first-offender ordering your new test asserts is real. try_unary walks valid indices through BitIndexIterator in ascending order and short-circuits on the first Err.
• Null handling is right. In arrow 58.4.0, unary_opt and try_unary both invoke the closure only on valid rows. unary in columnar_to_row.rs runs on every slot, but the widening is infallible so garbage under a null is harmless and the input null buffer carries over.
• Sliced inputs are fine. All three kernels go through values()/nulls(), which are already offset-adjusted. A scratch test on full.slice(2, 3) gives the right values and nulls.
• Dropping downcast_ref + ok_or_else in favour of as_primitive is safe. The match is on array.data_type(), so the old error arm was unreachable and this is not a new panic path.

So the risky parts went right. I have four things below, the first being the substantive one.

One process note: no CI checks have run on this branch yet, so a committer will need to approve the workflow run before this can merge.

[andygrove]

A question on the helper choice here. Arrow's own docs on try_unary note it "is often significantly slower than unary" because LLVM cannot vectorize fallible closures, and unary_opt has the same per-row shape. Meanwhile checkoverflow.rs, in this same directory and solving the same overflow problem, already uses a different idiom: widen unconditionally, then a short-circuiting is_valid_decimal_precision scan, paying for null-masking or error construction only when an overflow actually exists.

I benchmarked the three shapes on 8192-row Int64 batches at Decimal128(18, 2), the widest precision DecimalAggregates produces for MakeDecimal (times are non-ANSI / ANSI):

shape	no nulls	sparse nulls	dense nulls
builder loop (today)	19.0 / 19.1 µs	22.7 / 22.7 µs	19.1 / 19.2 µs
this PR	14.3 / 14.3 µs	15.8 / 14.9 µs	10.7 / 9.8 µs
unary + scan	5.58 / 8.50 µs	5.44 / 7.92 µs	5.48 / 9.04 µs

Your version is a genuine 1.3x to 1.9x win over what is there now. The checkoverflow.rs idiom looks like another 1.7x to 2.6x on top of that. MakeDecimal sits on the decimal sum and avg path, which is why its sibling got optimized in the first place, so it is worth getting right here.

Would you be up for trying that shape and seeing whether you reproduce the gain? The scan still finds the first offending value, so your new ordering test should keep passing. If you would rather stay with unary_opt/try_unary, could you note why in the PR description so the choice is on the record?

Either way, would you mind adding a make_decimal bench? benches/unscaled_value.rs and benches/check_overflow.rs cover the neighbouring expressions and would be easy to mirror. That would make the numbers checkable rather than something a reviewer has to measure by hand.

[0lai0]

Thanks, @andygrove. I appreciate you pointing this out and guiding me in the right direction. I’ll be using the checkflow.rs approach and adding a benchmark to measure the results.

[0lai0]

make_decimal benchmark (8192 rows, target Decimal128(18, 2))

Time per call (Criterion median). Lower is better.

case	main (per-row loop)	unary_opt / try_unary	unary + scan	speedup (main → best)
no nulls	32.38 µs	14.24 µs	5.90 µs	5.49x
sparse nulls (~10%)	35.84 µs	16.40 µs	6.87 µs	5.22x
dense nulls (~50%)	35.44 µs	10.41 µs	6.19 µs	5.72x
ansi no nulls	31.81 µs	21.61 µs	5.90 µs	5.39x
ansi sparse nulls (~10%)	34.68 µs	21.00 µs	6.95 µs	4.99x
ansi dense nulls (~50%)	35.39 µs	12.84 µs	6.11 µs	5.79x

[0lai0]

make_decimal update in abde580
unary + scan update in 89873f8

[andygrove]

PrimitiveArray::try_unary is generic over the error type. It is only the free function arity::try_unary that pins it to ArrowError. If you call the method form, as you already do for unary_opt just below, the closure can return DataFusionError directly:

arr.try_unary::<_, Decimal128Type, DataFusionError>(|v| {
    let v = v as i128;
    validate_decimal_precision(v, precision, scale)
        .map(|()| v)
        .map_err(|_| {
            DataFusionError::External(Box::new(decimal_overflow_error(v, precision, scale)))
        })
})?

I compiled and ran this and it produces the identical error, same variant and same SparkError payload. It drops the arrow::error::ArrowError import, the map_err match, and the other => arm that cannot currently be reached. It also removes the chance that a later edit loses the unwrap and turns the ANSI failure into a generic ArrowError, which would break the NUMERIC_VALUE_OUT_OF_RANGE mapping. This may become moot if you take the scan approach in my other comment.

[andygrove]

long_to_decimal is still here for the scalar path, and the array path now inlines the same validate-and-convert rule twice more. That is three copies with nothing keeping them in sync. Could one helper cover all three? Something like:

#[inline]
fn to_unscaled(v: i64, precision: u8, scale: i8) -> Option<i128> {
    let v = v as i128;
    validate_decimal_precision(v, precision, scale).ok().map(|()| v)
}

Then the non-ANSI array path becomes arr.unary_opt::<_, Decimal128Type>(|v| to_unscaled(v, precision, scale)), the ANSI path becomes to_unscaled(...).ok_or_else(|| overflow_err(v, precision, scale)), and long_to_decimal builds on the same two pieces.

[0lai0]

The scan rewrite removed the duplication: the array path is now just one unary widen + one find predicate, so it no longer inlines the validate logic at all.
That leaves a single call site (the scalar long_to_decimal), where extracting a shared helper would only add indirection. So I'd prefer to leave it as is.

[andygrove]

This reads well, and the note about an Arrow cast rescaling the value is worth having in the code. I confirmed that dropping the downcast_ref + ok_or_else is safe, since the match is on array.data_type(), so nothing new can panic here.

The substance of the change is null-buffer handling, and test_convert_int32_to_decimal128 and test_convert_int64_to_decimal128 both use all-valid arrays, so that is the one thing the tests do not exercise. Could you add a null to those arrays? A sliced input would be good too, following test_map_data_conversion_sliced_maparray further down the file. I checked both cases by hand and they behave correctly, so this is just about locking the behaviour in.

[0lai0]

Thanks @andygrove for review.
PR updated and also revise the description

[andygrove]

Second round. Both items from the first pass are handled well. Moving to unary plus a short-circuiting scan is the right shape, and the new tests around null slots, boundaries, all-null, and negative overflow are thorough. Kicking off CI now, so that process note from last round is cleared.

I checked out 8da10109a in a clean worktree and ran things rather than reading the diff alone. Recording what I verified:

• cargo test -p datafusion-comet-spark-expr make_decimal (12 passed) and cargo test -p datafusion-comet columnar_to_row (20 passed, including the four new null and sliced cases).
• cargo fmt --all -- --check and cargo clippy -p datafusion-comet-spark-expr -p datafusion-comet --all-targets -- -D warnings are both clean.
• Splitting the precision check across two arrow entry points is safe. This was the risk I was worried about when you declined the shared helper. The scalar path calls validate_decimal_precision(v, precision, scale) and the array path calls Decimal128Type::is_valid_decimal_precision(v, precision). In arrow 58.4.0 both reduce to the same comparison against MIN_DECIMAL128_FOR_EACH_PRECISION and MAX_DECIMAL128_FOR_EACH_PRECISION, and scale only formats the error text. They cannot disagree today, so your argument for leaving it alone holds up.
• The claim to cover all Int to Decimal128 sites holds. The only other Decimal128Builder and collect::<Decimal128Array>() occurrences in native/core and native/spark-expr are in test code.

Three things below.

The benchmark omits the overflow shapes, and they are where this wins hardest

I added overflow cases against Decimal128(3, 0) locally and measured this branch against its merge-base, 8192 rows, Criterion medians:

case	merge-base	this PR
overflow all rows, non-ANSI	1.0440 ms	17.13 µs
overflow last row, non-ANSI	32.26 µs	12.70 µs
overflow last row, ANSI	31.52 µs	7.40 µs
overflow all rows, ANSI	177.6 ns	2.00 µs

Could you add these cases to benches/make_decimal.rs? I had actually expected the non-ANSI overflow path to regress, since find scans and then null_if_overflow_precision scans again where the old code made a single pass. It does the opposite by a wide margin. 61x is a stronger argument for this change than anything currently in the table, and it is the shape most exposed to a future edit, being the only one that runs both the scan and null_if_overflow_precision.

The last row goes the other way. unary widens all 8192 rows before find can short-circuit on row zero, where the old loop bailed immediately. Two microseconds on the way to throwing an exception is not worth optimizing, but I would rather the benchmark show the shape than leave it uncovered.

Your no-overflow numbers reproduce directionally on my machine, roughly 31 µs down to 7 µs rather than 32 to 5.9. Different hardware, no concern.

benches/make_decimal.rs:28

Could you reword the comment to drop the reference to the review? Something like "every value in these benches is in the no-overflow common path" says the same thing. The comment outlives the PR, and a reader a year from now has no way to resolve "the shape Andy measured in the PR review."

make_decimal.rs, in test_array_overflow_reports_first_offending_value

The comment still says "Both the original per-row ? loop and try_unary walk rows in index order", but try_unary is gone. The ordering now comes from .iter().flatten().find(...), which is a different mechanism and worth naming. The test itself is right and still valuable.

While you are in there, test_array_boundary_precision pins 999 and 1000 on the array side, but the scalar side only has 999 and 123456. Since the two paths now reach the bounds check through different arrow functions, a scalar case at 1000 would lock them together for the cost of one test.

[0lai0]

Thanks @andygrove for second round review. PR updated
Address review by make_decimal overflow benches and scalar boundary test

[andygrove]

I'm not sure this comment is correct. I do not think with_precision_and_scale rescans in arrow 58.4.0. It calls validate_decimal_precision_and_scale::(precision, scale), which only bounds-checks the two numbers, and then swaps the data_type field.

Maybe the comment does not even need to mention with_precision_and_scale?

[andygrove]

LGTM pending CI, with one comment nit. Thanks @0lai0