@andygrove @viirya @alamb please take a look.

There is a regression in test_cast_decimal128_to_decimal256_negative. The new validation check is correctly throwing an error, but it looks like we also need to add this validation when creating decimal arrays since the current test is creating invalid arrays before the cast:

let array = vec![Some(i128::MAX), Some(i128::MIN)];
let input_decimal_array = create_decimal_array(array, 10, 3).unwrap();

I would expect this to fail, so we probably need to add the same validation there.

There is a regression in test_cast_decimal128_to_decimal256_negative. The new validation check is correctly throwing an error, but it looks like we also need to add this validation when creating decimal arrays since the current test is creating invalid arrays before the cast:

let array = vec![Some(i128::MAX), Some(i128::MIN)];
let input_decimal_array = create_decimal_array(array, 10, 3).unwrap();

I would expect this to fail, so we probably need to add the same validation there.

On second thoughts, it seems it was an intentional design decision not to validate this on array creation. Instead, an array.validate_decimal_precision method can optionally be called on the array to validate it after creation, so we should probably just update the test as needed (@alamb @tustvold perhaps you could correct me if I am wrong about this).

Have we run any benchmarks (I'm not sure if any actually exist) to confirm this doesn't significantly regress performance.

It seems unfortunate to be always performing overflow checks, when in many cases it should be possible to prove that precision overflow can't occur and need not be checked for

I'll create a separate PR (probably tomorrow) to add some criterion benchmarks

There is a regression in test_cast_decimal128_to_decimal256_negative. The new validation check is correctly throwing an error, but it looks like we also need to add this validation when creating decimal arrays since the current test is creating invalid arrays before the cast:

let array = vec![Some(i128::MAX), Some(i128::MIN)];
let input_decimal_array = create_decimal_array(array, 10, 3).unwrap();

I would expect this to fail, so we probably need to add the same validation there.

On second thoughts, it seems it was an intentional design decision not to validate this on array creation. Instead, an array.validate_decimal_precision method can optionally be called on the array to validate it after creation, so we should probably just update the test as needed (@alamb @tustvold perhaps you could correct me if I am wrong about this).

Changed the test to pass.

Can anyone please let the build run - workflows waiting for approval.

Have we run any benchmarks (I'm not sure if any actually exist) to confirm this doesn't significantly regress performance.

It seems unfortunate to be always performing overflow checks, when in many cases it should be possible to prove that precision overflow can't occur and need not be checked for

I created a simple benchmark for decimal casting in #6850.

Unsurprisingly, validating that the results are correct is slower than not validating the results.

before

cast_decimal            time:   [45.281 ns 45.549 ns 45.871 ns]

after (this PR)

cast_decimal            time:   [247.97 ns 248.78 ns 249.78 ns]
                        change: [+435.06% +439.47% +443.15%] (p = 0.00 < 0.05)

We currently have the config option of safe on or off:

pub struct CastOptions<'a> {
    /// how to handle cast failures, either return NULL (safe=true) or return ERR (safe=false)
    pub safe: bool,

So, yes, it is a performance regression, but the previous behavior was incorrect. This PR now makes this work as advertised.

@tustvold Is there a use case we need to support for faster casts without validating results per the CastOptions?

@tustvold Is there a use case we need to support for faster casts without validating results per the CastOptions?

No, the cast should be checked, apologies if that wasn't clear, my concern was the PR as originally formulated blindly performed the checked conversion regardless of the input type, even when the cast was increasing the precision. Given the whole purpose of tracking precision is to avoid overflow checks, it seemed a little off.

I'll try to find some time to take another look at this PR as from a quick scan this looks to have been addressed