Skip to content

More decimal 32/64 support - type coercsion and misc gaps #17808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jefffrey merged 3 commits into from
Sep 30, 2025
Merged

More decimal 32/64 support - type coercsion and misc gaps #17808

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4145a04
More small decimal support
AdamGS Sep 27, 2025
fd1f043
CR comments
AdamGS Sep 29, 2025
f0b7433
Add tests and cleanup some code
AdamGS Sep 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions datafusion/common/src/scalar/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1362,6 +1362,12 @@ impl ScalarValue {
DataType::Float16 => ScalarValue::Float16(Some(f16::from_f32(0.0))),
DataType::Float32 => ScalarValue::Float32(Some(0.0)),
DataType::Float64 => ScalarValue::Float64(Some(0.0)),
DataType::Decimal32(precision, scale) => {
ScalarValue::Decimal32(Some(0), *precision, *scale)
}
DataType::Decimal64(precision, scale) => {
ScalarValue::Decimal64(Some(0), *precision, *scale)
}
DataType::Decimal128(precision, scale) => {
ScalarValue::Decimal128(Some(0), *precision, *scale)
}
Expand Down
96 changes: 91 additions & 5 deletions datafusion/expr-common/src/type_coercion/binary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,14 @@ fn math_decimal_coercion(
| (Decimal256(_, _), Decimal256(_, _)) => {
Some((lhs_type.clone(), rhs_type.clone()))
}
// Cross-variant decimal coercion - choose larger variant with appropriate precision/scale
(Decimal32(_, _), Decimal64(_, _) | Decimal128(_, _) | Decimal256(_, _))
| (Decimal64(_, _), Decimal32(_, _) | Decimal128(_, _) | Decimal256(_, _))
| (Decimal128(_, _), Decimal32(_, _) | Decimal64(_, _) | Decimal256(_, _))
| (Decimal256(_, _), Decimal32(_, _) | Decimal64(_, _) | Decimal128(_, _)) => {
let coerced_type = get_wider_decimal_type_cross_variant(lhs_type, rhs_type)?;
Some((coerced_type.clone(), coerced_type))
}
Comment thread
AdamGS marked this conversation as resolved.
// Unlike with comparison we don't coerce to a decimal in the case of floating point
// numbers, instead falling back to floating point arithmetic instead
(
Expand Down Expand Up @@ -955,28 +963,106 @@ pub fn decimal_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<Data

match (lhs_type, rhs_type) {

@Jefffrey Jefffrey Sep 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be cleaner like so:

/// Decimal coercion rules.
pub fn decimal_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> {
    use arrow::datatypes::DataType::*;

    // Prefer decimal data type over floating point for comparison operation
    match (lhs_type, rhs_type) {
        // Same decimal types
        (lhs_type, rhs_type)
            if std::mem::discriminant(lhs_type) == std::mem::discriminant(rhs_type) =>
        {
            get_wider_decimal_type(lhs_type, rhs_type)
        }
        // Mismatched decimal types
        (lhs_type, rhs_type)
            if is_decimal(lhs_type)
                && is_decimal(rhs_type)
                && std::mem::discriminant(lhs_type)
                    != std::mem::discriminant(rhs_type) =>
        {
            get_wider_decimal_type_cross_variant(lhs_type, rhs_type)
        }
        // Decimal + non-decimal types
        (Decimal32(_, _) | Decimal64(_, _) | Decimal128(_, _) | Decimal256(_, _), _)
        | (_, Decimal32(_, _) | Decimal64(_, _) | Decimal128(_, _) | Decimal256(_, _)) => {
            get_common_decimal_type(lhs_type, rhs_type)
        }
        (_, _) => None,
    }
}

Following what was done above

@Jefffrey Jefffrey Sep 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops forgot the is_decimal() checks for the first branch

@AdamGS AdamGS Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I've added them locally :) should have something soon

@AdamGS AdamGS Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done as part of 4145a04

// Prefer decimal data type over floating point for comparison operation
(Decimal32(_, _), Decimal32(_, _)) => get_wider_decimal_type(lhs_type, rhs_type),
(Decimal32(_, _), Decimal64(_, _) | Decimal128(_, _) | Decimal256(_, _)) => {
get_wider_decimal_type_cross_variant(lhs_type, rhs_type)
}
(Decimal32(_, _), _) => get_common_decimal_type(lhs_type, rhs_type),
(Decimal64(_, _), Decimal64(_, _)) => get_wider_decimal_type(lhs_type, rhs_type),
(Decimal64(_, _), Decimal32(_, _) | Decimal128(_, _) | Decimal256(_, _)) => {
get_wider_decimal_type_cross_variant(lhs_type, rhs_type)
}
(Decimal64(_, _), _) => get_common_decimal_type(lhs_type, rhs_type),
(Decimal128(_, _), Decimal128(_, _)) => {
get_wider_decimal_type(lhs_type, rhs_type)
}
(Decimal128(_, _), Decimal32(_, _) | Decimal64(_, _) | Decimal256(_, _)) => {
get_wider_decimal_type_cross_variant(lhs_type, rhs_type)
}
(Decimal128(_, _), _) => get_common_decimal_type(lhs_type, rhs_type),
(_, Decimal128(_, _)) => get_common_decimal_type(rhs_type, lhs_type),
(Decimal256(_, _), Decimal256(_, _)) => {
get_wider_decimal_type(lhs_type, rhs_type)
}
(Decimal256(_, _), Decimal32(_, _) | Decimal64(_, _) | Decimal128(_, _)) => {
get_wider_decimal_type_cross_variant(lhs_type, rhs_type)
}
(Decimal256(_, _), _) => get_common_decimal_type(lhs_type, rhs_type),
(_, Decimal32(_, _)) => get_common_decimal_type(rhs_type, lhs_type),
(_, Decimal64(_, _)) => get_common_decimal_type(rhs_type, lhs_type),
(_, Decimal128(_, _)) => get_common_decimal_type(rhs_type, lhs_type),
(_, Decimal256(_, _)) => get_common_decimal_type(rhs_type, lhs_type),
(_, _) => None,
}
}

/// Handle cross-variant decimal widening by choosing the larger variant
fn get_wider_decimal_type_cross_variant(
lhs_type: &DataType,
rhs_type: &DataType,
) -> Option<DataType> {
use arrow::datatypes::DataType::*;

let (p1, s1) = match lhs_type {
Decimal32(p, s) => (*p, *s),
Decimal64(p, s) => (*p, *s),
Decimal128(p, s) => (*p, *s),
Decimal256(p, s) => (*p, *s),
_ => return None,
};

let (p2, s2) = match rhs_type {
Decimal32(p, s) => (*p, *s),
Decimal64(p, s) => (*p, *s),
Decimal128(p, s) => (*p, *s),
Decimal256(p, s) => (*p, *s),
_ => return None,
};

// max(s1, s2) + max(p1-s1, p2-s2), max(s1, s2)
let s = s1.max(s2);
let range = (p1 as i8 - s1).max(p2 as i8 - s2);
let required_precision = (range + s) as u8;
Comment on lines +1029 to +1031

@Jefffrey Jefffrey Sep 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we have:

Decimal256 with precision 76 (max) and scale 0, and Decimal128 with precision 38 (max) with scale 1;

So s = 1, range = 76, required_precision = 76 + 1 -> overflow?

Is this a valid case?

@AdamGS AdamGS Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think an overflow is valid, I'll have to think about it and maybe look into solutions in other systems.
We can also just return None in that case, which should force the user to add an explicit cast to one side.

@AdamGS AdamGS Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked around a bit, and what I could find is:

  1. DataFusion already has multiple issues regarding cast overflow/precision loss (decimal calculate overflow but not throw error #16406, Datafusion downcasts decimal loosing precision  #13492), which I'm happy to take on but are unrelated here.
  2. Spark (which seems to be the main inspiration for this code) has a configuration to control how it handles these cases (here and here).

I'm not sure what's the desired behavior regarding precision loss (should it be configurable? Is there currently an accepted desired behavior?), I think for this PR it should be fine to just return None if the precision overflows, and take the bigger conversation into an issue where people can weigh in, and I'll be glad to take that forward. What do you think?

@Jefffrey Jefffrey Sep 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning None in cases like this for this PR is fine 👍

@AdamGS AdamGS Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done as part of fd1f043

@Jefffrey Jefffrey Sep 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers; left another minor comment related to the check below. Also would be nice if we had a test for this edge case.


// Choose the larger variant between the two input types
match (lhs_type, rhs_type) {
(Decimal32(_, _), Decimal64(_, _)) | (Decimal64(_, _), Decimal32(_, _)) => {
Some(Decimal64(required_precision, s))
}
(Decimal32(_, _), Decimal128(_, _)) | (Decimal128(_, _), Decimal32(_, _)) => {
Some(Decimal128(required_precision, s))
}
(Decimal32(_, _), Decimal256(_, _)) | (Decimal256(_, _), Decimal32(_, _)) => {
Some(Decimal256(required_precision, s))
}
(Decimal64(_, _), Decimal128(_, _)) | (Decimal128(_, _), Decimal64(_, _)) => {
Some(Decimal128(required_precision, s))
}
(Decimal64(_, _), Decimal256(_, _)) | (Decimal256(_, _), Decimal64(_, _)) => {
Some(Decimal256(required_precision, s))
}
(Decimal128(_, _), Decimal256(_, _)) | (Decimal256(_, _), Decimal128(_, _)) => {
Some(Decimal256(required_precision, s))
}
_ => None,
}
}

/// Coerce `lhs_type` and `rhs_type` to a common type.
fn get_common_decimal_type(
decimal_type: &DataType,
other_type: &DataType,
) -> Option<DataType> {
use arrow::datatypes::DataType::*;
match decimal_type {
Decimal32(_, _) | Decimal64(_, _) | Decimal128(_, _) => {
Decimal32(_, _) => {
let other_decimal_type = coerce_numeric_type_to_decimal32(other_type)?;
get_wider_decimal_type(decimal_type, &other_decimal_type)
}
Decimal64(_, _) => {
let other_decimal_type = coerce_numeric_type_to_decimal64(other_type)?;
get_wider_decimal_type(decimal_type, &other_decimal_type)
}
Decimal128(_, _) => {
let other_decimal_type = coerce_numeric_type_to_decimal128(other_type)?;
get_wider_decimal_type(decimal_type, &other_decimal_type)
}
Expand All @@ -988,7 +1074,7 @@ fn get_common_decimal_type(
}
}

/// Returns a `DataType::Decimal128` that can store any value from either
/// Returns a decimal [`DataType`] variant that can store any value from either
/// `lhs_decimal_type` and `rhs_decimal_type`
///
/// The result decimal type is `(max(s1, s2) + max(p1-s1, p2-s2), max(s1, s2))`.
Expand Down Expand Up @@ -1209,14 +1295,14 @@ fn numerical_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataTy
}

fn create_decimal32_type(precision: u8, scale: i8) -> DataType {
DataType::Decimal128(
DataType::Decimal32(
DECIMAL32_MAX_PRECISION.min(precision),
DECIMAL32_MAX_SCALE.min(scale),
)
}

fn create_decimal64_type(precision: u8, scale: i8) -> DataType {
DataType::Decimal128(
DataType::Decimal64(
DECIMAL64_MAX_PRECISION.min(precision),
DECIMAL64_MAX_SCALE.min(scale),
)
Expand Down
90 changes: 90 additions & 0 deletions datafusion/expr-common/src/type_coercion/binary/tests/arithmetic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,3 +291,93 @@ fn test_coercion_arithmetic_decimal() -> Result<()> {

Ok(())
}

#[test]
fn test_coercion_arithmetic_decimal_cross_variant() -> Result<()> {
let test_cases = [
(
DataType::Decimal32(5, 2),
DataType::Decimal64(10, 3),
DataType::Decimal64(10, 3),
DataType::Decimal64(10, 3),
),
(
DataType::Decimal32(7, 1),
DataType::Decimal128(15, 4),
DataType::Decimal128(15, 4),
DataType::Decimal128(15, 4),
),
(
DataType::Decimal32(9, 0),
DataType::Decimal256(20, 5),
DataType::Decimal256(20, 5),
DataType::Decimal256(20, 5),
),
(
DataType::Decimal64(12, 3),
DataType::Decimal128(18, 2),
DataType::Decimal128(19, 3),
DataType::Decimal128(19, 3),
),
(
DataType::Decimal64(15, 4),
DataType::Decimal256(25, 6),
DataType::Decimal256(25, 6),
DataType::Decimal256(25, 6),
),
(
DataType::Decimal128(20, 5),
DataType::Decimal256(30, 8),
DataType::Decimal256(30, 8),
DataType::Decimal256(30, 8),
),
// Reverse order cases
(
DataType::Decimal64(10, 3),
DataType::Decimal32(5, 2),
DataType::Decimal64(10, 3),
DataType::Decimal64(10, 3),
),
(
DataType::Decimal128(15, 4),
DataType::Decimal32(7, 1),
DataType::Decimal128(15, 4),
DataType::Decimal128(15, 4),
),
(
DataType::Decimal256(20, 5),
DataType::Decimal32(9, 0),
DataType::Decimal256(20, 5),
DataType::Decimal256(20, 5),
),
(
DataType::Decimal128(18, 2),
DataType::Decimal64(12, 3),
DataType::Decimal128(19, 3),
DataType::Decimal128(19, 3),
),
(
DataType::Decimal256(25, 6),
DataType::Decimal64(15, 4),
DataType::Decimal256(25, 6),
DataType::Decimal256(25, 6),
),
(
DataType::Decimal256(30, 8),
DataType::Decimal128(20, 5),
DataType::Decimal256(30, 8),
DataType::Decimal256(30, 8),
),
];

for (lhs_type, rhs_type, expected_lhs_type, expected_rhs_type) in test_cases {
test_math_decimal_coercion_rule(
lhs_type,
rhs_type,
expected_lhs_type,
expected_rhs_type,
);
}

Ok(())
}
88 changes: 88 additions & 0 deletions datafusion/expr-common/src/type_coercion/binary/tests/comparison.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,3 +697,91 @@ fn test_map_coercion() -> Result<()> {
);
Ok(())
}

#[test]
fn test_decimal_cross_variant_comparison_coercion() -> Result<()> {
let test_cases = [
// (lhs, rhs, expected_result)
(
DataType::Decimal32(5, 2),
DataType::Decimal64(10, 3),
DataType::Decimal64(10, 3),
),
(
DataType::Decimal32(7, 1),
DataType::Decimal128(15, 4),
DataType::Decimal128(15, 4),
),
(
DataType::Decimal32(9, 0),
DataType::Decimal256(20, 5),
DataType::Decimal256(20, 5),
),
(
DataType::Decimal64(12, 3),
DataType::Decimal128(18, 2),
DataType::Decimal128(19, 3),
),
(
DataType::Decimal64(15, 4),
DataType::Decimal256(25, 6),
DataType::Decimal256(25, 6),
),
(
DataType::Decimal128(20, 5),
DataType::Decimal256(30, 8),
DataType::Decimal256(30, 8),
),
// Reverse order cases
(
DataType::Decimal64(10, 3),
DataType::Decimal32(5, 2),
DataType::Decimal64(10, 3),
),
(
DataType::Decimal128(15, 4),
DataType::Decimal32(7, 1),
DataType::Decimal128(15, 4),
),
(
DataType::Decimal256(20, 5),
DataType::Decimal32(9, 0),
DataType::Decimal256(20, 5),
),
(
DataType::Decimal128(18, 2),
DataType::Decimal64(12, 3),
DataType::Decimal128(19, 3),
),
(
DataType::Decimal256(25, 6),
DataType::Decimal64(15, 4),
DataType::Decimal256(25, 6),
),
(
DataType::Decimal256(30, 8),
DataType::Decimal128(20, 5),
DataType::Decimal256(30, 8),
),
];

let comparison_op_types = [
Operator::NotEq,
Operator::Eq,
Operator::Gt,
Operator::GtEq,
Operator::Lt,
Operator::LtEq,
];

for (lhs_type, rhs_type, expected_type) in test_cases {
for op in comparison_op_types {
let (lhs, rhs) =
BinaryTypeCoercer::new(&lhs_type, &op, &rhs_type).get_input_types()?;
assert_eq!(expected_type, lhs, "Coercion of type {lhs_type:?} with {rhs_type:?} resulted in unexpected type: {lhs:?}");
assert_eq!(expected_type, rhs, "Coercion of type {rhs_type:?} with {lhs_type:?} resulted in unexpected type: {rhs:?}");
}
}

Ok(())
}
12 changes: 10 additions & 2 deletions datafusion/functions/src/math/abs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ use std::any::Any;
use std::sync::Arc;

use arrow::array::{
ArrayRef, Decimal128Array, Decimal256Array, Float32Array, Float64Array, Int16Array,
Int32Array, Int64Array, Int8Array,
ArrayRef, Decimal128Array, Decimal256Array, Decimal32Array, Decimal64Array,
Float32Array, Float64Array, Int16Array, Int32Array, Int64Array, Int8Array,
};
use arrow::datatypes::DataType;
use arrow::error::ArrowError;
Expand Down Expand Up @@ -98,6 +98,8 @@ fn create_abs_function(input_data_type: &DataType) -> Result<MathArrayFunction>
| DataType::UInt64 => Ok(|input: &ArrayRef| Ok(Arc::clone(input))),

// Decimal types
DataType::Decimal32(_, _) => Ok(make_decimal_abs_function!(Decimal32Array)),
DataType::Decimal64(_, _) => Ok(make_decimal_abs_function!(Decimal64Array)),
DataType::Decimal128(_, _) => Ok(make_decimal_abs_function!(Decimal128Array)),
DataType::Decimal256(_, _) => Ok(make_decimal_abs_function!(Decimal256Array)),

Expand Down Expand Up @@ -162,6 +164,12 @@ impl ScalarUDFImpl for AbsFunc {
DataType::UInt16 => Ok(DataType::UInt16),
DataType::UInt32 => Ok(DataType::UInt32),
DataType::UInt64 => Ok(DataType::UInt64),
DataType::Decimal32(precision, scale) => {
Ok(DataType::Decimal32(precision, scale))
}
DataType::Decimal64(precision, scale) => {
Ok(DataType::Decimal64(precision, scale))
}
DataType::Decimal128(precision, scale) => {
Ok(DataType::Decimal128(precision, scale))
}
Expand Down
4 changes: 4 additions & 0 deletions datafusion/physical-plan/src/joins/sort_merge_join/stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1922,6 +1922,8 @@ fn compare_join_arrays(
DataType::BinaryView => compare_value!(BinaryViewArray),
DataType::FixedSizeBinary(_) => compare_value!(FixedSizeBinaryArray),
DataType::LargeBinary => compare_value!(LargeBinaryArray),
DataType::Decimal32(..) => compare_value!(Decimal32Array),
DataType::Decimal64(..) => compare_value!(Decimal64Array),
DataType::Decimal128(..) => compare_value!(Decimal128Array),
DataType::Timestamp(time_unit, None) => match time_unit {
TimeUnit::Second => compare_value!(TimestampSecondArray),
Expand Down Expand Up @@ -1994,6 +1996,8 @@ fn is_join_arrays_equal(
DataType::BinaryView => compare_value!(BinaryViewArray),
DataType::FixedSizeBinary(_) => compare_value!(FixedSizeBinaryArray),
DataType::LargeBinary => compare_value!(LargeBinaryArray),
DataType::Decimal32(..) => compare_value!(Decimal32Array),
DataType::Decimal64(..) => compare_value!(Decimal64Array),
Comment on lines +1999 to +2000

@Jefffrey Jefffrey Sep 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I am curious why 256 is omitted? Perhaps we can add it in if there's no compiler error doing so?

@AdamGS AdamGS Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll add it. Through this work I've noticed a few places where it was omitted, but I've yet to find a reason or even a failing test that would explain it, usually its just a matter of adding a branch to a match statement like here.

DataType::Decimal128(..) => compare_value!(Decimal128Array),
DataType::Timestamp(time_unit, None) => match time_unit {
TimeUnit::Second => compare_value!(TimestampSecondArray),
Expand Down
Loading