Spenserrrr opened a new pull request, #58263: URL: https://github.com/apache/spark/pull/58263
### What changes were proposed in this pull request? Corrects and completes the comment on the `signbit` entry in `unary_np_spark_mappings`, which documented the null handling more narrowly than the expression actually behaves. No code change -- every changed line is a comment. Three fixes: - `non-double` -> `non-floating`: the guard checks both `float` and `double`. - The unmatched nullable case is any nullable float dtype (`Float32` or `Float64`), not just `Float64`. - Adds the negative-NaN case: the sign of a NaN never reaches the expression, so it reads `False` where `np.signbit` reports `True`. While investigating I found the nullable-float case is actually fixable, but the fix is broader than a comment change, so I would rather have your input before revising: - The blocker exists only inside the expression. `F.typeof(c)` sees the *Spark* type, and both `float64` and `Float64` map to `double`. The pandas dtype is still available upstream in `maybe_dispatch_ufunc_to_spark_func`, so the answer for a null could be chosen there instead. - It would be exact rather than heuristic. In a `Float64` column a Spark null can only be `<NA>` -- a real NaN keeps its mask clear and arrives as an actual NaN value -- and in a `float64` column a null can only be a NaN. - It would subsume the `typeof` guard entirely, and the same question applies to `isnan` / `isfinite` / `isinf` / `logical_not`: each hardcodes a different answer for a missing value today, so a principled fix looks table-wide rather than `signbit`-only. - It would not fix the NaN sign, which is destroyed before the expression runs. That half of the comment stands either way. Happy to follow up under a separate JIRA if you think that direction is right, or to fold it in here if you would rather not land the comment on its own. ### Why are the changes needed? The comment is the only place this behavior is written down, and it named two of the cases that reach the floating-null branch while more land there. A reader trusting it would conclude a nullable `Float32` `<NA>` propagates and that a negative NaN is reported as negative; neither is true. ### Does this PR introduce _any_ user-facing change? No. Comment-only. ### How was this patch tested? No behavior change to test: the diff is entirely comments, verified by checking that every added and removed line begins with `#`. Ran `python/pyspark/pandas/tests/test_numpy_compat.py` (`test_np_signbit` passes) plus `ruff check` and `ruff format --check` on the changed file. The cases described in the comment were confirmed against a live build: a `Float64` `<NA>` arrives as a Spark null while a computed NaN in the same dtype arrives as a real NaN value (`isnan` true, `isnull` false), and a default `float64` NaN arrives as a Spark null. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (2.1.241) -- 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]
