You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "theirix (via GitHub)" <gi...@apache.org> on 2025/07/20 19:46:02 UTC
[PR] feat: enhance support for Decimal128 and Decimal256 [datafusion]
theirix opened a new pull request, #16831:
URL: https://github.com/apache/datafusion/pull/16831
## Which issue does this PR close?
- Closes #.
## Rationale for this change
Enhancing support for `ScalarValue::Decimal128` and `ScalarValue::Decimal256`
## What changes are included in this PR?
- Add support for `Decimal128` and `Decimal256` to `ScalarValue` in functions: `new_one`, `new_zero`, `new_ten`, `distance`
- Support simplifcation optimiser rule for `Decimal256`: `is_zero` and `is_one`
## Are these changes tested?
Unit tests for optimiser utils
## Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: enhance support for Decimal128 and Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #16831:
URL: https://github.com/apache/datafusion/pull/16831#discussion_r2218529490
##########
datafusion/optimizer/src/simplify_expressions/utils.rs:
##########
@@ -17,54 +17,14 @@
//! Utility functions for expression simplification
+use arrow::datatypes::i256;
use datafusion_common::{internal_err, Result, ScalarValue};
use datafusion_expr::{
expr::{Between, BinaryExpr, InList},
expr_fn::{and, bitwise_and, bitwise_or, or},
Expr, Like, Operator,
};
-pub static POWS_OF_TEN: [i128; 38] = [
Review Comment:
Let me conduct some tests. It could also have been introduced for clarity, too.
`i128` and `i256` pow are not hardware-backed (with `i256` introducing non-trivial low/high logic), so it's probably better to precompute lists via a `const` function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `ScalarValue::{new_one,new_zero,new_ten,distance}` support for `Decimal128` and `Decimal256` [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 merged PR #16831:
URL: https://github.com/apache/datafusion/pull/16831
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: enhance support for Decimal128 and Decimal256 [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #16831:
URL: https://github.com/apache/datafusion/pull/16831#discussion_r2218150407
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1382,6 +1382,12 @@ impl ScalarValue {
DataType::Float16 => ScalarValue::Float16(Some(f16::from_f32(1.0))),
DataType::Float32 => ScalarValue::Float32(Some(1.0)),
DataType::Float64 => ScalarValue::Float64(Some(1.0)),
+ DataType::Decimal128(precision, scale) => {
+ ScalarValue::Decimal128(Some(1), *precision, *scale)
Review Comment:
If we create a `new_one()` for type `Decimal128(3,3)`, the result in the natural scale will be 0.001:
https://github.com/apache/datafusion/blob/386985788a35e474b829d46c9dccdfd7c5117d98/datafusion/sql/src/expr/value.rs#L467
I think this function is supposed to construct 1 in the natural scale? So in this example it should be converted to `Decimal128(Some(1000), 3, 3)`?
##########
datafusion/optimizer/src/simplify_expressions/utils.rs:
##########
@@ -17,54 +17,14 @@
//! Utility functions for expression simplification
+use arrow::datatypes::i256;
use datafusion_common::{internal_err, Result, ScalarValue};
use datafusion_expr::{
expr::{Between, BinaryExpr, InList},
expr_fn::{and, bitwise_and, bitwise_or, or},
Expr, Like, Operator,
};
-pub static POWS_OF_TEN: [i128; 38] = [
Review Comment:
I guess this lookup table is used for performance? We can do some measurements to check if it's useful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: enhance support for Decimal128 and Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #16831:
URL: https://github.com/apache/datafusion/pull/16831#discussion_r2223625696
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1790,6 +1808,27 @@ impl ScalarValue {
(Self::Float64(Some(l)), Self::Float64(Some(r))) => {
Some((l - r).abs().round() as _)
}
+ (
+ Self::Decimal128(Some(l), lprecision, lscale),
+ Self::Decimal128(Some(r), rprecision, rscale),
+ ) => {
+ if lprecision == rprecision && lscale == rscale {
+ l.checked_sub(*r)?.abs().to_usize()
+ } else {
+ None
+ }
+ }
+ (
+ Self::Decimal256(Some(l), lprecision, lscale),
+ Self::Decimal256(Some(r), rprecision, rscale),
+ ) => {
+ if lprecision == rprecision && lscale == rscale {
+ // l.checked_sub(*r).and_then( |v| v.checked_abs() ).and_then(|v| v.to_usize() )
+ l.checked_sub(*r)?.checked_abs()?.to_usize()
Review Comment:
I think it does – `distance` returns an `Option`, and this code returns None on each failing combinator. Is it correct? Added tests to be sure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: enhance support for Decimal128 and Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #16831:
URL: https://github.com/apache/datafusion/pull/16831#discussion_r2223620746
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1382,6 +1382,12 @@ impl ScalarValue {
DataType::Float16 => ScalarValue::Float16(Some(f16::from_f32(1.0))),
DataType::Float32 => ScalarValue::Float32(Some(1.0)),
DataType::Float64 => ScalarValue::Float64(Some(1.0)),
+ DataType::Decimal128(precision, scale) => {
+ ScalarValue::Decimal128(Some(1), *precision, *scale)
Review Comment:
Added support for scale, input verification and some tests. It should match Arrow's [decimal semantics](https://docs.rs/arrow/latest/arrow/datatypes/enum.DataType.html#variant.Decimal128) now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: enhance support for Decimal128 and Decimal256 [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16831:
URL: https://github.com/apache/datafusion/pull/16831#discussion_r2228482416
##########
datafusion/optimizer/src/simplify_expressions/utils.rs:
##########
@@ -168,10 +133,17 @@ pub fn is_one(s: &Expr) -> bool {
Expr::Literal(ScalarValue::Float64(Some(v)), _) if *v == 1. => true,
Expr::Literal(ScalarValue::Decimal128(Some(v), _p, s), _) => {
*s >= 0
- && POWS_OF_TEN
Review Comment:
maybe since we don't have measurements one way or the other to justfy this change, we revert this change and keep the original approach?
Other than this particular change, this PR looks good to me
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1382,6 +1383,38 @@ impl ScalarValue {
DataType::Float16 => ScalarValue::Float16(Some(f16::from_f32(1.0))),
DataType::Float32 => ScalarValue::Float32(Some(1.0)),
DataType::Float64 => ScalarValue::Float64(Some(1.0)),
+ DataType::Decimal128(precision, scale) => {
+ if let Err(err) = validate_decimal_precision_and_scale::<Decimal128Type>(
Review Comment:
What is the reason to add new InternalError wrappers here?
As in why not just
```suggestion
validate_decimal_precision_and_scale::<Decimal128Type>(*precision, *scale)?;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `ScalarValue::{new_one,new_zero,new_ten,distance}` support for `Decimal128` and `Decimal256` [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #16831:
URL: https://github.com/apache/datafusion/pull/16831#discussion_r2229100403
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1382,6 +1383,38 @@ impl ScalarValue {
DataType::Float16 => ScalarValue::Float16(Some(f16::from_f32(1.0))),
DataType::Float32 => ScalarValue::Float32(Some(1.0)),
DataType::Float64 => ScalarValue::Float64(Some(1.0)),
+ DataType::Decimal128(precision, scale) => {
+ if let Err(err) = validate_decimal_precision_and_scale::<Decimal128Type>(
Review Comment:
Agree, no need for it, updated. Forgot about the auto-conversion from `ArrowError`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `ScalarValue::{new_one,new_zero,new_ten,distance}` support for `Decimal128` and `Decimal256` [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #16831:
URL: https://github.com/apache/datafusion/pull/16831#discussion_r2229103243
##########
datafusion/optimizer/src/simplify_expressions/utils.rs:
##########
@@ -168,10 +133,17 @@ pub fn is_one(s: &Expr) -> bool {
Expr::Literal(ScalarValue::Float64(Some(v)), _) if *v == 1. => true,
Expr::Literal(ScalarValue::Decimal128(Some(v), _p, s), _) => {
*s >= 0
- && POWS_OF_TEN
Review Comment:
Rolled back to the original lookup map. The new calculation method is used only for Decimal256.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: enhance support for Decimal128 and Decimal256 [datafusion]
Posted by "theirix (via GitHub)" <gi...@apache.org>.
theirix commented on code in PR #16831:
URL: https://github.com/apache/datafusion/pull/16831#discussion_r2223673060
##########
datafusion/optimizer/src/simplify_expressions/utils.rs:
##########
@@ -168,10 +133,17 @@ pub fn is_one(s: &Expr) -> bool {
Expr::Literal(ScalarValue::Float64(Some(v)), _) if *v == 1. => true,
Expr::Literal(ScalarValue::Decimal128(Some(v), _p, s), _) => {
*s >= 0
- && POWS_OF_TEN
Review Comment:
I think the initial idea is to mirror Arrow's approach https://github.com/apache/arrow-rs/blob/123045cc766d42d1eb06ee8bb3f09e39ea995ddc/arrow-data/src/decimal.rs
`i128::pow` and `i256::pow` have logarithmic complexity depending on the argument (scale in our case), which is usually low. The precomputed array lookup is surely done in constant time.
My other idea about const function to precalculate this array works only for `i128` since its methods are consts, which is not the case for arrow-buffer's i256. So, the const function cannot be written without tinkering with [`from_parts`](https://arrow.apache.org/rust/arrow_buffer/bigint/struct.i256.html#method.from_parts) manipulations.
```rust
const fn calculate_pows_of_ten_decimal128() -> [i128; DECIMAL128_MAX_PRECISION as usize] {
let mut result = [0i128; DECIMAL128_MAX_PRECISION as usize];
result[0] = 1;
let mut i = 0;
while i <(DECIMAL128_MAX_PRECISION-1) as usize {
result[i+1] = result[i] * 10;
i += 1
}
result
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `ScalarValue::{new_one,new_zero,new_ten,distance}` support for `Decimal128` and `Decimal256` [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #16831:
URL: https://github.com/apache/datafusion/pull/16831#discussion_r2233636830
##########
datafusion/optimizer/src/simplify_expressions/utils.rs:
##########
@@ -168,10 +133,17 @@ pub fn is_one(s: &Expr) -> bool {
Expr::Literal(ScalarValue::Float64(Some(v)), _) if *v == 1. => true,
Expr::Literal(ScalarValue::Decimal128(Some(v), _p, s), _) => {
*s >= 0
- && POWS_OF_TEN
Review Comment:
I have checked the lookup table approach is faster, perhaps it's better to implement such table in Arrow instead.
```rust
fn bench_println(c: &mut Criterion) {
c.bench_function("pow-lookup-table", |b| {
b.iter(|| {
let precision = 30;
let max_scale = 25;
for s in 1..max_scale {
is_one(&lit(ScalarValue::Decimal128(
Some(i128::from(1)),
precision,
max_scale,
)));
}
})
});
// Decimal256 doesn't have a pre-computed power table
c.bench_function("pow-with-calculation", |b| {
b.iter(|| {
let precision = 30;
let max_scale = 25;
for s in 1..max_scale {
is_one(&lit(ScalarValue::Decimal256(
Some(i256::from(1)),
precision,
max_scale,
)));
}
})
});
}
```
```
pow-lookup-table time: [159.16 ns 161.86 ns 166.03 ns]
change: [-0.9307% +0.0846% +1.1886%] (p = 0.90 > 0.05)
No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
5 (5.00%) low mild
2 (2.00%) high mild
3 (3.00%) high severe
pow-with-calculation time: [673.14 ns 674.23 ns 675.36 ns]
change: [-0.3838% -0.1634% +0.0709%] (p = 0.18 > 0.05)
No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) low mild
3 (3.00%) high mild
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: enhance support for Decimal128 and Decimal256 [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #16831:
URL: https://github.com/apache/datafusion/pull/16831#discussion_r2221398992
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1790,6 +1808,27 @@ impl ScalarValue {
(Self::Float64(Some(l)), Self::Float64(Some(r))) => {
Some((l - r).abs().round() as _)
}
+ (
+ Self::Decimal128(Some(l), lprecision, lscale),
+ Self::Decimal128(Some(r), rprecision, rscale),
+ ) => {
+ if lprecision == rprecision && lscale == rscale {
+ l.checked_sub(*r)?.abs().to_usize()
+ } else {
+ None
+ }
+ }
+ (
+ Self::Decimal256(Some(l), lprecision, lscale),
+ Self::Decimal256(Some(r), rprecision, rscale),
+ ) => {
+ if lprecision == rprecision && lscale == rscale {
+ // l.checked_sub(*r).and_then( |v| v.checked_abs() ).and_then(|v| v.to_usize() )
Review Comment:
remove
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1400,6 +1406,12 @@ impl ScalarValue {
DataType::Float16 => ScalarValue::Float16(Some(f16::from_f32(-1.0))),
DataType::Float32 => ScalarValue::Float32(Some(-1.0)),
DataType::Float64 => ScalarValue::Float64(Some(-1.0)),
+ DataType::Decimal128(precision, scale) => {
+ ScalarValue::Decimal128(Some(-1), *precision, *scale)
Review Comment:
same as https://github.com/apache/datafusion/pull/16831/files#r2218150407
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1790,6 +1808,27 @@ impl ScalarValue {
(Self::Float64(Some(l)), Self::Float64(Some(r))) => {
Some((l - r).abs().round() as _)
}
+ (
+ Self::Decimal128(Some(l), lprecision, lscale),
+ Self::Decimal128(Some(r), rprecision, rscale),
+ ) => {
+ if lprecision == rprecision && lscale == rscale {
+ l.checked_sub(*r)?.abs().to_usize()
+ } else {
+ None
+ }
+ }
+ (
+ Self::Decimal256(Some(l), lprecision, lscale),
+ Self::Decimal256(Some(r), rprecision, rscale),
+ ) => {
+ if lprecision == rprecision && lscale == rscale {
+ // l.checked_sub(*r).and_then( |v| v.checked_abs() ).and_then(|v| v.to_usize() )
+ l.checked_sub(*r)?.checked_abs()?.to_usize()
Review Comment:
to_usize returns None on overflow.
shouldn't we return None when checked_sub or checked_abs overflow too?
##########
datafusion/optimizer/src/simplify_expressions/utils.rs:
##########
@@ -168,10 +133,17 @@ pub fn is_one(s: &Expr) -> bool {
Expr::Literal(ScalarValue::Float64(Some(v)), _) if *v == 1. => true,
Expr::Literal(ScalarValue::Decimal128(Some(v), _p, s), _) => {
*s >= 0
- && POWS_OF_TEN
Review Comment:
Why were powers of 10 precomputed?
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1790,6 +1808,27 @@ impl ScalarValue {
(Self::Float64(Some(l)), Self::Float64(Some(r))) => {
Some((l - r).abs().round() as _)
}
+ (
+ Self::Decimal128(Some(l), lprecision, lscale),
+ Self::Decimal128(Some(r), rprecision, rscale),
+ ) => {
+ if lprecision == rprecision && lscale == rscale {
+ l.checked_sub(*r)?.abs().to_usize()
Review Comment:
to_usize returns None on overflow.
shouldn't we return None when checked_sub overflows too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: enhance support for Decimal128 and Decimal256 [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #16831:
URL: https://github.com/apache/datafusion/pull/16831#discussion_r2223630607
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1790,6 +1808,27 @@ impl ScalarValue {
(Self::Float64(Some(l)), Self::Float64(Some(r))) => {
Some((l - r).abs().round() as _)
}
+ (
+ Self::Decimal128(Some(l), lprecision, lscale),
+ Self::Decimal128(Some(r), rprecision, rscale),
+ ) => {
+ if lprecision == rprecision && lscale == rscale {
+ l.checked_sub(*r)?.abs().to_usize()
+ } else {
+ None
+ }
+ }
+ (
+ Self::Decimal256(Some(l), lprecision, lscale),
+ Self::Decimal256(Some(r), rprecision, rscale),
+ ) => {
+ if lprecision == rprecision && lscale == rscale {
+ // l.checked_sub(*r).and_then( |v| v.checked_abs() ).and_then(|v| v.to_usize() )
+ l.checked_sub(*r)?.checked_abs()?.to_usize()
Review Comment:
i didn't realize this is `?` in a None-returning function, not a Result-returning function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org