kosiew commented on code in PR #22306:
URL: https://github.com/apache/datafusion/pull/22306#discussion_r3292593975
##########
datafusion/functions/src/math/factorial.rs:
##########
@@ -139,14 +154,23 @@ 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 {
- Ok(1)
- } else if n < FACTORIALS.len() as i64 {
- Ok(FACTORIALS[n as usize])
- } else {
- exec_err!("Overflow happened on FACTORIAL({n})")
+ return Ok(i256::from(1));
+ }
+
+ 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(|| {
Review Comment:
`compute_factorial` currently checks for `i256` overflow, but it does not
enforce the declared `Decimal256(76, 0)` precision limit.
For example, `57!` has 77 digits, so it exceeds the supported decimal range
even though it still fits in `i256`. That means the scalar path can produce
`ScalarValue::Decimal256(Some(result), 76, 0)` values that violate the return
type, while the array path correctly errors later in `with_precision_and_scale`.
Could we enforce the Decimal256 precision bound in `compute_factorial`, or
in a shared helper used by both the scalar and array paths, so the behavior is
consistent?
It would also be great to add a regression test around the boundary, for
example verifying that `56!` succeeds and `57!` returns an overflow error.
--
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]
