[WIP][SPARK-55206][PYTHON][SQL] Transpilation minimal functional implementation with python#56327
Conversation
|
CC @devin-petersohn would love your input and review, I know it's still in draft phase though. I'm going to try and finish it up over the coming week |
|
I have a question - how correct and maintainable we want this to be? Python Also, do we expect the users to "try it out" and see if it kind of works, or we aim to be "always correct or raising an error"? There's a huge difference. For example: case ast.Compare():
# All comparison operators produce bool.
return TrueNope, case ast.BoolOp():
# and / or of booleans produces bool.
return TrueNope, The non-boolean check for operations are not fully valid either - any user defined type can return a Like I mentioned when this SPIP was firstly discussed - Python is a super dynamic language and having a transpiler that is always correct on where it claims to be correct is not easy. So I want to confirm the purpose of the SPIP - do we want to be always correct, or correct most of the time to cover more transpilable UDFs, with the cost that we could produce wrong result? |
|
For ast module changes I think we're ok with just not transpiling new / different items. For the comparison operators right now we're limiting the UDF transpilation logic to scalar inputs we can scope it down more to also exclude non scalar captures (and expand back out later if we want for arrow / pandas types but handle them explicitly). To be clear this is. WIP PR but was talking with Scott about this in the context of some of his work so I wanted to raise the current draft for folks while |
holdenk
left a comment
There was a problem hiding this comment.
Left some notes after going through this my self for some things to fix before I switch it to regular from WIP but it feels close
| # 64-bit signed range, kept clear of overflow so arithmetic in both | ||
| # Python and Spark stays in the safe LongType range. | ||
| _LONG_BOUND = 2**31 - 1 |
There was a problem hiding this comment.
Range mismatch between text and impl
| # value -- e.g. NULL, zero, the type's max -- deterministic across | ||
| # runs. These are the values we always want to try, before random | ||
| # generation kicks in. | ||
| _LONG_EDGES = (None, 0, 1, -1, 7, -7, _LONG_BOUND, -_LONG_BOUND) |
There was a problem hiding this comment.
Let's add int32 bounds too
| # Multi-arg edges -- nulls plus the four sign-combos for non-zero | ||
| # values. Catches off-by-one errors in parameter-index plumbing | ||
| # better than random generation alone. | ||
| _LONG_PAIR_EDGES = ( |
There was a problem hiding this comment.
Let's add int32 bounds too
| # Sign-combo edges (plus NULL combinations) for the boolean tests. | ||
| # The bodies (``x > 0 and y > 0`` / ``x > 0 or y > 0``) raise in | ||
| # pure Python on a None input (``TypeError``), and the transpiler's | ||
| # NULL-guarded Compare also raises -- so the ``_run`` helper's "both | ||
| # raised" equivalence covers the NULL cases here. | ||
| _BOOLEAN_PAIR_EDGES = ( | ||
| (None, None), | ||
| (None, 0), | ||
| (0, None), | ||
| (0, 0), | ||
| (1, -1), | ||
| (-1, 1), | ||
| (1, 1), | ||
| (-1, -1), | ||
| (_LONG_BOUND, 1), | ||
| (1, -_LONG_BOUND), | ||
| ) |
There was a problem hiding this comment.
Let's also add str/int compare and we should verify we throw an error Scala and Python side respectively.
| def plus_four(x): | ||
| if x is not None: | ||
| return x + 4 |
There was a problem hiding this comment.
Let's also add plus_four unsafe where we don't check None
| # A transpiled rewrite replaces the (now nondeterministic) Python UDF | ||
| # with a plain Catalyst expression, which the optimizer is free to | ||
| # fold, reorder, or duplicate -- discarding the nondeterminism barrier | ||
| # the caller just asked for. Drop any transpiled options so a | ||
| # nondeterministic UDF always runs as interpreted Python. | ||
| self.transpiled = [] | ||
| self._transpiled_param_names = [] |
There was a problem hiding this comment.
The more I think about this the less certain I am, we could (for example) have a non-deterministic UDF calling rand and transpile it into the sql rand / randn function but lets do that as a follow up.
Co-authored-by: Holden Karau <holden@pigscanfly.ca>
…file Co-authored-by: Holden Karau <holden@pigscanfly.ca>
… class but reset the python worker logs between Co-authored-by: Holden Karau <holden@pigscanfly.ca>
Co-authored-by: Holden Karau <holden@pigscanfly.ca>
… and transpialtion subsequently disabled or ANSI mode swapped and instead just ignore all of the transpiled elems Co-authored-by: Holden Karau <holden@pigscanfly.ca>
Co-authored-by: Holden Karau <holden@pigscanfly.ca>
…s still there or not. Co-authored-by: Holden Karau <holden@pigscanfly.ca>
Co-authored-by: Holden Karau <holden@pigscanfly.ca>
* [SPARK][PYTHON] Refuse to lower non-boolean and/or/not in UDF transpiler; fix Column truthiness in transpile path; surface column in CANNOT_CONVERT_COLUMN_INTO_BOOL The UDF transpiler previously lowered Python `and` / `or` / `not` to Spark's bitwise `&` / `|` / `~` regardless of operand type. For non-bool operands (e.g. `x or 0` against an int column) this silently diverges from Python's truthiness semantics. Detect statically-non-boolean operands (numeric / string literals, arithmetic BinOps, USub/UAdd, IfExp with both arms non-boolean) and refuse to lower; the UDF then falls back to interpreted Python. Also: - Fix `if transpiled_column:` triggering CANNOT_CONVERT_COLUMN_INTO_BOOL during transpilation (which broke test_udf_transpile_basic). Use `is not None` instead. - Include the offending column's repr in the CANNOT_CONVERT_COLUMN_INTO_BOOL error so users can tell which column triggered it. - Add unit tests for boolean and/or transpilation, non-boolean fallback, and the improved error message; add hypothesis tests for both_positive / either_positive. * Fix ruff F841 and CI test failures in transpiler boolean tests - Remove leftover unused ``transpiled_param_names`` local in udf.py (the class attribute ``self._transpiled_param_names`` already covers this; the local was a leftover from an earlier refactor). - Rewrite the unit-test UDFs ``both_positive`` / ``either_positive`` / the matching hypothesis UDFs as single-statement bodies so the transpiler (which only handles one top-level statement) actually lowers them. The previous form short-circuited out before the BoolOp path was exercised. - Stop relying on schema inference for None-only Row inputs in ``test_udf_transpile_falls_back_for_non_boolean_short_circuit``. Pass an explicit LongType schema so ``createDataFrame`` doesn't fail with CANNOT_DETERMINE_TYPE on the ``or_zero_none`` case. - Use a non-null long strategy for the boolean hypothesis tests since the interpreted Python body would raise on a None operand. * Fix ruff format issues in udf.py and test_udf_transpile_unit.py The previous lint failure was masked by ruff check failing first (F841 unused variable). With check passing, ruff format now flags the surrounding region: a stray blank line in the unit test file and a multi-line conditional in udf.py that fits on one line. Apply ruff format auto-fix. * Support comparison operators in transpiler; fix mypy FunctionDef typing Two CI fixes: 1. The new ``test_udf_transpile_boolean_and_or_lowered`` test exercises ``x > 0 and y > 0``, but the transpiler's Compare handler only covered ``Is`` / ``IsNot``. Add Eq, NotEq, Lt, LtE, Gt, GtE so the bool-typed and/or path actually transpiles instead of bailing on the first comparison. Drop the now-stale ``less_than_zero`` case from the ``falls_back_for_unsupported_patterns`` matrix. 2. mypy 1.8.0 (CI) couldn't pick the right ``ast.FunctionDef`` overload from kwargs when ``decorator_list=[]`` was an unannotated empty literal (it inferred ``list[Never]``). Annotate the intermediate list locals so the call resolves to the documented overload. * Drop ast.FunctionDef call to Any to dodge mypy overload mismatch The typed overloads for ``ast.FunctionDef`` in mypy 1.8.0's typeshed require keyword-only ``type_params`` on Python 3.12+, but that field isn't accepted at runtime on every supported Python (it was added in 3.12, before that the constructor rejects it). Setting the constructor to ``Any`` keeps the runtime behaviour unchanged while sidestepping the typeshed overload resolution that the previous typed-locals approach couldn't satisfy. * Raise on NULL in transpiled value comparisons; restore lt test and exercise None Python raises ``TypeError`` when an operand of ``<``, ``<=``, ``>``, ``>=``, ``==`` or ``!=`` is ``None``, but Spark's three-valued logic silently returns ``NULL`` -- a silent semantic divergence. The transpiler now wraps these comparisons with ``when(left.isNull() | right.isNull(), raise_error(...)).otherwise(...)`` so the rewritten plan fails loudly to match the Python source. Code that guards against NULL upstream (``if x is not None: x > 0``) hits the otherwise branch and is unaffected. Tests: - Unit: ``test_udf_transpile_less_than_zero`` restores the ``Lt`` case as a positive transpile assertion (previously in the unsupported-patterns matrix); ``test_udf_transpile_compare_with_none_raises`` pins the new raise behaviour for unguarded comparisons. - Hypothesis: ``_run`` now treats "both sides raised" as equivalent so the boolean hypothesis tests (``both_positive`` / ``either_positive``) can be re-armed with the full nullable ``_long_strategy`` and a ``_BOOLEAN_PAIR_EDGES`` tuple that explicitly seeds NULL combos. * Address Copilot review comments - ``_lower_eq``: Python's ``None == x`` / ``None != x`` returns a bool, not a TypeError. Stop routing ``ast.Eq`` / ``ast.NotEq`` through the raise-on-NULL guard and instead emit the four ``when`` branches matching Python's equality semantics (both NULL -> True/False, one NULL -> False/True, otherwise -> ``==`` / ``!=``). The ``raise_error`` guard now only fires on ordering comparisons (``<``, ``<=``, ``>``, ``>=``). - ``_is_definitely_non_boolean``: narrow the ``ast.BinOp`` match from a bare catch-all to the arithmetic / shift operators we actually want to flag. Bitwise ``&`` / ``|`` / ``^`` are deliberately left out so e.g. ``not ((x > 0) & (y > 0))`` keeps being recognised as boolean. - ``Column.__nonzero__`` (connect): use ``repr(self._expr)`` rather than the dunder directly, in keeping with the standard API. - Hoist ``_BOOLEAN_PAIR_EDGES`` to module-level alongside ``_LONG_PAIR_EDGES`` so the decorator references a stable module attribute rather than a class-body local. --------- Co-authored-by: Claude <noreply@anthropic.com>
…improve warnings. Done with Claude
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…l in Scala for parent is udf and reduce scope remove some intermedite vals for debugging we don't need anymore, and use MDC logging.
The field was populated in five places but never read; callers already receive the same information via warnings.warn(). Removing it eliminates write-only state with no observable effect. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Expression objects in a logical plan should never be null; the find(expr => expr != null) pattern implied nulls were expected. Replacing it with headOption makes the intent clearer and removes the misleading guard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TranspiledUDFParityTests used the Python 2-style super(BaseUDFTestsMixin, cls).setUpClass() which was inconsistent with the other two parity classes that call ReusedSQLTestCase.setUpClass() directly. All three now use the same explicit form. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
conf.set() requires strings; add a comment explaining the difference from the unit tests which use Python True/False via sql_conf(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
transpiledOptions and optionInputCategories must have the same length (or the latter must be empty to mean "no restriction"). Adding a require() makes violations fail immediately at construction instead of producing confusing behaviour during analysis. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Six two-argument test methods inlined identical StructType([LongType, LongType]) construction. Extracting it to _two_long_arg_df (mirroring the existing _single_arg_df) removes the repetition and makes test bodies easier to read. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous bound of 2^31-1 was overly conservative and the comment incorrectly said "64-bit range". The actual 64-bit max is now used so Hypothesis can find overflow-boundary behaviour. Arithmetic UDFs (plus_four, times_three, etc.) may see values where both the transpiled Spark path and the interpreted Python UDF path raise; _run's "both raised" equivalence handles that case. A test failure here would indicate a real asymmetry in overflow handling between the two paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sign() returns DoubleType, so sign(b) * a promoted a to Double and lost integer precision for LongType values near 2^63. For example, -9223372036854775807 rounded to -9223372036854775808 as float64, shifting the mod result from 0 to 2. Replace sign() with a CASE-based integer sign expression so all arithmetic stays in LongType and avoids the implicit Double promotion. Remove the now-unused sign import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Expanding to 64-bit (_LONG_BOUND = 2**63 - 1) revealed that arithmetic UDFs (plus_four, times_three, add_then_mod, etc.) can generate inputs where Spark ANSI mode raises ArithmeticException on overflow while the Python UDF runner silently wraps the out-of-range return value -- a genuine semantic mismatch that the "both raised" equivalence cannot bridge. Two changes: 1. Add _LONG_ARITH_BOUND = 2**61 and a matching _long_arith_strategy for arithmetic tests. 2**61 is safe for +7, -2, *3, and two-arg addition without triggering LongType overflow. 2. Pin ANSI=True in the interpreted path of _run so both sides use the same overflow semantics (previously the interpreted path inherited the ambient session default, which is False by default). Comparison and equality tests (both_positive, eq_pair, is_none_branch, etc.) continue to use the full 2**63-1 range since they involve no arithmetic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…instead of v and Names are currently resolved to basic types
…ivergences (#34) * [SPARK-55206][PYTHON] Self-review fixes for Python UDF transpilation Catch correctness errors and tidy the minimal transpilation implementation without expanding scope: - transpile.py: refuse to lower an if/ternary whose two branches resolve to different input-type categories (e.g. numeric vs string). The lowered CASE WHEN has no common type under ANSI and, being a child of the TranspiledPythonUDF, was type-checked by CheckAnalysis before ConvertToCatalyst could drop it -- failing the query instead of falling back to interpreted Python. Adds a `_safe_category` helper that treats a bare `None` branch (which unifies with any type) and boolean-producing branches correctly, so homogeneous and `else None` cases still transpile. - UserDefinedPythonFunction.scala: apply the `_udf_param_N` placeholder resolution only to the transpiled options, never to the original UDF's argument subtree, so a user column literally named `_udf_param_N` is not silently rewritten to another argument. - transpile.py: remove the dead `_is_definitely_non_boolean` helper (never called; gating uses `_is_definitely_boolean`) and fix a stale config-key reference in a comment (`optimizer.transpilers` -> `optimizer.pyTranspilers`). - transpile.py: correct the modulo comment -- the sign/pmod rewrite matches Python for every non-zero divisor except at the LongType overflow boundaries, where it raises ARITHMETIC_OVERFLOW under ANSI. - udf.py: also reset `_transpiled_input_categories` in the transpile-failure handler for consistency with the other option lists. - Replace stray non-ASCII typographic characters in comments with ASCII. - Add a unit test covering mismatched-branch fallback plus a homogeneous positive control; fix the hypothesis test's stale default-example doc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GibQDjXJ5jmUhA1yXAxFUr * [SPARK-55206][PYTHON] Exercise the If-statement path in mismatch-branch test Make the `mixed_if` case a single `if`/`else` so the transpiler's If-statement lowering (and the new branch-category guard) is actually exercised, rather than falling back on the "more than one top-level statement" rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GibQDjXJ5jmUhA1yXAxFUr * [SPARK-55206][PYTHON] Fix ruff format violations failing CI lint The "Linters, licenses, and dependencies" job fails at the ruff format check (`ruff format --diff python/pyspark ...`): - transpile.py: two top-level functions were separated from the preceding block by a single blank line instead of two. - test_udf_transpile_unit.py: a createDataFrame call was wrapped across three lines where it fits on one. Applied `ruff format` (0.14.8, the version pinned in dev/requirements.txt) to both files; `ruff check` and `ruff format --diff` over the CI paths (python/pyspark dev python/packaging python/benchmarks) now pass, and the mypy annotations check reports no errors in the feature files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GibQDjXJ5jmUhA1yXAxFUr * [SPARK-55206][PYTHON] Make RUN_HYPOTHESIS gate value-based to match CI wiring build_and_test.yml sets RUN_HYPOTHESIS at the pyspark job level to the string "true" or "false" from the transpile precondition, so the env var is always PRESENT. The hypothesis suite's gate was presence-based ("any value (including empty) opts in"), which meant RUN_HYPOTHESIS=false still enabled the very slow suite on every PySpark job -- the opposite of the gating intent. It doesn't show on this branch (transpile=true here), but after merging, every unrelated PR would silently pay the cost. Switch the gate to value-based (1/true/yes, case-insensitive), update the module docstring and skip reason, and pin the contract from both directions with an always-running gating smoke test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GibQDjXJ5jmUhA1yXAxFUr * [SPARK-55206][PYTHON][SQL] Fix correctness bugs found in whole-feature review of UDF transpilation Adjudicated a full review of the transpilation feature (26 files vs master); each fix below was reproduced against a real Spark build before changing code, and has a regression test. Never-break-a-working-UDF fixes: - transpile.py: the branch-category guard now sees through ast.Return wrappers. `if x > 0: return x > 5 else: return 1` previously slipped past the guard and failed analysis (CASE WHEN [BOOLEAN, INT]) instead of falling back. - transpile.py: `==`/`!=` operands are category-gated like ordering comparisons. `x == True` on a numeric column previously failed ANSI analysis (BIGINT = BOOLEAN) and broke the query; cross-type literals like `x == "5"` on a long column now fall back to interpreted Python, matching Python's cross-type equality instead of Spark's coercion. - transpile.py: a plain function whose first parameter is named `self` is no longer mistaken for a bound __call__; stripping it emitted `_udf_param_-1` and threw an AnalysisException at call construction. - transpile.py: functools.wraps-decorated callables are refused. inspect.getsource follows __wrapped__, so the transpiler compiled the wrapped function's source while the interpreted path ran the wrapper -- a silent wrong result. - transpile.py: a literal None operand in `and`/`or` forces a fallback: Python's `None and x` short-circuits to None while Spark's three-valued `null AND false` is false. - UserDefinedPythonFunction.scala: call-site arity mismatches skip transpilation. Too few args previously raised a misleading "internal error in the Python UDF transpiler" AnalysisException; extra args on a zero-param UDF silently succeeded where Python raises TypeError. Both now surface the standard Python-side error. - ResolveTranspiledPythonUDFOptions.scala: "numeric" no longer matches DecimalType (Python gets decimal.Decimal, which raises TypeError when mixed with float literals) and "string" requires UTF8_BINARY collation (under UTF8_LCASE, `'abc' = 'ABC'` is true while Python's == is False). Both fall back to interpreted Python. - util/package.scala, Expression.scala: auto-generated column names stay `f(a)` when transpilation engages. The TranspiledPythonUDF wrapper previously leaked into schema names as `transpiledpythonudf(f(a), CAST(...))`. - udf.py: the transpile/ANSI conf gates compare case-insensitively; `SET ...=True` stores the literal "True" and silently disabled transpilation (and mis-triggered the ANSI warning). - udf.py: the profiler paths build their UDF without transpiled options. Profiling requires the Python function to execute, and the substituted TranspiledPythonUDF has no resultId for the profiler to key on. Hardening and cleanup: - Optimizer.scala: ConvertToCatalyst traverses subquery plans (the batch runs Once; predicates pulled out of subqueries later could otherwise carry an un-stripped Unevaluable node to execution), and the recursion now tells children when their parent is a scalar Python UDF so UDF-chain pipelining is preserved under a non-transpiled outer UDF. - transpile.py: _get_src_ast_from_func returns cleanly when no source is available instead of surfacing UnboundLocalError as the failure reason; documented the NaN equality divergence. - PythonUDF.scala: drop an unused Logging mixin. - Reverted CoercesExpressionTypes.scala (visibility widening had no remaining caller) and IntegratedUDFTestUtils.scala (no-op change) to master, removing both files from the feature diff. Tests: nine new regression tests in test_udf_transpile_unit.py covering each fixed behavior; the pinned `int == "5"` coercion divergence in test_udf_transpile_known_value_divergences is now Python-faithful and updated accordingly. Verified: unit (51s), parity (77s), gating smoke tests, ConvertToCatalystSuite + ResolveTranspiledPythonUDFOptionsSuite (16/16), CI-equivalent ruff format/check, mypy clean on feature files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GibQDjXJ5jmUhA1yXAxFUr * [SPARK-55206][PYTHON] Classify ternary and boolean operands correctly in _category Codex review on PR #34 found that `_category`'s catch-all labels a nested ternary "numeric", so `("5" if x > 0 else "6") == 5` passed the equality guard as numeric-vs-numeric and Spark's string-number coercion silently returned true where Python's cross-type `==` is False. Reproduced, along with two more instances of the same root cause: a None-branch ternary (`("5" if c else None) == 5`) diverging the same way, and `(x > 0) + 1` (valid Python) failing ANSI analysis because the boolean Compare was labeled numeric and lowered into Add. Fix in `_category`: - ast.IfExp now recurses into its branches: same category -> that category; a None-literal branch adopts the other branch's category (NULL unifies in the lowered CASE WHEN); mismatched or all-None branches raise so the variant is dropped. - Nodes that are statically boolean (comparisons, `not`, boolean ops) are categorized "bool" instead of falling through to "numeric", so they refuse arithmetic/equality lowerings against other categories and fall back to interpreted Python. All four reproduced cases now fall back and match Python exactly. Adds regression tests for the nested-ternary equality (both variants) and boolean arithmetic; unit (50s) and parity (91s) suites pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GibQDjXJ5jmUhA1yXAxFUr --------- Co-authored-by: Claude <noreply@anthropic.com>
97524f0 to
2ba417e
Compare
What changes were proposed in this pull request?
This is the first PR adding initial transpilation from Python to Catalyst and the framework for others to plug in different transpilers which can target other targets. This replaces #53547 and is the first PR as part of the transpilation SPIP https://docs.google.com/document/d/1cHc6tiR4yO3hppTzrK1F1w9RwyEPMvaeEuL2ub2LURg/edit?tab=t.0#heading=h.iz92aps6qbo5
Why are the changes needed?
Python UDF performance has been a perinial challenge in Spark, and while PyArrow makes the data copy slightly less expensive it's still much slower than native code. Additionally, Python UDF usage has increased substantailly with Pandas on Spark and it is especially hard for folks to write catalyst expressions here.
Does this PR introduce any user-facing change?
New configuration flags are user visible (default to off).
How was this patch tested?
New unit tests that trigger always and hypothesis tests that trigger only selectively.
Was this patch authored or co-authored using generative AI tooling?
Hypothesis teests were generated by Claude 4.7, eq semantics null and truthiness w/claude 4.6, GH code review (unknown backing model) on itiial draft). snake camel swap with sonnet.