fix(spark): accept every integer width in `factorial`, matching Spark's implicit cast by ShayanGho · Pull Request #24943 · apache/datafusion

ShayanGho · 2026-09-04T03:25:18Z

Which issue does this PR close?

Closes [Bug] spark factorial rejects BIGINT and untyped integer literals that Spark accepts #24940.

Rationale for this change

In Spark mode, SELECT factorial(5) fails to plan: an untyped integer literal is Int64 in
DataFusion and the function's signature was Exact(Int32). Spark's Factorial declares
inputTypes = Seq(IntegerType) but extends ImplicitCastInputTypes, so a real Spark casts
TINYINT, SMALLINT and BIGINT arguments to INT before evaluating. Verified against
pyspark==4.2.0 under both ANSI settings: every integer width returns the same values.

The narrow signature was chosen deliberately in #16125 from the Databricks reference, which
documents the parameter as an INTEGER expression. That describes the declared parameter type;
the accepted set is wider because of the implicit cast, which is only visible by running Spark.

What changes are included in this PR?

SparkFactorial now uses Signature::coercible with an implicit coercion from the Integer
type class to Int32, the same construct round, width_bucket and bit_get in this crate
use. Planning inserts the cast, so spark_factorial still only receives Int32.
factorial.slt: replaces the assertion that factorial(5::BIGINT) must fail with two blocks: one
aliased row covering the untyped literal, TINYINT, SMALLINT, BIGINT and a bare NULL, and an
array-shaped BIGINT query covering a negative value, both range boundaries, an above-range value
and NULL. Every expected value was observed from PySpark 4.2.0, not from DataFusion output.
Non-integer inputs still fail, now with DataFusion's standard coercion error. Spark's wider
acceptance of STRING / DECIMAL / FLOAT inputs is tracked in [Bug] spark factorial rejects STRING, DECIMAL and floating-point inputs that Spark implicitly casts to INT #24941, and the ANSI-dependent
handling of out-of-range integers in [Bug] spark factorial: BIGINT overflow should wrap in non-ANSI mode and raise CAST_OVERFLOW in ANSI mode, as in Spark #24950 (part of [EPIC] Support ANSI mode #23929).

What is the testing strategy for this PR?

The new .slt queries fail on main and pass with this change, verified by checking out the
pre-fix implementation file and re-running cargo test --test sqllogictests -- spark/math/factorial:

with the old signature checked out: both blocks fail with
coercion from Int64 to the signature Exact(Int32) failed.
restored: Progress: 1/1 files completed (100%).

cargo test -p datafusion-spark factorial, cargo fmt --all -- --check and
cargo clippy -p datafusion-spark --all-targets -- -D warnings pass. The existing unit tests in
factorial.rs are unchanged.

Are there any user-facing changes?

factorial in datafusion-spark now accepts all integer widths. No breaking API change.

🤖 Generated with Claude Code

… implicit cast Spark's Factorial extends ImplicitCastInputTypes, so TINYINT through BIGINT (and an untyped literal, which is Int64 in DataFusion) are cast to INT before evaluation. Replace the Exact(Int32) signature with a coercible one that casts the Integer type class to Int32, and replace the test asserting the BIGINT coercion error with Spark-verified expectations. STRING, DECIMAL and floating-point inputs remain rejected; tracked in apache#24941. Closes apache#24940 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

ShayanGho · 2026-09-04T04:18:32Z

Hello,

This is my first DataFusion PR, so the CI needs a committer to trigger it. @andygrove @comphead could one of you please approve the workflows when you have a moment?

Context: the changfe came out of running the audit-datafusion-spark-expression skill on factorial. Spark 4.2.0 (verified with PySpark) accepts every integer width via ImplicitCastInputTypes; the crate's Exact(Int32) signature rejected even a bare factorial(5). Details and the deferred STRING/DECIMAL/FLOAT cases are in #24940 and #24941.

kumarUjjawal

Thank you @ShayanGho for working on this. I left few comments please take a look.

kumarUjjawal · 2026-09-04T13:53:38Z

        Self {
-            signature: Signature::exact(vec![Int32], Volatility::Immutable),
+            signature: Signature::coercible(
+                vec![Coercion::new_implicit(


Could we use Coercion::new_implicit_native(logical_int32(), vec![TypeSignatureClass::Integer]) here? That constructor was added for native-target coercions so the desired Int32 type and NativeType::Int32 cannot accidentally diverge. It would also remove the NativeType import.

Switched to Coercion::new_implicit_native and removed the NativeType import.

kumarUjjawal · 2026-09-04T13:54:02Z

-query error Error during planning: Failed to coerce arguments to satisfy a call to 'factorial' function
-SELECT factorial(5::BIGINT);
+# Spark declares factorial(INT) with ImplicitCastInputTypes, so every integer width is
+# accepted; an untyped literal (Int64 in DataFusion) must work too. Values from Spark 4.2.0.


Could we clarify the test provenance here? This row includes UInt64, but Spark has no unsigned integer type, so that expected value cannot come from Spark 4.2.0. I suggest identifying the signed cases as Spark-verified and the unsigned case as DataFusion-specific coverage, or removing the unsigned case.

Removed the UInt64 case and updated the query signature and expected output. The remaining cases use types available in Spark.

kumarUjjawal · 2026-09-04T13:55:08Z

CI needs a committer to trigger it.

Done!

codecov-commenter · 2026-09-04T14:22:39Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.57%. Comparing base (5980374) to head (4a8c1b7).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24943      +/-   ##
==========================================
- Coverage   81.63%   81.57%   -0.07%     
==========================================
  Files        1123     1123              
  Lines      410298   410796     +498     
  Branches   410298   410796     +498     
==========================================
+ Hits       334965   335118     +153     
- Misses      55642    55919     +277     
- Partials    19691    19759      +68

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

comphead · 2026-09-04T15:22:46Z

+NULL
+NULL
+
+# DataFusion always fails at the Int32 cast here (this function does not consult


lets concise comments, for this particular thing we should create another follow up issue and link it to #23929

Removed the detailed comments and overflow assertion from this PR. The ANSI-dependent BIGINT-to-INT cast behavior is now tracked in #24950, linked to #23929.

comphead · 2026-09-04T15:23:48Z

+# DataFusion always fails at the Int32 cast here (this function does not consult
+# datafusion.execution.enable_ansi_mode). Spark 4.2.0 raises CAST_OVERFLOW under
+# ANSI mode but returns NULL when ANSI is off.
+query error Can't cast value 5000000000 to type Int32


Removed this overflow assertion from the PR as well.

comphead · 2026-09-04T15:25:28Z

+SELECT factorial(5000000000::BIGINT);
+
+# Spark 4.2.0 also accepts STRING, DECIMAL and floating-point inputs through ImplicitCastInputTypes
+# (strings and overflow are ANSI dependent there). DataFusion rejects these types at planning time.


this test is prob not needed. Spark functions are not designed to be called directly from DF, its rather DF extension for Spark, and Spark already provides correct datatype

Removed the string-rejection test. The tests now focus on integer coercion, factorial boundaries, and NULL handling.

…t cases

ShayanGho · 2026-09-04T22:24:53Z

Thank you @ShayanGho for working on this. I left few comments please take a look.

Thank you for checking my PR and running the CI

github-actions Bot added sqllogictest SQL Logic Tests (.slt) spark labels Sep 4, 2026

kumarUjjawal reviewed Sep 4, 2026

View reviewed changes

comphead reviewed Sep 4, 2026

View reviewed changes

Address review: use new_implicit_native, keep only Spark-verified tes…

ff3968e

…t cases

ShayanGho mentioned this pull request Sep 4, 2026

[Bug] spark factorial: BIGINT overflow should wrap in non-ANSI mode and raise CAST_OVERFLOW in ANSI mode, as in Spark #24950

Open

Uh oh!

Conversation

ShayanGho commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

Uh oh!

ShayanGho commented Sep 4, 2026

Uh oh!

kumarUjjawal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumarUjjawal commented Sep 4, 2026

Uh oh!

codecov-commenter commented Sep 4, 2026

Codecov Report

Uh oh!

comphead Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShayanGho commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ShayanGho commented Sep 4, 2026 •

edited

Loading

comphead Sep 4, 2026 •

edited

Loading