holdenk commented on code in PR #58901:
URL: https://github.com/apache/spark/pull/58901#discussion_r4067552366
##########
python/pyspark/sql/transpile.py:
##########
@@ -81,6 +82,23 @@
from pyspark.sql import SparkSession
from pyspark.sql._typing import DataTypeOrString
+# Categories that represent numeric (non-string) scalar types.
+_NUMERIC_CATS = frozenset(("numeric", "integer", "float"))
Review Comment:
So as it's written down bellow numeric is "an unknown numeric type" but
elsewhere we treat numeric as the tightest of them. This kind of changes the
logic needed in both the widening sub function and the comparison check
function (if it's unknown then we should have the NaN check there too for
numeric) and then also numeric should be the widest not least wide.
##########
python/pyspark/sql/transpile.py:
##########
@@ -81,6 +82,23 @@
from pyspark.sql import SparkSession
from pyspark.sql._typing import DataTypeOrString
+# Categories that represent numeric (non-string) scalar types.
+_NUMERIC_CATS = frozenset(("numeric", "integer", "float"))
+
+
+def _is_numeric_cat(cat: str) -> bool:
+ """True for any numeric sub-category (``"numeric"``, ``"integer"``,
``"float"``)."""
+ return cat in _NUMERIC_CATS
+
+
+def _wider_numeric(lc: str, rc: str) -> str:
+ """Return the wider of two numeric categories (float > integer >
numeric)."""
+ if "float" in (lc, rc):
+ return "float"
+ if "integer" in (lc, rc):
+ return "integer"
+ return "numeric"
Review Comment:
Makes sense, although for a little extra safety let's just raise or none on
a non-numeric input col
##########
python/pyspark/sql/transpile.py:
##########
@@ -294,31 +322,53 @@ def _lower_eq(
where Python's ``==`` is simply False. Refuse those so the UDF falls
back to interpreted Python. A ``None`` literal operand stays allowed
(the four-branch NULL handling above reproduces Python exactly).
+ Numeric sub-categories (``"integer"`` vs ``"float"``) are treated as
+ compatible; Spark widens to double in the comparison.
- One value-level difference remains (needs runtime values, so it is
- documented, not guarded): Spark treats ``NaN = NaN`` as true, while
- Python's ``nan == nan`` is False.
+ For floating-point operands a NaN guard is added: ``NaN == NaN`` is
+ ``False`` in Python (IEEE 754), but Spark's ``EqualTo`` returns
+ ``True`` for ``NaN = NaN``. When both operands are provably integral
+ the guard is skipped because integers cannot be NaN.
+
+ One value-level difference remains for ordering comparisons (tracked
+ separately): Spark orders NaN as greater than every value, whereas
+ Python's NaN comparisons are all False.
"""
lc = self._safe_category(params, left_node)
rc = self._safe_category(params, right_node)
if lc is not None and rc is not None and lc != rc:
- raise UnsupportedOperationException(
- f"`==`/`!=` operands have incompatible categories ({lc} vs
{rc}); "
- "Python compares across types as unequal while Spark would
coerce "
- "or fail analysis, so the transpiler falls back to interpreted
Python"
- )
+ # Numeric sub-categories are mutually compatible for equality.
+ if not (_is_numeric_cat(lc) and _is_numeric_cat(rc)):
+ raise UnsupportedOperationException(
+ f"`==`/`!=` operands have incompatible categories ({lc} vs
{rc}); "
+ "Python compares across types as unequal while Spark would
coerce "
+ "or fail analysis, so the transpiler falls back to
interpreted Python"
+ )
left_col = self._convert_chunk(params, left_node)
right_col = self._convert_chunk(params, right_node)
left_null = left_col.isNull()
right_null = right_col.isNull()
+ # NaN guard: Python's `NaN == NaN` is False (IEEE 754 reflexivity
fails),
+ # but Spark's EqualTo returns True. Only emit when at least one operand
+ # is "float" -- the "float" variant is only selected for FractionalType
+ # columns (see ResolveTranspiledPythonUDFOptions), so isnan() is safe
to
+ # call without a cast. For "integer" and "numeric" (integral columns)
+ # NaN is impossible and the guard is dead code.
+ has_float = lc == "float" or rc == "float"
+ if has_float:
Review Comment:
personally I'd move these two lines together but nit
--
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]
