Skip to content

Conversation

@getChan
Copy link
Contributor

@getChan getChan commented Dec 31, 2025

Which issue does this PR close?

  • Closes #.

Rationale for this change

benchmark

factorial_array         time:   [778.63 ns 783.03 ns 787.52 ns]
                        change: [−90.314% −90.211% −90.055%] (p = 0.00 < 0.05)
                        Performance has improved.

factorial_scalar        time:   [317.32 ns 319.36 ns 321.41 ns]
                        change: [−28.219% −27.508% −26.683%] (p = 0.00 < 0.05)
                        Performance has improved.

What changes are included in this PR?

  1. factorial implementation from using loop to looking up precomputed values.
  2. using the try_unary kernel function on PrimitiveArray. (However, since this isn’t unary function, it does not get vectorized execution)

and add benchmark

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@github-actions github-actions bot added the functions Changes to functions implementation label Dec 31, 2025
} else if a < FACTORIALS.len() as i64 {
Ok(FACTORIALS[a as usize])
} else {
Err(ArrowError::ComputeError(format!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should prob to be exec_err!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! applied it

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @getChan it makes sense as long as input is i64, for bigger input datatypes it would be needed to expand the lookup table, for example duckdb supports larger factorial and converts it to i128

D select factorial(33);
┌───────────────────────────────────────┐
│             factorial(33)             │
│                int128                 │
├───────────────────────────────────────┤
│ 8683317618811886495518194401280000000 │
└───────────────────────────────────────┘

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 1, 2026
@Jefffrey
Copy link
Contributor

Jefffrey commented Jan 2, 2026

This is very nice 🚀

@comphead
Copy link
Contributor

comphead commented Jan 2, 2026

@getChan please resolve conflicts

# Conflicts:
#	datafusion/functions/Cargo.toml
@Jefffrey Jefffrey added this pull request to the merge queue Jan 3, 2026
Merged via the queue into apache:main with commit 715962c Jan 3, 2026
31 checks passed
@Jefffrey
Copy link
Contributor

Jefffrey commented Jan 3, 2026

Thanks @getChan & @comphead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants