Repository navigation
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
holdenk
force-pushed
the
SPARK-59090-drop-transpiled-options-that-fail-to-resolve
branch
from
September 2, 2026 21:08
614a814 to
539098f
Compare
A category match is not enough: an unresolved option is a child of TranspiledPythonUDF, so CheckAnalysis INTERNAL_ERRORs the query. Drop it and fall back to interpreted Python, same as a category miss. Co-authored-by: Cursor <cursoragent@cursor.com>
…to end The drop-if-unresolved tests used UnresolvedAttribute, a shape the transpiler cannot emit -- `resolveUDFParams` substitutes every `_udf_param_N` and throws on anything else -- and one that fails CheckAnalysis as UNRESOLVED_COLUMN rather than through the cast path the rule exists for. Use `cast(binary as bigint)`, which is what actually reaches CheckAnalysis unresolved. Options are born unresolved: the transpiler emits concat/coalesce/repeat as UnresolvedFunction and wraps each option in `cast(returnType)`, so `option.resolved` only reports what we want because ResolveFunctions and the coercion rules sit earlier in the Resolution batch. Nothing pinned that ordering. The new full-analyzer tests do, so a future reordering fails here instead of silently falling every transpiled UDF back to interpreted Python. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
holdenk
force-pushed
the
SPARK-59090-drop-transpiled-options-that-fail-to-resolve
branch
from
September 4, 2026 15:16
539098f to
e0596d0
Compare
…ail-to-resolve SPARK-58626 restructured ResolveTranspiledPythonUDFOptions: options now hold untyped `_udf_param_N` references and are unresolved by construction until the rule types them, so checking `option.resolved` where the branch used to would drop every option and turn transpilation off. The feature moves to DropUnresolvedTranspiledPythonUDFOptions, which runs once the Resolution batch has converged and `!resolved` really does mean unresolvable. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
…annot reach For `f(g(x))` with an unresolvable inner option, the outer call's argument IS the inner TranspiledPythonUDF, so while that option lives the outer is unresolved too and ResolveTranspiledPythonUDFOptions -- which needs every argument resolved -- never fires on it. Its categories stayed set and its `_udf_param_N` references stayed untyped, and the drop rule skipped exactly that shape, so CheckAnalysis read `dataType` off an untyped parameter and called it an internal error. Master reported DATATYPE_MISMATCH here, so this was a regression, on the one shape the rule's own comment claimed to handle. The drop rule now does the prune-and-type work for such a node instead of skipping it, via a helper shared with the resolution rule so the two cannot drift. Coercion cannot run again this late, so an option body still needing it stays unresolved and is dropped: the call falls back to Python. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
Two comments stated measured-wrong things. The motivation claimed an unresolved option comes back as an INTERNAL_ERROR, citing `cast(binary as bigint)`. That example actually gives DATATYPE_MISMATCH.CAST_WITHOUT_SUGGESTION; an internal error needs an untyped `_udf_param_N`, which since SPARK-58626 means a nested call. Both sites now say so, and neither oversells the symptom. The documented limitation had it backwards: it was never an internal error for that class, and the fallback makes the message worse rather than less internal. For `SELECT r FROM (SELECT f(b) AS r FROM t)` a DATATYPE_MISMATCH becomes UNRESOLVED_COLUMN offering `r` as the fix for `r`, never mentioning the UDF. Recorded with the shapes it reaches -- SQL and Connect, not classic DataFrame. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
The drop is the fallback path for a condition the rule's own doc says only arises when the transpiler emitted an option it cannot resolve -- so doing it silently erases the evidence of that bug: the query runs interpreted Python and nothing records why. ConvertToCatalyst logs each of its skip paths for the same reason; match it. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
…d option analysis ran out of passes for The WARN claimed every drop meant the transpiler emitted an option it could not resolve. On the nested path that is usually false: the node sits behind a call whose own option failed, so it never got a Resolution-batch pass, and an option needing type coercion or function resolution is dropped though it is fine on a plain column. One transpiler bug produced two WARNs, the second naming a healthy `x / 2`. The comment said both things three lines apart -- "coercion cannot run again from here, so it is dropped" and "this only happens where the transpiler emitted an option it should not have". The message now distinguishes the two causes and only one of them blames the transpiler. The loss is also wider than the comment's "coercion": function resolution goes the same way, since the transpiler emits concat/upper as UnresolvedFunction. And it is not purely all-or-nothing -- dropping the options that needed a further pass moves which one ConvertToCatalyst finds at headOption. Both recorded, with a test for the UnresolvedFunction shape, which is dropped nested and kept flat and which nothing pinned before. Also: assert the node count before the foralls in the nested test, which otherwise pass vacuously on an empty result and fail as a bare MatchError, and identify the two calls by name rather than by traversal order. And redact the logged option like a plan string, since TreeNode.simpleString does not. Co-authored-by: Holden Karau <holden@pigscanfly.ca> Co-authored-by: Holden Karau <holden.karau@snowflake.com>
Owner
Author
|
Folding this into #59 which introduces the issue. |
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?
Drop transpilations which don't resolve with the types.
Why are the changes needed?
If expression's don't resolve query errors could occur.
Does this PR introduce any user-facing change?
Transpilation drops invalid queries.
How was this patch tested?
new test suite
Was this patch authored or co-authored using generative AI tooling?
Claude and GLM