Repository navigation
Conversation
…type promotion Extends the experimental Python UDF transpiler across Python's numeric surface -- `/`, `//`, `**`, `& | ^ ~`, `<< >>`, and the `abs`, `min`, `max` and `round` builtins -- and makes the arithmetic it emits follow Python's overflow behaviour rather than the column type's. The enabling change for the operators is a narrowing of the input-type category vocabulary. "numeric" spans ByteType..DoubleType, which is not enough to know whether a lowering is exact: `x ** 3` as `x*x*x` is exact on a bigint but rounds twice on a double, and `//` needs `div`, which rejects fractional input. So `ResolveTranspiledPythonUDFOptions` gains "integral", "integral32" and "fractional", which narrow "numeric" rather than replacing it. Bodies whose exactness depends on the kind of number emit one option per kind; everything else produces exactly the options, and the plan size, it did before. The enabling change for overflow is `PythonNumericPromotion` plus four new Catalyst expressions. A Python `int` has no width -- it is a digit array that grows per value -- so `x * 100` over a tinyint column answers 12700, and the interpreted UDF agrees because it converts each row's result to the declared return type individually. Catalyst picks one type per column before seeing any data and `Multiply` keeps its operands' type, so the transpiled form raised ARITHMETIC_OVERFLOW. Note `a + b` on two int columns already *reported* bigint, since the transpiler casts to the declared return type at the end -- the addition still happened in IntegerType and overflowed long before that cast. It is the operands that need widening. Reproducing "per value" with a static type is impossible, and unnecessary: Python needs unbounded integers because a Python int is unbounded, while a Spark column is 8, 16, 32 or 64 bits, and that bound propagates. The worst case over every row a column could hold is therefore computable from the input types with no data at all. This has to live in Catalyst, because the transpiler only sees a category and concrete widths are known only once the arguments resolve. Two correctness fixes fall out of testing the bounds: - FloatType columns were lowered and had no faithful lowering. Python has no single-precision float, so a value arrives widened to a double and every step runs in double, while an expression that stays in FloatType rounds to 24 bits per step -- `(x + y) * y` disagreed on 398 of 398 sampled random pairs, and float32 saturates to infinity where Python has 250 orders of magnitude spare. FloatType now falls back, next to DecimalType, and the type matrix is expressed as allow-lists so a numeric type added later fails closed. - NaN comparison and NULL string concatenation are now guarded (adopted from apache#58185, adapted for the refined categories -- a double column reports "fractional", so an equality test against "numeric" would silently stop firing exactly where NaN lives). The transpiler must never return a different answer than the UDF it replaces. Two of the shapes above did: a FloatType column silently, and any column whose arithmetic overflowed an intermediate loudly, failing a whole query over one large row while the interpreted path computed every row happily. Yes, when the unreleased experimental transpiler is explicitly enabled. More numeric bodies lower; FloatType columns and the shapes with no exact lowering stay interpreted; and arithmetic that used to raise on an intermediate now computes. A result too wide for the *declared return type* still raises, deliberately. Interpreted Python does not -- `makeFromJava` applies `.toByte` / `.toShort` / `.toInt`, so a UDF declared `-> ByteType` turns 500 into -12 without a word, and LongType nulls the row. Matching that would mean reproducing silent data corruption. Promotion is about not failing on an intermediate Python would have carried, not licence to invent a wrong answer. Promotion stops at LongType, so `x * y` on two bigints still raises as before. DecimalType(38, 0) would reach further, but `Multiply` on two decimals wants a result precision of 76 and the DecimalPrecision *coercion rule* caps that; a RuntimeReplaceable's replacement never goes through coercion, so a decimal replacement does not resolve. Reaching decimal means doing the widening in an analyzer rule that runs before coercion; `narrowestFor` documents this. - `build/sbt -Phive package` - `PythonNumericPromotionSuite` (new) and `ResolveTranspiledPythonUDFOptionsSuite` -- 30 tests. The promotion widths are asserted as derived arithmetic, so the numbers stop lining up if the rule drifts. - `pyspark.sql.tests.test_udf_transpile_unit` -- 69 tests. - `pyspark.sql.tests.test_udf_transpile_parity` -- 112 tests, re-running the existing UDF suites with transpilation on. - `pyspark.sql.tests.test_udf_transpile_hypothesis` -- 74 tests, with `RUN_HYPOTHESIS=1`. These test the *bounds* rather than the interior: every integral width (not just LongType, so the widening casts are actually executed), the whole double space including NaN and both infinities, the shapes that must fall back with CPython as the oracle, mixed-magnitude rows within one column, and the divergences kept on purpose pinned as divergences. Generated-by: Claude Opus 5 (1M context) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
Operand promotion broke the argument that let `_int32_exact` accept `*`. The reasoning was that ANSI keeps every intermediate in IntegerType, so a product of int32 operands either stays under 2**31 or raises -- true when it was written, and false the moment promotion started evaluating that product in bigint, where it reaches 2**62. Past 2**53 the cast to double rounds before dividing, so `(a * b) / c` over three int columns returned 422461242.77102745 where CPython gives 422461242.7710275. That is the exact class of silent divergence the transpiler exists to avoid, and it was introduced by the change meant to remove such divergences, which is worth saying plainly. `+`, `-` and `%` stay: their worst case is 2**32 even after promotion widens them, still exact as a double. Found by review, not by the suites -- the only compound `/` test used `(x + 1) / (y - 1)`, Add and Sub only, so nothing exercised a promoted product feeding a division. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
Four findings, three of which were code I wrote to solve problems that did not exist. `flatten` did nothing. It existed because `x ** 3` once produced an INTERNAL_ERROR whose message mentioned a nested marker, and I concluded a replacement may not contain another RuntimeReplaceable. It may: `ReplaceExpressions.replace` recurses unconditionally, and the nested form resolves and flattens on its own. The actual fault in that error was an *unaligned* `Add(bigint, int)`, which `plain` already fixes. So the helper, its three call sites and fifteen lines defending a misdiagnosis are gone, and `plain`'s doc now states the real constraint: operands must be type-aligned because a replacement never goes through type coercion. `magnitude`'s DecimalType arm was worse than refusing. Ranking a decimal by precision looked more honest than declining, but only scale-0 precision <= 18 survived, and it promoted to *bigint* -- silently discarding the decimal type. Everything else fell through unaligned to INTERNAL_ERROR. Returning None for every decimal sends them to `plain`, which keeps the type. The four expressions are now registered with `registerInternalExpression`, which is what they should have been from the start. Registering them publicly meant `SELECT python_promoting_add(1, 1.5)` surfaced as INTERNAL_ERROR (SQLSTATE XX000) rather than DATATYPE_MISMATCH, because RuntimeReplaceable does not forward `checkInputDataTypes`; it also exposed a `foldable`-then-`eval(EmptyRow)` path in PivotTransformer that every other registered RuntimeReplaceable escapes by extending UnaryLike. And it obliged them to carry @ExpressionDescription and a row in sql-expression-schema.md, which they did not, failing ExpressionInfoSuite and ExpressionsSchemaSuite -- two suites this branch had never run. Internal registration removes all four problems at once, and PySpark reaches them through InternalFunction, as pandas_product and friends do. Finally, `narrowestFor`'s explanation of the LongType ceiling was false and is corrected in place rather than quietly dropped. It claimed a decimal replacement could not resolve because `DecimalPrecision` coercion is what caps a product's precision at 38. `Multiply` caps it itself in `resultDecimalType`, and `Multiply(cast(a, decimal(38,0)), cast(b, decimal(38,0)))` resolves standalone. What failed was `Add(decimal, int_literal)` meeting a `widestOf` that does not rank DecimalType -- a gap in this file, not in Catalyst. The ceiling is self-inflicted, and the note now says where it goes away: widen in an analyzer rule, which `ResolveTranspiledPythonUDFOptions` is already positioned for, since it runs in the Resolution batch alongside typeCoercionRules and would be coerced on the next fixed-point iteration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
`forNegation`'s scaladoc claimed to cover "abs(a) and unary minus", and the module docstring listed `-` among the operators that no longer overflow an intermediate, but nothing ever called it for negation. So `-x` on an int column holding Integer.MinValue raised ARITHMETIC_OVERFLOW while `abs(x)` on the same column answered 2147483648 -- the identical two's-complement gap, fixed in one place and not the other. PythonPromotingNegate closes it; verified at the byte, short and int minima. PythonPromotingAbs was reaching the promotion rule through the *binary* `widened` with its child in both slots. That evaluated the child twice, and because `replacement` is a `def` rather than a cached value, a nested `abs` cost 2^(depth-1) leaf type reads; it also built a `Cast` for the second slot that the operator then discarded. It now applies `forNegation` directly. `round(x, negative_scale)` has the same defect and is NOT fixed here, only recorded. `bround` keeps its child's type and a negative scale multiplies the magnitude by 10**|scale|, so `round(x, -1)` on a tinyint holding 127 raises where Python answers 130, and the declared LongType return would hold it fine. The worst case is computable the same way, but the widening cast has to be sized on the JVM -- only the analyzer knows the column width -- so it wants a promoting expression of its own rather than something improvised in transpile.py. A first attempt at the Python-side shortcut multiplied and re-divided by the scale factor, which would have quietly turned the result into a double; better a documented gap than that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
Two gaps where the behaviour was already right but nothing held it there. `"ab" * 2.5` is a TypeError in Python, and a lowering that cast the count down would return "abab" -- a plausible-looking wrong answer, the worst kind. The existing coverage asserted only that the body stays interpreted, which is the cheaper half of the contract: it would still pass if the interpreted path started returning "abab" too. Now the raise itself is pinned, via the CPython oracle, and so is the integral count still lowering, so it reads as a guard rather than a blanket refusal of `*` on strings. Unary minus at each width minimum had no test at all -- `test_abs_at_the_width_ minimum_matches_python` covered only abs, and the one existing negation body was exercised at 5 and -3. That is how `-x` came to raise on an int column holding Integer.MinValue while `abs(x)` on the same column answered, even though `forNegation`'s scaladoc claimed to cover both. bigint is deliberately absent for the same reason it is absent from the abs pin: -(-2**63) is 2**63, which no LongType return can hold however wide the arithmetic gets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
Review found the comments in this area had drifted from the code, in one case
badly enough to teach the next reader something false.
`unpromoted`'s doc had its case backwards. It read as though it existed for a
child that is unresolved in the `UnresolvedAttribute` sense, but `plain` asks its
operands for `dataType`, so such a child throws `UnresolvedException` rather than
being reported around -- the doc described the one thing the code cannot do. What
it actually serves is a child unresolved because its *own* type check failed
while still reporting a dataType, `ShiftLeft(tinyint, tinyint)` for instance.
Nothing reaches the throwing path through the SQL parser today, since
`ResolveFunctions` waits for `childrenResolved`, but it is a trap for a
single-pass resolver or a hand-built plan and the comment now says so.
Also: a stranded scaladoc sat above `forNegation` describing `widened`, which had
none, and is moved; the trait doc said the replacement "is lazy" eleven lines
above the note explaining it is deliberately a `def`; `widestOf` cited
`_promoting_if_integral`, renamed to `_promoting_if_numeric` when the gate was
widened, and claimed only integral operands reach it, which the widening is
precisely what stopped being true; the suite still described `decimal(38,0)`
"landing exactly on DecimalType.MAX_PRECISION" directly above an `isEmpty`
assertion; and `nodePatterns` duplicated RuntimeReplaceable's own declaration.
The three `.cast("long")` justifications in the bitwise and invert arms are
re-derived rather than reworded, because they were load-bearing comments: each
said a promoted operand could arrive as a decimal, which the LongType ceiling now
makes impossible. So the casts are defensive, not required -- a promoted operand
is at worst a bigint, which `bitwiseAND` coerces against an int happily. They are
kept, since Python's bitwise operators are int-only and normalising is honest
about that, and the comments now say they could go with a test run rather than
implying they must stay. Rewording without re-deriving would have preserved the
original mistake in nicer prose.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Holden Karau <holden@pigscanfly.ca>
Co-authored-by: Holden Karau <holden.karau@snowflake.com>
Asked whether a custom Catalyst expression for Python-semantics equality was
worth adding. It is not -- but the reason it seemed attractive was that
`_lower_eq` hand-rolls a four-branch CASE WHEN, and it turns out Spark already
ships the operator it was reimplementing.
`EqualNullSafe` (`<=>`) *is* Python's None-equality: both-null true, one-null
false, values otherwise. Measured against CPython over the full grid of
{null, 0.0, -0.0, 1.0, NaN} on double columns and {null, 0, 1, -1} on bigints,
both operand orders, and against an int literal on a bigint column with coercion
intact -- identical on every row, signed zero included.
Applied only where `_nan_guard` declines, which is where NaN cannot occur:
strings, provably-integral numerics, a None literal. `nan <=> nan` is true and
Python's is not, so the guard is still required elsewhere.
And the guard deliberately stays *after* the null branches rather than being
hoisted above an `eqNullSafe`, which would let the whole chain collapse in every
case. `isnan(l) | isnan(r)` short-circuits: with a NaN on the left the right
operand is never evaluated, so `x == 1.0 / y` would answer False where Python
raises ZeroDivisionError. The comment says so, because it is the obvious next
simplification and it is wrong.
A custom expression was rejected on cost and on risk. Roughly 150 lines across
six files, two of them shared optimizer files, plus a StackOverflow guard in
UnwrapCastInBinaryComparison. Worse, `case _: BinaryComparison` is matched at
around fourteen generic sites with no subclass filter: `symbol = "="` gets pushed
to DSv2/JDBC as three-valued SQL and silently drops the both-null rows Python
calls true, while a custom symbol recurses infinitely in
V2ExpressionSQLBuilder into a StackOverflowError that NonFatal does not catch.
Silent wrong answers and hard crashes in place of the loud INTERNAL_ERRORs the
RuntimeReplaceable route gave -- a bad trade for a plan-size win.
Not taken, and noted in NUMERICS.md: tightening `_nan_guard` to require *both*
operands fractional for `==`. The argument is that `isnan(cast(bigint as
double))` is provably dead, but the probe I ran to check it read a NULL row and
measured nothing, so there is no evidence yet. It must not be applied to
`_lower_value_compare` in any case -- `bigint < NaN` is true in Spark and false
in Python, so the both-integral bail has to stay there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Holden Karau <holden@pigscanfly.ca>
Co-authored-by: Holden Karau <holden.karau@snowflake.com>
Two comments pointed at notes that only exist on my box -- one at a scratch JIRA draft, one at `NUMERICS.md`. Neither ships with the patch, so a reader hits a dangling pointer. The `round()` TODO also named SPARK-55210, which this change resolves, so it would have pointed at a closed ticket the moment it landed; it is a plain TODO now and the follow-up gets its own number. Comments only -- no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
Seven findings from `/code-review max`, two of which mattered. **Analysis went exponential.** `replacement` was a `def`, and `RuntimeReplaceable` derives `dataType` and `nullable` from it with no memoization, so one type read on a nested node rebuilt its whole subtree and each level multiplied the level below. Measured before: 0.10s at 9 nested nodes, 3.5s at 13, and 19 nodes never finished in nine minutes -- and `x ** 8` lowers to seven nested multiplies on its own. It cannot be a plain `lazy val` either; that pins the answer computed while the children were unresolved, which is the bug that made every compound body fall back. Gating the cache on `childrenResolved` gets both, since children never return to being unresolved. A 24-node chain now types in 5ms. **An integral return type truncated a double input.** Nothing in `x * 2` proves a float -- that depends on the bound column -- so `udf(lambda x: x * 2, LongType())` on a double column holding 3.7 lowered to `cast((x * 2.0) as bigint)` and answered 7, where the interpreted path yields NULL because `makeFromJava` takes only Byte/Short/Int/Long. A silent wrong answer is the one thing transpilation must never produce. The option now narrows to integral, so a double column falls back, and no extra plan variants are emitted. That fix needed the `_narrow` fix first: it walked the whole subtree with `ast.walk` and narrowed parameters appearing only in a *condition*, so `s * (2 if n > 1.0 else 3)` refused a double `n` for no reason. It now walks value positions only, the line `_int32_exact` already drew. Also: dropped the dead `.filter` in `promote` (the chosen type is always strictly wider than every input, now pinned by a test over the whole cross product); hoisted `widestOf`'s rank table out of the call; read each operand's type once; derived `_NARROWABLE_CATEGORIES` from `_NARROWING_ORDER` rather than restating it; and dropped two `val (l, r) = (left, right)` self-rebindings. `ast.Call` was matching every call and routing it to the builtin lowering, so `(lambda y: y + 1)(x)` reported "only positional calls to a small set of builtins" -- advice for a body that was not calling a builtin. Narrowed to `Call(func=Name())` so everything else keeps the generic message that names the node. Seven new Scala tests cover the five expressions, which had none: the resolution-gated cache by object identity, `plain`'s operand alignment, operand order through `withNewChildren`, and a 24-deep chain as the regression guard for the blowup above. Verified: catalyst 36, transpile unit 79, udf parity 93, and the hypothesis suite 76 with zero skips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
`NumericEvalContext` states the rule in its own scaladoc: a context "must be stored as part of the expression's state to ensure deterministic evaluation", because otherwise copying the expression or evaluating it elsewhere -- inside a view, say -- can read different conf. These five were a sharp case of breaking it. Their replacement is built lazily, so the operator's eval mode was decided by *when* something first asked for `dataType` rather than by the query: two otherwise identical nodes canonicalized differently and stopped sharing, and a plan analyzed under ANSI could be optimized into a non-ANSI `Add`. All five now take an `evalContext` defaulted at construction and thread it into three places: - the `Add`/`Subtract`/`Multiply` they build; - `Abs`/`UnaryMinus`'s `failOnError`, since those predate the context and take a Boolean -- ANSI and TRY both map to true, the same reading `BinaryArithmetic` uses, TRY evaluating as though it would fail and capturing the error after; - the `evalMode` of every widening `Cast`. That one is canonical form only, not behaviour: these casts always widen, so there is no value to overflow and no mode that changes the answer. A differing evalMode splits the canonical form just as effectively as a differing `Add`. Adding a defaulted field changes the primary constructor's arity, and `FunctionRegistryBase.build` looks for a constructor whose parameters are all `Expression`, so each expression carries a children-only secondary constructor beside it exactly as `Add` does. That failure mode strikes at the call site rather than at startup, so a test resolves all five through `FunctionRegistry.internal` -- without it the promotion suite stays green while every transpiled UDF breaks. `allowDecimalPrecisionLoss` is stored but not consulted: the LongType ceiling means we never produce a decimal. It becomes load bearing if that ceiling is ever lifted. Five new tests: the stored context reaching the replacement and its casts, canonical form no longer depending on force ordering, survival through `withNewChildren`, the unary failOnError mapping over all three eval modes, and registry construction. Verified: catalyst 41, transpile unit 79, udf parity 93, hypothesis 76. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
`dev/check_pyspark_custom_errors.py` scans for lines whose `strip()` starts with "raise" and whose next token is capitalised, then insists the error be one of PySpark's own. A docstring that happened to wrap onto "raises TypeError on ``None > 0``" matched all of that -- `startswith` does not stop at a word boundary -- so the check failed on prose describing what CPython does, and took the whole lint run with it before black, ruff and mypy got to run. Reworded to "throws". Swept the other changed Python files for lines that would trip the same rule; there are none. `dev/lint-python` and `dev/scalastyle` both clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
SPARK-58626's test read zero draws as proof that both sides of `x == x` became one column: `col = col` folded to true and pruning dropped the draw. On this branch a double `==` lowers to `NOT isnan(col)`, since Python's `x == x` is False for NaN, and that still depends on the column -- so the one shared draw stays in the plan. Assert the sharing directly instead: one draw and the shared alias, where two draws would be the bug. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
Merge interaction, not a defect in either side. Master's bare `if x`
(SPARK-56925) keys `_truthiness_col` off an exact `cat == "numeric"`,
while this branch refines a numeric parameter to "integral" /
"integral32" / "fractional" as soon as some other operator in the body
needs the narrower kind. So a parameter read by BOTH a bare `if x` and
one of those operators matched nothing and the whole option was refused:
`if x: 1 else 0` lowered, `x // 2` lowered, `if x: x // 2 else 0` lowered
neither. Falling back is safe, so this cost throughput rather than
correctness, and no test on either side covered the combination.
`_truthiness_col` now coarsens first, which is how the comparison
lowerings already gate -- see `_coarse_category`, whose docstring
describes this same class of bug ("would refuse mixed int/float
comparisons that used to transpile"). Which kind of number it is cannot
change whether the number is zero, so the refinement was never
information this lowering wanted. Non-numeric categories are untouched:
"binary" still returns None and still falls back.
The test pins the combined shape on both narrowings, `//` for integral
and `/` for fractional. Verified it fails without the one-line change,
on the "category is unknown or unsupported" path.
Co-authored-by: Holden Karau <holden@pigscanfly.ca>
Co-authored-by: Holden Karau <holden.karau@snowflake.com>
The branch's two merges from master had kept master's caller-facing `_param_index` (the receiver is stripped by the caller, so the lookup is plain) and the two extra `AbstractTranspiler.transpile` docstring paragraphs. Replaying the branch's commits without the merges resurrected the older text; put it back. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com>
The CatalystTranspiler's per-variant body-category check still refuses every array-typed body, so an atomic body declared as returning an array falls back the same way; only a bare-None body lowers (NULL casts to any array type cleanly, and the interpreted path returns None too). Other transpiler varieties can produce full array-typed expressions whose cast to the declared array type resolves, so the outer gate no longer rejects the array-of-atomic shape outright. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com>
The rebase auto-merge appended master's SPARK-55214 `len` arm after the branch's generic `ast.Call(func=ast.Name())` arm in `_convert_chunk`, so every `len(s)` call routed into `_lower_builtin_call` and was refused (`len` is not one of the rebinding-gated builtins). Match arms are ordered; the specific arm has to come first. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com>
… SPARK-55210 SPARK-55210 loosens the transpiler's outer return-type gate to let array-of-atomic returns through, which is only safe-by-net for transpiler varieties beyond the built-in one. Folding SPARK-59090's DropUnresolvedTranspiledPythonUDFOptions rule in here means the loosening can't bite a future variety (e.g. the SPARK-55209 Java target) even before SPARK-59090 merges to master: an option that matches its category but never resolves is dropped and the call falls back to interpreted Python, instead of failing the query at CheckAnalysis. Pinned by a new full-analysis test that throws DATATYPE_MISMATCH on #59 alone and passes once the drop rule is in. All 24 of #59's existing category tests stay green alongside it. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…ds, not the remainder The floor-division lowering corrects Spark's div (truncates toward zero) toward Python's // (floors toward -inf) by subtracting one when the division is not exact and the operands' signs differ. The condition read the signs off the remainder (%, Spark's Remainder, which takes the dividend's sign) rather than off the operands directly. That happens to match -- Spark's Remainder carries the dividend's sign -- but it is an invariant the lowering does not need to lean on, and spelling the condition in terms of the operands keeps the lowering correct if that invariant ever moves. Add a unit test pinning the four sign combinations, the near-edge LongType widening values, and a NULL operand. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…s over multi-row DataFrames The SPARK-55210 suite grew to 63 @_hyp_settings tests, and at CI's RUN_HYPOTHESIS_MAX_EXAMPLES=50 the single module ran ~700s -- well past PYSPARK_TEST_TIMEOUT (450s), so CI killed it every run (only ~24 of 63 tests ever completed on the PR). Two changes get it back under budget: 1. Lower the CI cap 50 -> 10. The explicit @example edge seeds always run on top of the generated examples, so the boundary cases that actually matter stay covered; only the random fuzz volume drops. 2. Pack the non-raising single-argument differentials over multi-row DataFrames. Each @given example now draws a list of _BATCH (10) values, builds one multi-row DataFrame, and runs the UDF once per side (two Spark jobs total) instead of one job per value -- so the same number of jobs fuzzes _BATCH times as many inputs. A UDF that raises on any row aborts the whole batch comparison, so the raising UDFs stay on the one-row-per-example path. Locally the whole module now runs 76 tests in ~390s at cap=10, comfortably under the 450s CI budget (it was ~650s at cap=25 and ~700s at cap=50). Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…and multi-row width tests The test_udf_transpile_hypothesis file (76 methods) blows past the 450s PYSPARK_TEST_TIMEOUT the fast pyspark-sql group shares on CI runners, so move it into its own pyspark-sql-slow module with a 3600s per-test timeout and 20 hypothesis examples instead of cranking the fast group's timeout. Pack the one-row-per-example width tests (square, pow_one, pow_at_cap, abs, round, bit_invert, plus_four) into multi-row batches via a new _integral_column_batch strategy, so each @given example covers _BATCH values at the same two-Spark-job cost -- the same trick the LongType tests already use. This does not cut wall-clock but fuzzes _BATCH times as many values per example within the new budget. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…for pyspark-sql-slow Adding the pyspark-sql-slow module (which depends on hive and so transitively on sql) puts it in the output of determine_modules_to_test([modules.sql]) and the sql+core / sparkr+sql variants. Those functions have doctests with hardcoded expected module lists, so every "Build modules: X" CI job failed on the doctest. Add pyspark-sql-slow to the three expected lists in sorted position. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…ithmetic Transpiled numeric arithmetic propagated NULL where CPython raises TypeError: `x + 1` on a NULL row came back NULL, silently dropping a row that should have failed. Wrap every numeric lowering in a shared `_raise_on_null_operand` guard -- the same pattern the comparison, min/max, and concat lowerings already used -- so a NULL operand raises via raise_error instead of propagating. This closes the tier-2 "NULL against TypeError" divergence: `x ** 0` on NULL now raises too, and the `>>` lowering's explicit NULL branch folds into the shared guard. The pinned divergence tests become regression guards, `_raises` takes the UDF's return type so `/` and string repeat can be pinned, and two pre-existing plan assertions switch from the "UDF" substring (which USER_RAISED_EXCEPTION now trips) to counting EvalPython nodes. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…, drop-batch parent resolution Fixes from bot review on PR #59: - len() category: return 'integral' not 'numeric'. len(s) is always a Python int; the integral-only lowerings (//, &, shifts, round) were missing len(s) // 2 etc. and falling back unnecessarily. - repeat count: narrow to 'integral32' not 'integral'. repeat casts the count to int, which raises under ANSI for a bigint above Int.MaxValue where Python returns '' (negative) or a huge string. A bigint column now drops the option and falls back instead of risking overflow. - Drop-batch parent resolution: change the 'Drop Unresolved Transpiled UDF Options' batch from Once to fixedPoint and add ResolveAliases + ResolveReferences. After the drop rule resolves the UDF, an enclosing UnresolvedAlias or parent UnresolvedAttribute was left untouched (the Resolution batch already converged), causing CheckAnalysis to fail instead of falling back. - Warning wording: only claim 'falls back to interpreted Python' when every option was dropped; with survivors, say how many remain. - func= keyword compat: feature-detect whether a custom transpiler's _transpile_from_ast accepts the func keyword via inspect.signature. Without this, old-signature transpilers silently dropped every option via the broad except. - Callable __call__ shadow check: use _call_impl(_call_dunder(func)) for non-function callables, matching _transpile_func. The old getattr(func, '__call__') found an instance attribute Python ignores, missing a shadowed free variable and lowering a builtin as the builtin. - Test fixes: array<int> -> array<bigint> assertion, repeat count column type long -> int, sign-preserving scaling in hypothesis test, updated outdated overflow comments, added two Scala tests for the drop-batch parent resolution fix. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…t changed The previous commit re-ran ResolveAliases and ResolveReferences in a fixedPoint batch after RemoveTempResolvedColumn, which broke ordinary queries: a second ResolveReferences pass that late re-resolves an invalid ORDER BY reference over an Aggregate into a TempResolvedColumn that nothing strips, so queries that must fail with UNRESOLVED_COLUMN instead analyzed. AnalysisErrorSuite, AnalysisSuite, LateralColumnAliasSuite and the group-by/order-by golden files in SQLQueryTestSuite all caught it. The parent-resolution need is real -- dropping an option resolves the UDF after the Resolution batch converged, leaving the enclosing UnresolvedAlias of SELECT udf(a) and by-name parent references behind, and CheckAnalysis then reports the fall-back itself as an error. So the drop rule now runs the resolution rules itself, only on plans it actually changed: ResolveReferences, ResolveAliases, and ResolveAggregateFunctions, one pass each in Resolution-batch order. ResolveAggregateFunctions is load-bearing: it marks the TempResolvedColumn that ResolveReferences re-creates in ORDER BY/HAVING as hasTried, so the RemoveTempResolvedColumn batch can turn invalid references back into UNRESOLVED_COLUMN errors. The batch therefore also moves before RemoveTempResolvedColumn. Pins the corner with a new test: an invalid ORDER BY in the same query as a dropped option must still fail with UNRESOLVED_COLUMN, not MISSING_ATTRIBUTES from a bare unwrapped attribute. All four previously-failing suites re-run locally: AnalysisErrorSuite, AnalysisSuite, LateralColumnAliasSuite and SQLQueryTestSuite (847 golden files) are green, plus the 27-test ResolveTranspiledPythonUDFOptionsSuite. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Two related changes to the experimental Python UDF transpiler's option resolution, the second folded in from SPARK-59090:
SPARK-55210 -- numeric type promotion. Match Python's type promotion of transpiled operations. The transpiler tags options with narrower numeric categories (
integral,integral32,fractional) where an exact lowering depends on the kind of number, andResolveTranspiledPythonUDFOptionsmatches them against an allow-list of column types with a faithful Python counterpart (Byte/Short/Integer/Long/Double fornumeric; the narrow ones as documented in the rule). Decimal and Float are deliberately excluded -- Python'sdecimal.Decimaland the absence of single-precision float make their Catalyst arithmetic diverge. Also lets array-of-atomic return types through the transpiler's outer gate (the per-variant body-category check still refuses every array-typed body, so only a NULL lowering survives, which casts cleanly to any array type).PythonNumericPromotioncomputes the worst-case magnitude of an operation from the input types alone and widens the operands to a type that holds it (tinyint * 100-> short arithmetic,int + int-> long arithmetic), through internalpython_promoting_add/subtract/multiply/abs/negateexpressions; bigint-range worst cases stay put and raise exactly as before.SPARK-59090 -- drop transpiled options that fail to resolve (folded in). A category match does not make an option resolvable --
cast(binary as bigint)matches "binary" and never resolves -- and an option is a child ofTranspiledPythonUDF, so one left unresolved when analysis finishes reachedCheckAnalysisand failed the whole query with a type mismatch naming a cast the transpiler invented. AddsDropUnresolvedTranspiledPythonUDFOptions, aOncebatch after the Resolution batch converges, which drops any still-unresolved option so the call falls back to interpreted Python instead of failing. The pruning body is shared between the two rules viapruneByCategoryAndTypeParametersso they cannot drift. Because the drop resolves the UDF after the Resolution batch converged, whatever was waiting on it -- the enclosingUnresolvedAliasofSELECT udf(a), or a by-name parent reference like therofSELECT r + 1 FROM (SELECT udf(a) AS r ...)-- is left behind andCheckAnalysisreports the fall-back itself as an error. The drop rule therefore also re-runsResolveReferences,ResolveAliasesandResolveAggregateFunctionson exactly the plans it changed, in Resolution-batch order, with the batch placed before "Remove TempResolvedColumn" so theTempResolvedColumnmachinery that re-run may create is still cleaned up by that batch.Why are the changes needed?
ARITHMETIC_OVERFLOWon intermediates the interpreted UDF carries without noticing (udf(lambda x: x * 100)on a tinyint column,absof a width's minimum), or silently diverge from the interpreted UDF (e.g.x ** 3rounds twice on a double;a // bneeds integral input). The narrower categories pick the exact lowering per operand kind, and the promotion expressions make the result match Python where the lowering already exists.CheckAnalysisinstead of falling back to Python -- for the most common shape,df.select(udf(col)), as anINTERNAL_ERRORon the unresolved alias once the option was gone. The failure mode the SPARK-59090 half removes, including for the enclosing aliases and parent references above.Does this PR introduce any user-facing change?
Transpilation (experimental, off by default) is closer to Python's semantics, and an unresolvable transpiled option now falls back to interpreted Python instead of failing the query. No change when transpilation is disabled.
How was this patch tested?
New hypothesis and unit tests for the numeric promotion (
PythonNumericPromotionSuite, extendedResolveTranspiledPythonUDFOptionsSuite), a full-analysis test pinning the SPARK-59090 drop behavior, and three more for the parent resolution: the unaliased call, the by-name parent reference, and a corner case pinning that an invalidORDER BYin the same query as a dropped option still fails withUNRESOLVED_COLUMNrather than being resolved through the re-run. Because the drop rule shares batches with the aggregate machinery, the analysis suites that pin that machinery were re-run locally (JDK 21):ResolveTranspiledPythonUDFOptionsSuiteAnalysisErrorSuite+AnalysisSuiteLateralColumnAliasSuite,AnalyzerExtensionPropagationSuiteSQLQueryTestSuite(golden files incl. group-by / order-by / having / postgreSQL)PythonNumericPromotionSuitetest_udf_transpile_unit/_parity/_hypothesis(RUN_HYPOTHESIS=1, 200 examples)Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude 5
Generated-by: Kimi (kimi-k3, via Cursor) -- 2026-09-29 rebase onto upstream/master, conflict resolution, array-of-atomic return-type gate
Generated-by: GLM 5.2 (Cursor) -- folding SPARK-59090 (drop-unresolved-options) into this PR
Generated-by: GLM 5.2 (Cursor) -- 2026-10-04 rebase onto upstream/master, preflight, finishing for review
Generated-by: GLM 5.3 (Cursor) -- 2026-10-22 review fixes: scoped post-drop parent resolution, repeat narrowing, custom-transpiler compat