Dividing decimal type gives wrong error: "170141183460469231731687303715884105727 is too large to store in a Decimal128

[alamb]

Describe the bug
Dividing by Decimal value by zero results in

cd datafusion-cli
cargo run

DataFusion CLI v12.0.0
❯ select cast(1 as int) / cast (0 as int);

ArrowError(DivideByZero)
❯ select cast(1 as double) / cast (0 as double);

ArrowError(DivideByZero)
❯ select cast(1 as decimal(10, 1)) / cast (0 as decimal(10, 1));

ArrowError(InvalidArgumentError("170141183460469231731687303715884105727 is too large to store in a Decimal128 of precision 10. Max is 9999999999"))
❯ 

To Reproduce
See above (divide a decimal number by zero)

Expected behavior
I expect an DivideByZero error:

❯ select cast(1 as decimal(10, 1)) / cast (0 as decimal(10, 1));

ArrowError(DivideByZero)

Additional context
Found while working on #3483

I am pretty sure the problem is in

pub(crate) fn divide_decimal(
    left: &Decimal128Array,
    right: &Decimal128Array,
) -> Result<Decimal128Array> {
    let mul = 10_f64.powi(left.scale() as i32);
    let array = arith_decimal(left, right, |left, right| {
        let l_value = left as f64;
        let r_value = right as f64;
        let result = ((l_value / r_value) * mul) as i128;
        Ok(result)
    })?

Specifically, using floating point math to divide two floats and then casting to an int128 is where the 170141183460469231731687303715884105727 value comes from

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b5949eb324d9828a802aa11b4fa9d029

FYI @liukun4515

[kmitchener]

I'm trying to learn the decimal implementation details, let me take a shot at this.

[alamb]

FWIW @kmitchener -- I would love to move the decimal compute kernels to arrow-rs (and out of datafusion). I think they were originally implemented in datafusion for convenience as we (well mostly @liukun4515 ) worked to do the initial implementation

I think moving them now would be appropriate (especially if we could add them to the various dyn kernels)

[kmitchener]

FWIW @kmitchener -- I would love to move the decimal compute kernels to arrow-rs (and out of datafusion). I think they were originally implemented in datafusion for convenience as we (well mostly @liukun4515 ) worked to do the initial implementation

I think moving them now would be appropriate (especially if we could add them to the various dyn kernels)

Yeah, I see TODOs sprinkled all over about moving it to arrow-rs. :) Makes complete sense, but I think I'm not up to such a huge task yet. However, I think I can do some bug-fixing and add tests to DF while I'm learning about this implementation, then maybe move, or help move the implementation over to arrow-rs after I have a better idea what's going on.

[liukun4515]

FWIW @kmitchener -- I would love to move the decimal compute kernels to arrow-rs (and out of datafusion). I think they were originally implemented in datafusion for convenience as we (well mostly @liukun4515 ) worked to do the initial implementation

I think moving them now would be appropriate (especially if we could add them to the various dyn kernels)

Yes, I think it's time to move the operation about the decimal to the arrow-rs kernel.

I will do this in the next sprint. I think we may meet some issue about the decimal type in the arrow-rs talked
apache/arrow-rs#2387 (comment)
apache/arrow-rs#2637

I think we can resolve them.

[liukun4515]

I think this bug is caused by using the f32 or f64 to do the divide/multiply operation.

Because the intermediate result will be overflow when do the agg.

I will try to do investigation and to resolve them, and find the reasonable behavior.

@alamb @kmitchener

I have listed a issue to collect the issue about decimal and TODO work.

[alamb]

Sorry @liukun4515 -- I didn't see your comment before I merged #3517 -- I am happy to back it out or make other changes if needed.

[alamb]

I will do this in the next sprint. I think we may meet some issue about the decimal type in the arrow-rs talked

This is great news -- thanks @liukun4515

It may take some time to get the kernels into arrow, but I think the process will likely work out some of the corner cases we have left in datafusion for a while

[liukun4515]

Sorry @liukun4515 -- I didn't see your comment before I merged #3517 -- I am happy to back it out or make other changes if needed.

don't need to back it out.
this pr solves the issue divide zero error before move the decimal operation to the arrow-rs kernel.