You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "Spenserrrr (via GitHub)" <gi...@apache.org> on 2026/08/26 18:28:06 UTC
[PR] [SPARK-58582][PS][FOLLOWUP] Keep integral operands in integer space for NumPy fmod [spark]
Spenserrrr opened a new pull request, #58319:
URL: https://github.com/apache/spark/pull/58319
### What changes were proposed in this pull request?
`_fmod_func` cast both operands to double before taking the remainder, including on the branch that is reached only when neither operand is floating. Integral operands now take the remainder as longs, and only the result is cast back to double. The floating branch is unchanged.
### 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 ever taken:
```python
>>> pdf = pd.DataFrame({"x1": [9007199254740993], "x2": [2]})
>>> np.fmod(pdf.x1, pdf.x2)[0] # pandas
1
>>> np.fmod(ps.from_pandas(pdf).x1, ps.from_pandas(pdf).x2).to_pandas()[0] # before
0.0
```
Every epoch-nanosecond value is past that boundary, since 2**53 nanoseconds is about 104 days. The same cast also gave an integral remainder a sign of zero it cannot have, because an IEEE remainder carries the dividend's sign: `-64 % 2` returned `-0.0` where NumPy returns `0`. The `pandas_udf` that SPARK-58582 replaced called NumPy directly, so this restores its results.
The floating branch is faithful as it stands: NumPy also promotes a mixed integral/floating pair to float64 and loses the same bits. A remainder above 2**53 is still rounded by the double return type, which requires a divisor above 2**53; the result is exact for any divisor up to it.
### Does this PR introduce _any_ user-facing change?
No change relative to a released version, since SPARK-58582 is not in a release branch. Within unreleased master, `np.fmod` on integral columns returns the exact remainder for operands above 2**53, and an integral remainder of zero is `0.0` rather than `-0.0`.
### How was this patch tested?
New rows in `test_np_fmod`: an integral frame above 2**53 compared exactly rather than approximately, since a relative tolerance accepts an off-by-one at those magnitudes, plus an explicit `np.signbit` assertion because `almost=True` treats `-0.0` and `0.0` as equal. Both were confirmed to fail without the change by reverting only `numpy_compat.py`.
Also ran the classic and Spark Connect parity suites for `test_numpy_compat`, and checked parity with pandas by hand for int8/int16/int32/int64, nullable `Int64` with nulls and zero divisors, mixed integral/floating pairs, `-2**63 % -1`, and decimal columns, which keep taking the double path.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58582][PS][FOLLOWUP] Keep integral operands in integer space for NumPy fmod [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #58319:
URL: https://github.com/apache/spark/pull/58319#discussion_r3867878897
##########
python/pyspark/pandas/numpy_compat.py:
##########
@@ -152,17 +152,29 @@ def _copysign_func(c1: Column, c2: Column) -> Column:
def _fmod_func(c1: Column, c2: Column) -> Column:
c1_double = c1.cast("double")
c2_double = c2.cast("double")
+ integral_types = ["tinyint", "smallint", "int", "bigint"]
+ # Dispatched on type twice: floating operands need a NaN for a zero divisor, and among the
+ # rest, at the end of the branch below, only integral operands can take the remainder in
+ # integer space.
return F.when(
F.typeof(c1).isin("float", "double") | F.typeof(c2).isin("float", "double"),
F.when(c1.isNull() | F.isnan(c1), c1_double)
.when(c2.isNull() | F.isnan(c2), c2_double)
.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.
Review Comment:
will numpy raise a error for types like string
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58582][PS][FOLLOWUP] Keep integral operands in integer space for NumPy fmod [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng closed pull request #58319: [SPARK-58582][PS][FOLLOWUP] Keep integral operands in integer space for NumPy fmod
URL: https://github.com/apache/spark/pull/58319
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58582][PS][FOLLOWUP] Keep integral operands in integer space for NumPy fmod [spark]
Posted by "Spenserrrr (via GitHub)" <gi...@apache.org>.
Spenserrrr commented on PR #58319:
URL: https://github.com/apache/spark/pull/58319#issuecomment-5429808497
Hi @zhengruifeng! This is a PR to fix the fmod function. The original `_fmod_func` cast both operands to double before `try_mod`, so an integral operand above 2**53 lost its low bits. Right now, the code takes the remainder as longs and only the result is cast back to double. Could you take a look when you have time?
While checking the rest of the table, I found the same double cast affects `trunc`, `fmax,` and `fmin`. For example, `np.trunc(9007199254740993)` gives 9007199254740992. I'll follow up on those separately unless you'd rather keep them out of scope. Thanks!
cc @Yicong-Huang
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58582][PS][FOLLOWUP] Keep integral operands in integer space for NumPy fmod [spark]
Posted by "Spenserrrr (via GitHub)" <gi...@apache.org>.
Spenserrrr commented on code in PR #58319:
URL: https://github.com/apache/spark/pull/58319#discussion_r3875186022
##########
python/pyspark/pandas/numpy_compat.py:
##########
@@ -152,17 +152,29 @@ def _copysign_func(c1: Column, c2: Column) -> Column:
def _fmod_func(c1: Column, c2: Column) -> Column:
c1_double = c1.cast("double")
c2_double = c2.cast("double")
+ integral_types = ["tinyint", "smallint", "int", "bigint"]
+ # Dispatched on type twice: floating operands need a NaN for a zero divisor, and among the
+ # rest, at the end of the branch below, only integral operands can take the remainder in
+ # integer space.
return F.when(
F.typeof(c1).isin("float", "double") | F.typeof(c2).isin("float", "double"),
F.when(c1.isNull() | F.isnan(c1), c1_double)
.when(c2.isNull() | F.isnan(c2), c2_double)
.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.
Review 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.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58582][PS][FOLLOWUP] Keep integral operands in integer space for NumPy fmod [spark]
Posted by "Spenserrrr (via GitHub)" <gi...@apache.org>.
Spenserrrr commented on code in PR #58319:
URL: https://github.com/apache/spark/pull/58319#discussion_r3876682978
##########
python/pyspark/pandas/numpy_compat.py:
##########
@@ -152,17 +152,29 @@ def _copysign_func(c1: Column, c2: Column) -> Column:
def _fmod_func(c1: Column, c2: Column) -> Column:
c1_double = c1.cast("double")
c2_double = c2.cast("double")
+ integral_types = ["tinyint", "smallint", "int", "bigint"]
+ # Dispatched on type twice: floating operands need a NaN for a zero divisor, and among the
+ # rest, at the end of the branch below, only integral operands can take the remainder in
+ # integer space.
return F.when(
F.typeof(c1).isin("float", "double") | F.typeof(c2).isin("float", "double"),
F.when(c1.isNull() | F.isnan(c1), c1_double)
.when(c2.isNull() | F.isnan(c2), c2_double)
.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.
Review 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.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58582][PS][FOLLOWUP] Keep integral operands in integer space for NumPy fmod [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #58319:
URL: https://github.com/apache/spark/pull/58319#issuecomment-5486994949
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/6374c2f83df8b83abade73b80008156972ee602a
- merged into branch-4.x https://github.com/apache/spark/commit/31241456933350b17d578ffcdaba3aae27f4129d
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org