feat: Add ScalarValue::{new_one,new_zero,new_ten,distance} support for Decimal128 and Decimal256
#16831
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
| }; | ||
|
|
||
|
Contributor
There was a problem hiding this comment. I guess this lookup table is used for performance? We can do some measurements to check if it's useful.
Contributor
Author
There was a problem hiding this comment. Let me conduct some tests. It could also have been introduced for clarity, too.
|
||
| /// returns true if `needle` is found in a chain of search_op | ||
| /// expressions. Such as: (A AND B) AND C | ||
| fn expr_contains_inner(expr: &Expr, needle: &Expr, search_op: Operator) -> bool { | ||
|
|
@@ -150,6 +110,11 @@ pub fn is_zero(s: &Expr) -> bool { | |
| Expr::Literal(ScalarValue::Float32(Some(v)), _) if *v == 0. => true, | ||
| Expr::Literal(ScalarValue::Float64(Some(v)), _) if *v == 0. => true, | ||
| Expr::Literal(ScalarValue::Decimal128(Some(v), _p, _s), _) if *v == 0 => true, | ||
| Expr::Literal(ScalarValue::Decimal256(Some(v), _p, _s), _) | ||
| if *v == i256::ZERO => | ||
| { | ||
| true | ||
| } | ||
| _ => false, | ||
| } | ||
| } | ||
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Why were powers of 10 precomputed?
Contributor
Author
There was a problem hiding this 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
My other idea about const function to precalculate this array works only for 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
}
Contributor
There was a problem hiding this 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
Contributor
Author
There was a problem hiding this comment. Rolled back to the original lookup map. The new calculation method is used only for Decimal256.
Contributor
There was a problem hiding this comment. I have checked the lookup table approach is faster, perhaps it's better to implement such table in Arrow instead. 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,
)));
}
})
});
} |
||
| && match i128::from(10).checked_pow(*s as u32) { | ||
| Some(res) => res == *v, | ||
| None => false, | ||
| } | ||
| } | ||
| Expr::Literal(ScalarValue::Decimal256(Some(v), _p, s), _) => { | ||
| *s >= 0 | ||
| && match i256::from(10).checked_pow(*s as u32) { | ||
| Some(res) => res == *v, | ||
| None => false, | ||
| } | ||
| } | ||
| _ => false, | ||
| } | ||
|
|
@@ -365,3 +337,78 @@ pub fn distribute_negation(expr: Expr) -> Expr { | |
| _ => Expr::Negative(Box::new(expr)), | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::{is_one, is_zero}; | ||
| use arrow::datatypes::i256; | ||
| use datafusion_common::ScalarValue; | ||
| use datafusion_expr::lit; | ||
|
|
||
| #[test] | ||
| fn test_is_zero() { | ||
| assert!(is_zero(&lit(ScalarValue::Int8(Some(0))))); | ||
| assert!(is_zero(&lit(ScalarValue::Float32(Some(0.0))))); | ||
| assert!(is_zero(&lit(ScalarValue::Decimal128( | ||
| Some(i128::from(0)), | ||
| 9, | ||
| 0 | ||
| )))); | ||
| assert!(is_zero(&lit(ScalarValue::Decimal128( | ||
| Some(i128::from(0)), | ||
| 9, | ||
| 5 | ||
| )))); | ||
| assert!(is_zero(&lit(ScalarValue::Decimal256( | ||
| Some(i256::ZERO), | ||
| 9, | ||
| 0 | ||
| )))); | ||
| assert!(is_zero(&lit(ScalarValue::Decimal256( | ||
| Some(i256::ZERO), | ||
| 9, | ||
| 5 | ||
| )))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_one() { | ||
| assert!(is_one(&lit(ScalarValue::Int8(Some(1))))); | ||
| assert!(is_one(&lit(ScalarValue::Float32(Some(1.0))))); | ||
| assert!(is_one(&lit(ScalarValue::Decimal128( | ||
| Some(i128::from(1)), | ||
| 9, | ||
| 0 | ||
| )))); | ||
| assert!(is_one(&lit(ScalarValue::Decimal128( | ||
| Some(i128::from(10)), | ||
| 9, | ||
| 1 | ||
| )))); | ||
| assert!(is_one(&lit(ScalarValue::Decimal128( | ||
| Some(i128::from(100)), | ||
| 9, | ||
| 2 | ||
| )))); | ||
| assert!(is_one(&lit(ScalarValue::Decimal256( | ||
| Some(i256::from(1)), | ||
| 9, | ||
| 0 | ||
| )))); | ||
| assert!(is_one(&lit(ScalarValue::Decimal256( | ||
| Some(i256::from(10)), | ||
| 9, | ||
| 1 | ||
| )))); | ||
| assert!(is_one(&lit(ScalarValue::Decimal256( | ||
| Some(i256::from(100)), | ||
| 9, | ||
| 2 | ||
| )))); | ||
| assert!(!is_one(&lit(ScalarValue::Decimal256( | ||
| Some(i256::from(100)), | ||
| 9, | ||
| -1 | ||
| )))); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we create a
new_one()for typeDecimal128(3,3), the result in the natural scale will be 0.001:datafusion/datafusion/sql/src/expr/value.rs
Line 467 in 3869857
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)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added support for scale, input verification and some tests. It should match Arrow's decimal semantics now.