feat(trino): parse WITH FUNCTION ... RETURNS ... RETURN inline UDFs [CLAUDE]#7934
Open
rusackas wants to merge 3 commits into
Open
feat(trino): parse WITH FUNCTION ... RETURNS ... RETURN inline UDFs [CLAUDE]#7934rusackas wants to merge 3 commits into
rusackas wants to merge 3 commits into
Conversation
Trino supports declaring SQL UDFs inline in a WITH clause preceding the query (https://trino.io/docs/current/udf/sql.html). These currently fail to parse ("Expecting (") because a WITH-clause entry starting with FUNCTION isn't recognized. Closes tobymao#5178. This is the minimal slice: TrinoParser._parse_cte recognizes a `FUNCTION <name>(...)` entry (as opposed to a CTE literally named "function") and parses it into a new exp.FunctionSpecification node, with a mandatory RETURNS clause and a RETURN <expr> body. Inline functions live in their own WITH clause before the query's own WITH clause per the grammar, so TrinoGenerator.with_sql emits `WITH <functions> WITH <ctes>` when both are present. walk_in_scope treats FunctionSpecification as opaque so qualify does not try to resolve UDF parameters as outer-scope columns. Not covered here, tracked as follow-ups: routine characteristics (LANGUAGE, DETERMINISTIC, SECURITY, ...), BEGIN...END block bodies with DECLARE/SET, and control statements (IF, CASE, LOOP/WHILE/REPEAT). This supersedes tobymao#7926, which bundled all of the above and was closed as too large to review in one pass. Disclosure: implemented with Claude Code, reviewed, tested (make unit, make style) and understood by me. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author
|
As mentioned in the PR description, I'm happy to see if we can get through this first, then tackle next steps, or "stack" some PRs (in draft) to show how this will all come together. |
geooo109
reviewed
Jul 24, 2026
Collaborator
|
One last comment, we also need to cover this: |
Author
|
Thanks for the feedback... digging in! |
- Parse routine characteristics via the existing _parse_properties() instead of a bespoke _parse_routine_characteristics wrapper. - Simplify TrinoGenerator.with_sql to delegate to the base implementation for the CTE portion instead of reimplementing it. - Use properties(..., prefix=" ") instead of a separate blank-check line. - Revert the walk_in_scope/FunctionSpecification opacity change; that's deferred to a follow-up once merge_subqueries and friends are audited. - Trim test_inline_udf: drop explanatory comments, the find_all-based scope-traversal assertions, and the invalid-input cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…TION WITH FUNCTION f(...) ... WITH RECURSIVE t AS (...) SELECT ... failed to parse: the base _parse_with() only checks for RECURSIVE once, right after the very first WITH token, so it choked trying to parse a RECURSIVE that shows up on the query's own (second) WITH clause. TrinoParser._parse_with now also checks for RECURSIVE after a later bare WITH. Also guards add_recursive_cte_column_names (inherited from Presto) against non-CTE entries in a recursive With's expressions -- it assumed every entry had an "alias" arg, which a FunctionSpecification doesn't, and crashed with a KeyError building this AST directly rather than through the parser. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author
|
Indeed, found and fixed a gap while poking at |
Author
|
I think/hope we're cleaned up now, but you're a fantastic reviewer, so I'll just cross my fingers for now. |
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.
This re-splits #7926 (closed as too large to review in one pass) into small, incremental PRs per @geooo109's request. This is PR 1 of ~7. I know it's just as many files touched, but it's
+154 -15rather than+493 -16at least ;)Trino supports declaring SQL UDFs inline in a
WITHclause preceding the query (https://trino.io/docs/current/udf/sql.html):These currently fail to parse (
Expecting (). Happens to resolve #5178 (closed), it seems.This PR is the minimal slice:
TrinoParser._parse_cterecognizes aFUNCTION <name>(...)entry in aWITHclause (as opposed to a CTE literally namedfunction, e.g.WITH function AS (SELECT 1)) and parses it into a newexp.FunctionSpecificationnode, with a mandatoryRETURNSclause and aRETURN <expr>body — enough to round-trip the literal example from #5178. Inline functions live in their ownWITHclause before the query's ownWITHclause per the grammar, soTrinoGenerator.with_sqlemitsWITH <functions> WITH <ctes>when both are present.walk_in_scopetreatsFunctionSpecificationas opaque soqualifydoesn't try to resolve UDF parameters as outer-scope columns.Next steps (planned follow-up PRs, each gated on the previous)
WITH FUNCTION ... RETURNS ... RETURN ...LANGUAGE,DETERMINISTIC/NOT DETERMINISTIC,CALLED/RETURNS NULL ON NULL INPUT,SECURITY,COMMENT)BEGIN...ENDblock bodies +DECLARE/SET, including the semicolon/chunk-continuation handling so routine bodies don't get split as separate statementsIF/ELSEIF/ELSE— reusing/extendingexp.IfBlockper review feedback on feat(trino): support inline SQL UDFs (WITH FUNCTION ... SELECT) [CLAUDE] #7926, instead of a new expression typeCASE...WHEN...END CASEWHILE...DO...END WHILE— reusing/extendingexp.WhileBlockwith alabelarg, per review feedbackLOOP/REPEAT/ITERATE/LEAVEPRs 4-7 depend on #3 (they all wrap bodies in
BEGIN...END); PR 2 and PR 3 are independent of each other, both depending only on this one. Let me know if this subdivision works better. I can "stack" the PRs here if that helps lay out the roadmap, I'll just have to deal with reabases. 😅Disclosure: implemented with Claude Code, reviewed, tested (
make unit,make style) and understood by me (again, I really do look at this stuff. See, it's me, a huuuuuuumannnnn!).