fix: add support for Decimal128 and Decimal256 types in interval arithmetic #14126
Conversation
…hmetic Signed-off-by: Ruihang Xia <[email protected]>
waynexia
changed the title
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
Done 👍, added one in |
| @@ -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) | |||
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ahh, that makes sense. These huge arrays remind me it should be all of 9s...
There was a problem hiding this comment.
sadly the decimal 256 version is not public 😢 let me find a workaround
There was a problem hiding this comment.
maybe get_extreme_value should be part of arrow?
There was a problem hiding this comment.
sadly the decimal 256 version is not public 😢 let me find a workaround
I recommend
- copy/pasting the value into DataFusion for now
- File a ticket in arrow-rs to make it public (or maybe even a PR!)
- leave a comment next to the copy in DataFUsion referencing the upstream ticket
There was a problem hiding this comment.
I filed a PR in arrow to expose these constants:
wait for apache/datafusion#14126 to fix this
…hmetic
Which issue does this PR close?
Closes #14124
Rationale for this change
Fixes the panic when decimal computation overflows
What changes are included in this PR?
This patch contains two fixes:
get_extreme_valuefor decimal 128 and 256 typesAre these changes tested?
adding test
Are there any user-facing changes?