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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
