[SPARK-58582][PS][FOLLOWUP] Keep integral operands in integer space for NumPy fmod - #58319
[SPARK-58582][PS][FOLLOWUP] Keep integral operands in integer space for NumPy fmod#58319Spenserrrr wants to merge 2 commits into
Conversation
…or NumPy fmod The native `fmod` mapping cast both operands to double before taking the remainder, including on the branch reached only when neither operand is floating. A double carries 53 bits of mantissa, so an integral operand above 2**53 was rounded before the remainder was ever taken: `9007199254740993 % 2` returned 0 where NumPy and pandas return 1. Every epoch-nanosecond value is past that boundary. The same cast also gave an integral remainder a sign of zero it cannot have, since an IEEE remainder carries the dividend's sign: `-64 % 2` returned -0.0 where NumPy returns 0. Integral operands now take the remainder as longs, and only the result is cast back to double. The floating branch is unchanged, and it is faithful for a mixed pair because NumPy promotes to float64 there and loses the same bits. A remainder above 2**53 is still rounded by the double return type, which needs a divisor above 2**53; the result is exact for any divisor up to it. The added test rows compare exactly rather than approximately, since a relative tolerance accepts an off-by-one at those magnitudes, and an explicit signbit assertion covers the sign of zero, which `almost=True` cannot see.
|
Hi @zhengruifeng! This is a PR to fix the fmod function. The original While checking the rest of the table, I found the same double cast affects |
| .when(c2_double == 0, F.lit(float("nan"))) | ||
| .otherwise(F.try_mod(c1_double, c2_double)), | ||
| ).otherwise( | ||
| # Non-floating operands, where NumPy normalizes a zero divisor to 0 instead of a NaN. |
There was a problem hiding this comment.
will numpy raise a error for types like string
There was a problem hiding this comment.
NumPy always raises on a string column. Ours raises only conditionally: ["7", "8"] computes and returns [1.0, 2.0], while ["7", "abc"] fails at runtime with CAST_INVALID_INPUT, because we cast columns to double at the start of the helper function. This PR does not change this part.
Note that the current mapping table doesn't validates dtypes so each entry just accepts whatever Spark can cast. If we want to fix this, we may need to add dtype check in the mapping table.
There was a problem hiding this comment.
I also flattened the dispatch here to match your suggestion on #58306, with one branch per operand type.
One thing I noticed while doing it: NumPy raises on Decimal objects too, so the decimal branch has no NumPy behavior to match. I kept your original answer there, which is 0 for a zero divisor and the remainder in double. I think fixing it properly needs the same dtype check as the string case.
The mapping dispatched on whether either operand was floating and then repeated the null and nan rungs inside both branches. It now dispatches on the operand type directly, with one branch each for integral, floating and decimal operands, so those rungs are written once and each branch carries its own zero divisor answer. Behavior is unchanged. The old and new expressions were compared row by row with both results cast to string, so -0.0, NaN and Infinity compare exactly: 851 rows over twelve dtype combinations, covering double specials, random doubles, integral edges above 2**53, narrow integers, float32, mixed integral and floating pairs, and decimals. String, boolean, timestamp and date columns were checked separately and are unchanged too.
2 participants
What changes were proposed in this pull request?
_fmod_funccast both operands to double before taking the remainder, including on the branch reached only when neither operand is floating. Integral operands now take the remainder as longs, and only the result is cast back to double.Why are the changes needed?
A double carries 53 bits of mantissa, so an integral operand above 2**53 was rounded before the remainder was taken:
np.fmod(9007199254740993, 2)gave0.0where pandas gives1. The same cast also returned-0.0for-64 % 2, a sign of zero an integral remainder cannot have. Thepandas_udfthis mapping replaced called NumPy directly and was exact.Does this PR introduce any user-facing change?
No.
How was this patch tested?
New rows in
test_np_fmodfor integral operands above 2**53, compared exactly since a relative tolerance accepts an off-by-one at those magnitudes, plus annp.signbitassertion for the sign of zero. Both fail without the change.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)