[SPARK-58581][PS][FOLLOWUP] Fix NumPy floor_divide rounding and integer precision - #58306
[SPARK-58581][PS][FOLLOWUP] Fix NumPy floor_divide rounding and integer precision#58306Spenserrrr wants to merge 4 commits into
Conversation
…er precision The native `floor_divide` mapping computed `floor(c1 / c2)`, which diverges from NumPy and pandas in two ways. Flooring the quotient is wrong when the division rounds up across an integer: `1.0 // 0.1` returned 10 where NumPy, pandas and Python return 9, and Spark's own `%` reports a remainder that no valid division leaves for that quotient. The floating branch now derives the quotient from the remainder, as npy_divmod does. Casting integral operands to double drops their low bits above 2**53, so `-9007199254740993 // 2` returned -4503599627370496 rather than -4503599627370497. Integral operands now divide in integer space, and only the result is cast back to double. The added test rows compare exactly rather than approximately, since a relative tolerance accepts an off-by-one at those magnitudes.
…ative long Integer division overflows for the most negative long divided by -1, the one quotient a long cannot hold, and Spark's `div` raises ARITHMETIC_OVERFLOW there. The mapping's earlier pandas UDF delegated to NumPy and so returned NumPy's wrapped value; the native conversion to a double divide returned it with the opposite sign instead. Guard that operand pair and return the wrapped value, which both restores the UDF's result and keeps the expression from raising.
…unrepresentable quotients and int64 above 2^53 ### What changes were proposed in this pull request? This PR fixes the four implementations in `python/pyspark/pandas/data_type_ops/num_ops.py`: - `IntegralOps.floordiv` + `.rfloordiv` - `FractionalOps.floordiv` + `.rfloordiv` by introducing two helper functions (`_floor_divide_floating` and `_floor_divide_integral`) that correctly compute `floor(c1 / c2)` using the remainder-based approach from NumPy's `npy_divmod` and integer-space division for int64. ### Why are the changes needed? The previous `F.floor(col / divisor)` approach produced incorrect results in two scenarios: 1. **Floating-point operands where the quotient rounds across an integer boundary** `ps.Series([1.0]) // 0.1` returned `10.0` instead of `9.0`. This is because `1.0 / 0.1` in IEEE-754 double is not exactly representable and rounds **up** to exactly `10.0`, so `floor(10.0) = 10`. NumPy and pandas use a remainder-based approach to avoid this. 2. **Int64 operands above 2^53** `ps.Series([-9007199254740993]) // 2` returned `-4503599627370496` instead of `-4503599627370497` (off by 1). This is because Spark's `/` always divides as double, casting int64 operands first. Values above 2^53 lose precision in the low bits during the cast, breaking the result. ### Does this PR introduce _any_ user-facing change? Yes. `floordiv` operations on pandas-on-Spark Series now match pandas/NumPy behavior for the two cases above. ### How was this patch tested? Added `python/pyspark/pandas/tests/data_type_ops/test_num_ops_floordiv_fix.py` with regression coverage for: - `1.0 // 0.1` and similar unrepresentable float quotients - Int64 above 2^53 precision - The `-(2**63) // -1` overflow case - Both forward and reverse (`rfloordiv`) operations The helpers are adapted from draft PR apache#58306, which fixed the same bugs in `numpy_compat._floor_divide_func` but deliberately left the `num_ops.py` paths unchanged. That PR's author noted the `num_ops.py` defects "deserve their own JIRA", which is now apache#58307. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Sonnet 4)
…unrepresentable quotients and int64 above 2^53 ### What changes were proposed in this pull request? This PR fixes the four implementations in `python/pyspark/pandas/data_type_ops/num_ops.py`: - `IntegralOps.floordiv` + `.rfloordiv` - `FractionalOps.floordiv` + `.rfloordiv` by introducing two helper functions (`_floor_divide_floating` and `_floor_divide_integral`) that correctly compute `floor(c1 / c2)` using the remainder-based approach from NumPy's `npy_divmod` and integer-space division for int64. ### Why are the changes needed? The previous `F.floor(col / divisor)` approach produced incorrect results in two scenarios: 1. **Floating-point operands where the quotient rounds across an integer boundary** `ps.Series([1.0]) // 0.1` returned `10.0` instead of `9.0`. This is because `1.0 / 0.1` in IEEE-754 double is not exactly representable and rounds **up** to exactly `10.0`, so `floor(10.0) = 10`. NumPy and pandas use a remainder-based approach to avoid this. 2. **Int64 operands above 2^53** `ps.Series([-9007199254740993]) // 2` returned `-4503599627370496` instead of `-4503599627370497` (off by 1). This is because Spark's `/` always divides as double, casting int64 operands first. Values above 2^53 lose precision in the low bits during the cast, breaking the result. ### Does this PR introduce _any_ user-facing change? Yes. `floordiv` operations on pandas-on-Spark Series now match pandas/NumPy behavior for the two cases above. ### How was this patch tested? Added `python/pyspark/pandas/tests/data_type_ops/test_num_ops_floordiv_fix.py` with regression coverage for: - `1.0 // 0.1` and similar unrepresentable float quotients - Int64 above 2^53 precision - The `-(2**63) // -1` overflow case - Both forward and reverse (`rfloordiv`) operations The helpers are adapted from draft PR apache#58306, which fixed the same bugs in `numpy_compat._floor_divide_func` but deliberately left the `num_ops.py` paths unchanged. That PR's author noted the `num_ops.py` defects "deserve their own JIRA", which is now apache#58307. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Sonnet 4)
|
Hi @zhengruifeng! This is a PR fixing the One thing is worth your attention. The mapping I fixed here is the unreachable one, since |
A quotient that overflows to an infinity, such as `1e300 // 1e-300`, came back as nan. The floating branch emulates the floor with `q - pmod(q, 1.0)`, because `F.floor` returns a bigint and raises on an infinity, and pmod of an infinity is nan, so that subtraction left nan behind. An infinite quotient is its own floor, so it is now returned before that step. NumPy keeps the infinity here. The formula this PR replaced returned nan as well, so the case is not a regression from the earlier commits.
| c2_double = c2.cast("double") | ||
| integral_types = ["tinyint", "smallint", "int", "bigint"] | ||
|
|
||
| # Dispatched on type twice: floating operands need IEEE answers for infinities and signed |
There was a problem hiding this comment.
what about dispatching earlier by datatype?
when(is int, _floor_divide_integral)
.when(is float, _floor_divide_floating)
.otherwise
There was a problem hiding this comment.
Thanks for the advice @zhengruifeng! I originally choose to write it nested to keep the diff small. I revise the structure in the new commit. It is now a single chain:
- null/nan and a zero divisor first, since they answer the same way for every operand type;
- when(both integral) -> _floor_divide_integral;
- when(either floating) -> the infinity and signed-zero cases, then _floor_divide_floating;
- otherwise is decimal
All the existing tests still pass, so the refactor is behaviour-preserving.
Flatten the two-level dispatch in `_floor_divide_func` into one chain, as suggested in review. The null and zero-divisor cases answer the same way for every operand type, so they precede the type tests; the integral and floating branches then sit side by side, and a decimal falls through to the floating helper since `typeof` reports its precision and cannot be matched by name. Behaviour-preserving: the two spellings of the zero-divisor answer collapse into one, because a non-floating zero can never be negative. Verified against the previous version on 6361 rows covering every combination of double, float, bigint, int, tinyint, smallint and decimal operands, including negative zero divisors, infinities, values above 2**53 and -2**63 // -1.
…er precision ### What changes were proposed in this pull request? The native `floor_divide` mapping computes `floor(c1 / c2)`. That gives a wrong answer in two cases, both fixed here. - **Floating operands.** `1.0 // 0.1` returned 10, where pandas, NumPy and Python all give 9. The exact quotient is a little under 10, and the division rounds it up before the floor ever runs. NumPy works the quotient out from the remainder instead, which is exact in IEEE arithmetic, so the floating branch now does the same, including NumPy's sign correction and snap-to-nearest. - **Integral operands.** They were cast to double before dividing, which drops the low bits above 2^53, so `-9007199254740993 // 2` came back as `-4503599627370496` instead of `-4503599627370497`. They now divide in integer space, and only the result is cast. A second commit handles `-2**63 // -1`, the one quotient a long cannot hold. Spark's integer division raises `ARITHMETIC_OVERFLOW` there, so that pair is guarded and returns NumPy's wrapped value, which is what this mapping's original pandas UDF produced. A third commit covers a quotient that overflows to an infinity, which the old and new formulas both returned as `nan`. NumPy keeps the infinity, and an infinite quotient is its own floor, so `1e300 // 1e-300` now skips the flooring step. ### Why are the changes needed? Both answers disagree with pandas, which pandas-on-Spark is meant to match. The `1.0 // 0.1` case is also inconsistent inside Spark itself. `1.0 % 0.1` gives `0.09999999999999995`, and a remainder that large only makes sense if the quotient is 9: with a quotient of 10 there would be nothing left over. ### Does this PR introduce _any_ user-facing change? No. `np.floor_divide` goes through the pandas-on-Spark `floordiv` dunder operation before this registry is consulted, so the mapping is not reached today, as the comment on the entry notes. **A follow-up PR will cover the reachable path.** The same two bugs are live in `data_type_ops/num_ops.py` (`IntegralOps.floordiv`, `FractionalOps.floordiv` and both `rfloordiv`), which use `F.floor(lc / rc)`: `ps.Series([1.0]) // 0.1` gives `10.0` where pandas gives `9.0`, and `psser // 2` is off by one above 2^53. That one does change what users see on a long-standing path, so it gets its own JIRA and PR rather than riding along here. ### How was this patch tested? Added three cases to `test_floor_divide_func`: divisors that binary cannot represent exactly, integral operands above 2^53, and `-2**63 // -1`. They compare exactly rather than with `almost=True`, which would let an off-by-one through at these magnitudes. Ran `pyspark.pandas.tests.test_numpy_compat` and `pyspark.pandas.tests.connect.test_parity_numpy_compat`; both pass. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5) Closes #58306 from Spenserrrr/numpy-floordiv-precision. Authored-by: Spenser Sun <hsun112358@gmail.com> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org> (cherry picked from commit 670de9d) Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>
What changes were proposed in this pull request?
The native
floor_dividemapping computesfloor(c1 / c2). That gives a wrong answer in two cases, both fixed here.1.0 // 0.1returned 10, where pandas, NumPy and Python all give 9. The exact quotient is a little under 10, and the division rounds it up before the floor ever runs. NumPy works the quotient out from the remainder instead, which is exact in IEEE arithmetic, so the floating branch now does the same, including NumPy's sign correction and snap-to-nearest.-9007199254740993 // 2came back as-4503599627370496instead of-4503599627370497. They now divide in integer space, and only the result is cast.A second commit handles
-2**63 // -1, the one quotient a long cannot hold. Spark's integer division raisesARITHMETIC_OVERFLOWthere, so that pair is guarded and returns NumPy's wrapped value, which is what this mapping's original pandas UDF produced.A third commit covers a quotient that overflows to an infinity, which the old and new formulas both returned as
nan. NumPy keeps the infinity, and an infinite quotient is its own floor, so1e300 // 1e-300now skips the flooring step.Why are the changes needed?
Both answers disagree with pandas, which pandas-on-Spark is meant to match.
The
1.0 // 0.1case is also inconsistent inside Spark itself.1.0 % 0.1gives0.09999999999999995, and a remainder that large only makes sense if the quotient is 9: with a quotient of 10 there would be nothing left over.Does this PR introduce any user-facing change?
No.
np.floor_dividegoes through the pandas-on-Sparkfloordivdunder operation before this registry is consulted, so the mapping is not reached today, as the comment on the entry notes.A follow-up PR will cover the reachable path. The same two bugs are live in
data_type_ops/num_ops.py(IntegralOps.floordiv,FractionalOps.floordivand bothrfloordiv), which useF.floor(lc / rc):ps.Series([1.0]) // 0.1gives10.0where pandas gives9.0, andpsser // 2is off by one above 2^53. That one does change what users see on a long-standing path, so it gets its own JIRA and PR rather than riding along here.How was this patch tested?
Added three cases to
test_floor_divide_func: divisors that binary cannot represent exactly, integral operands above 2^53, and-2**63 // -1. They compare exactly rather than withalmost=True, which would let an off-by-one through at these magnitudes.Ran
pyspark.pandas.tests.test_numpy_compatandpyspark.pandas.tests.connect.test_parity_numpy_compat; both pass.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)