Skip to content

fix: add support for Decimal128 and Decimal256 types in interval arithmetic #14126

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
merged 8 commits into from
Jan 25, 2025
Merged

fix: add support for Decimal128 and Decimal256 types in interval arithmetic #14126

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
34088b5
fix: add support for Decimal128 and Decimal256 types in interval arit…
waynexia Jan 14, 2025
d0bef9a
add unit test
waynexia Jan 14, 2025
e3b193a
add sqllogictest
waynexia Jan 15, 2025
c51f5fb
copy constant vectors from arrow
waynexia Jan 23, 2025
a5f129b
toml file
waynexia Jan 23, 2025
e0768e6
update ut
waynexia Jan 23, 2025
fe8d3b6
update lock file
waynexia Jan 23, 2025
0305028
use auto link
waynexia Jan 24, 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
17 changes: 14 additions & 3 deletions datafusion/expr-common/src/interval_arithmetic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,14 @@ macro_rules! get_extreme_value {
DataType::Interval(IntervalUnit::MonthDayNano) => {
ScalarValue::IntervalMonthDayNano(Some(IntervalMonthDayNano::$extreme))
}
DataType::Decimal128(precision, scale) => {
ScalarValue::Decimal128(Some(i128::$extreme), *precision, *scale)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct for min/max of i128? It seems like the minimum value of Decimal128(precision, scale) would be the minimum value for the precision and scale separately rather than the min value of the overall i128 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I dug around in arrow a bit and I think these are the min/max values for each decimal precision

https://docs.rs/arrow/latest/arrow/datatypes/constant.MIN_DECIMAL_FOR_EACH_PRECISION.html
https://docs.rs/arrow/latest/arrow/datatypes/constant.MAX_DECIMAL_FOR_EACH_PRECISION.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, that makes sense. These huge arrays remind me it should be all of 9s...

Copy link
Member Author

Choose a reason for hiding this comment

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

sadly the decimal 256 version is not public 😢 let me find a workaround

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe get_extreme_value should be part of arrow?

Copy link
Contributor

Choose a reason for hiding this comment

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

sadly the decimal 256 version is not public 😢 let me find a workaround

I recommend

  1. copy/pasting the value into DataFusion for now
  2. File a ticket in arrow-rs to make it public (or maybe even a PR!)
  3. leave a comment next to the copy in DataFUsion referencing the upstream ticket

Copy link
Contributor

Choose a reason for hiding this comment

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

I filed a PR in arrow to expose these constants:

}
DataType::Decimal256(precision, scale) => ScalarValue::Decimal256(
Some(arrow::datatypes::i256::$extreme),
*precision,
*scale,
),
_ => unreachable!(),
}
};
Expand Down Expand Up @@ -1008,17 +1016,20 @@ fn handle_overflow<const UPPER: bool>(
lhs: &ScalarValue,
rhs: &ScalarValue,
) -> ScalarValue {
let zero = ScalarValue::new_zero(dt).unwrap();
let lhs_zero = ScalarValue::new_zero(&lhs.data_type()).unwrap();
let rhs_zero = ScalarValue::new_zero(&rhs.data_type()).unwrap();
let positive_sign = match op {
Operator::Multiply | Operator::Divide => {
lhs.lt(&zero) && rhs.lt(&zero) || lhs.gt(&zero) && rhs.gt(&zero)
lhs.lt(&lhs_zero) && rhs.lt(&rhs_zero)
|| lhs.gt(&lhs_zero) && rhs.gt(&rhs_zero)
}
Operator::Plus => lhs.ge(&zero),
Operator::Plus => lhs.ge(&lhs_zero),
Operator::Minus => lhs.ge(rhs),
_ => {
unreachable!()
}
};

match (UPPER, positive_sign) {
(true, true) | (false, false) => ScalarValue::try_from(dt).unwrap(),
(true, false) => {
Expand Down
Loading