Jefffrey commented on code in PR #22306:
URL: https://github.com/apache/datafusion/pull/22306#discussion_r3325257896
##########
datafusion/functions/src/math/factorial.rs:
##########
@@ -139,14 +155,27 @@ const FACTORIALS: [i64; 21] = [
6402373705728000,
121645100408832000,
2432902008176640000,
-]; // if return type changes, this constant needs to be updated accordingly
+];
-fn compute_factorial(n: i64) -> Result<i64> {
+fn compute_factorial(n: i64) -> Result<i256> {
if n < 0 {
- exec_err!("factorial of a negative number is undefined")
- } else if n < FACTORIALS.len() as i64 {
- Ok(FACTORIALS[n as usize])
- } else {
- exec_err!("Overflow happened on FACTORIAL({n})")
+ return exec_err!("factorial of a negative number is undefined");
+ }
+
+ if n < FACTORIALS.len() as i64 {
+ return Ok(i256::from(FACTORIALS[n as usize]));
+ }
+
+ let mut result = i256::from(FACTORIALS[FACTORIALS.len() - 1]);
+ for value in FACTORIALS.len() as i64..=n {
+ result = result.checked_mul(i256::from(value)).ok_or_else(|| {
+ DataFusionError::Execution(format!("Overflow happened on
FACTORIAL({n})"))
+ })?;
+ }
+
+ if result.to_string().len() > DECIMAL256_MAX_PRECISION as usize {
Review Comment:
we could consider extending the lookup table to 56 entries which removes the
need for the multiplication & validation path
##########
datafusion/functions/src/math/factorial.rs:
##########
@@ -100,9 +112,13 @@ impl ScalarUDFImpl for FactorialFunc {
}
ColumnarValue::Array(array) => match array.data_type() {
Int64 => {
- let result: Int64Array = array
+ let result = array
Review Comment:
we can keep the try_unary which should be much simpler, e.g.
```rust
let result = array
.as_primitive::<Int64Type>()
.try_unary::<_, Decimal256Type,
_>(compute_factorial)?
.with_precision_and_scale(DECIMAL256_MAX_PRECISION,
0)?;
Ok(ColumnarValue::Array(Arc::new(result) as ArrayRef))
```
--
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]
