[SPARK-59091][SQL] Emit the NaN guard in transpiled equality only for floating-point columns - #58901
jzhan-2026 wants to merge 1 commit into
Conversation
|
@holdenk PTAL when you get a chance! Thanks in advance! |
holdenk
left a comment
holdenk
left a comment
There was a problem hiding this comment.
Great work, some questions :)
| # Categories that represent numeric (non-string) scalar types. | ||
| _NUMERIC_CATS = frozenset(("numeric", "integer", "float")) |
There was a problem hiding this 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.
| 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" |
There was a problem hiding this comment.
Makes sense, although for a little extra safety let's just raise or none on a non-numeric input col
| has_float = lc == "float" or rc == "float" | ||
| if has_float: |
There was a problem hiding this comment.
personally I'd move these two lines together but nit
2 participants
What changes were proposed in this pull request?
Split the transpiler's
"numeric"category into"integer"(IntegralType) and"float"(FractionalType, excluding DecimalType). The NaN guard in
_lower_eqis now only emittedwhen at least one operand has category
"float".transpile.py:_categoryreturns"integer"/"float"instead of"numeric"forliterals and parameters;
_param_category_combostries["integer", "float", "string"]per parameter;
_lower_eqgates the guard onlc == "float" or rc == "float";_convert_chunkBinOp dispatch uses_is_numeric_cat()for all three sub-categories.ResolveTranspiledPythonUDFOptions.scala: adds"integer"→ IntegralType and"float"→ FractionalType (excluding Decimal) cases to
optionMatchesTypes.Why are the changes needed?
For integer columns (e.g.
BIGINT), the old plan forx == yincludedisnan(cast(x AS DOUBLE)), which is always false—integers can't be NaN. The NaN guard only makes sense for floating-point types.Does this PR introduce any user-facing change?
No. The generated Catalyst plan for integer-column equality no longer contains dead
isnanguards, but the query results are identical.How was this patch tested?
New Python tests in
test_udf_transpile_unit:test_udf_transpile_integer_eq_no_nan_guard— noisnanin plan forBIGINT ==test_udf_transpile_float_eq_nan_semantics—isnanpresent forDOUBLE ==; NaN and null semantics verified end-to-endtest_udf_transpile_int_float_annotation_categories— unit tests for new category namesNew Scala tests in
ResolveTranspiledPythonUDFOptionsSuite:"integer"/"float"matching, cross-type rejection, DecimalType exclusion, FloatType matching.Was this patch authored or co-authored using generative AI tooling?
Co-authored by: Claude Sonnet 4.6