Skip to content

feat: support factorial, pmod, and rint expressions#3586

Open
n0r0shi wants to merge 1 commit intoapache:mainfrom
n0r0shi:math-funcs
Open

feat: support factorial, pmod, and rint expressions#3586
n0r0shi wants to merge 1 commit intoapache:mainfrom
n0r0shi:math-funcs

Conversation

@n0r0shi
Copy link

@n0r0shi n0r0shi commented Feb 25, 2026

Summary

  • Wire three math functions from the datafusion-spark crate (SparkFactorial, SparkPmod, SparkRint) to Comet
  • Register in jni_api.rs and add serde mappings via CometScalarFunction in QueryPlanSerde.scala

Test plan

  • New tests in CometMathExpressionSuite (factorial, pmod, rint) — all pass
  • Spotless formatting check passes
  • Rust native build passes

Register datafusion-spark's SparkFactorial, SparkPmod, and SparkRint
UDFs and add serde mappings in mathExpressions.
Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

Serde wiring and UDF registrations look correct for all three expressions.

For the tests, the SQL file framework in spark/src/test/resources/sql-tests/expressions/math/ would be a natural home for these. There are already many math expression SQL files there.

On pmod, it might be worth testing the divide-by-zero case. Spark returns null for pmod(10, 0) with integer types and NaN for floating-point types. Floating-point inputs would also be good to cover since SparkPmod supports them.

On factorial, I like the coverage of negative and boundary values. One small thing: factorial(21) is the first value that returns null due to overflow protection. It could be worth adding that as a test case.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants