ShayanGho opened a new pull request, #24943:
URL: https://github.com/apache/datafusion/pull/24943
## Which issue does this PR close?
- Closes #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 four blocks: one
aliased row covering the untyped literal, TINYINT, SMALLINT, BIGINT, an
unsigned width and a bare
NULL; an array-shaped BIGINT query covering a negative value, both range
boundaries, an
above-range value and NULL; and two `query error` cases pinning the
boundaries (a BIGINT outside
the i32 range fails at the cast; a STRING argument is rejected, linking
#24941). Expected values
come 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, and its ANSI-dependent cast
behaviour, are
captured in #24941. The `query error` test for a STRING argument links
that issue, so the fix for
it will surface here when the behaviour changes.
## What is the testing strategy for this PR?
Yes. The new `.slt` queries fail on `main` and pass with this change,
verified by stashing only the
implementation file and re-running `cargo test --test sqllogictests --
spark/math/factorial`:
- stashed (old signature): `4 errors in file ... factorial.slt`. The two
value blocks fail with
`coercion from Int64 to the signature Exact(Int32) failed`; the two `query
error` blocks fail
because the old signature produces a different message than the one
asserted.
- restored: `Completed 1 test files in 0 seconds`.
`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](https://claude.com/claude-code)
--
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]
