feat(trino): parse routine characteristics for inline UDFs [CLAUDE] - #7981
Conversation
LANGUAGE, DETERMINISTIC/NOT DETERMINISTIC, CALLED/RETURNS NULL ON NULL INPUT, SECURITY DEFINER/INVOKER, and COMMENT now parse and generate correctly for WITH FUNCTION inline UDFs (tobymao#7934 was PR 1 of ~7).
Trino has no SQL SECURITY clause of its own; the merged base keyword otherwise swallows the SQL in LANGUAGE SQL SECURITY DEFINER.
|
Thanks for the PR, it's looking good! I noticed that the Trino UDF docs show an optional Let's include that in this PR so we have the full spec covered. Annoyingly, base sqlglot property handling expects either all or none of the properties to be wrapped in So the parsing would happen in three steps: We'll also need to add an arg to the Then, in the generator, generate the two sets separately. We won't always be able to retain the original order - it will always be WITH at the end. But that doesn't affect the semantics so should be fine. |
Per @treysp's review: the routine characteristics and the trailing WITH (property_name = expression, ...) clause need separate parsing, since WITH's entries are arbitrary key/value pairs rather than one of the fixed PROPERTY_PARSERS keywords the rest of the characteristics loop matches on. FunctionSpecification gets a new characteristics arg for the former; the existing properties arg now holds the WITH set.
|
Addressed in 87bd0a1. Split the routine characteristics from the trailing |
|
Thanks for the review @treysp - will open the next item in the lineup once this merges. Or, if y'all want to try the new Github PR Stack feature, this might be a fun excuse to do so. |
There was a problem hiding this comment.
There are several invalid tests, because we use catalog-stored UDF syntax in inline UDF declarations, which don't seem to support the full grammar spec.
If the machinery is reused for the former category of statements (catalog-stored UDFs), then it may be helpful adding tests for them and/or cleaning up existing invalid tests to ensure they represent runnable workloads.
Other than that the approach looks good.
- _parse_property() now intercepts NOT DETERMINISTIC directly instead of merging it into one token at the tokenizer level (as BigQuery does): the merged token misparsed SELECT NOT deterministic FROM t, silently swallowing NOT as a boolean operator. Scoped to Trino only. - Clarify/trim the two comments flagged as unclear or inaccurate. - Note in test_inline_udf that the SECURITY / WITH (...) cases assert grammar round-tripping, not that Trino accepts that combination on an inline SQL-language UDF specifically (confirmed against a real Trino instance in review).
|
Thanks for the thorough review, I think everything is addressed. Thanks also for actually running these against Trino, @georgesittas, it caught some legit issues:
Full unit suite (1257 tests) plus |
…E] (#8004) * feat(trino): parse BEGIN...END routine bodies with DECLARE/SET [CLAUDE] Trino inline UDFs can use a BEGIN...END compound body instead of a bare RETURN <expr>, with DECLARE (including Trino's multi-identifier-one-type form, DECLARE a, b, c type) and SET statements, and nested BEGIN blocks: https://trino.io/docs/current/udf/sql/begin.html https://trino.io/docs/current/udf/sql/declare.html https://trino.io/docs/current/udf/sql/set.html DECLARE/SET parsing and generation is entirely existing generic infrastructure (exp.Declare/DeclareItem/Set already support Trino's grammar as-is); Trino only needed its own DECLARE tokenizer keyword and a STATEMENT_PARSERS entry, mirroring BigQuery/Spark/T-SQL, plus DECLARE_DEFAULT_ASSIGNMENT = "DEFAULT" (also matching BigQuery) since the base default is "=". The routine body itself needed more care. _parse_block() (used for e.g. CREATE PROCEDURE ... BEGIN ... END) only recognizes its closing END when nothing follows it in the token stream, which doesn't fit Trino's grammar: the UDF's END is always followed by the enclosing query with no separating semicolon (... END SELECT f(1)). _parse_routine_block reuses the same underlying chunk-advancing primitives (_chunks/_advance_chunk) that back _parse_block's own semicolon/chunk-continuation handling, but closes on END regardless of what follows it, leaving the remaining tokens for the caller. block_sql wraps unconditionally in BEGIN ..., since every exp.Block in Trino's grammar is one (including nested ones, which a first pass missed by only prefixing BEGIN at the top level). _parse_routine_statement's fallback dispatches through the same STATEMENT_PARSERS entries _parse_statement() uses (for SET/DECLARE) rather than delegating to _parse_statement() itself, since that also carries a generic expression/SELECT fallback that would otherwise silently swallow whatever follows a malformed routine body as if it were the routine's own, rather than raising a clear parse error. Continues #7934/#7981. Remaining phases: IF/CASE/WHILE/LOOP control statements (reusing/extending exp.IfBlock and exp.WhileBlock, per review feedback on the original #7926). Related: apache/superset#26162. * docs(trino): trim the _parse_routine_block comment * test(trino): verify BEGIN...END tests against a real Trino instance All test_inline_udf_begin_end cases confirmed against Trino via Docker (trinodb/trino): - meaning_of_life() = 42, one() = 1, multi-identifier DECLARE, and doubled(x)+CTE all execute and return the expected values - Nested BEGIN confirmed to actually run (returns 2) - The LANGUAGE SQL + NOT DETERMINISTIC + BEGIN combination itself is valid and runs fine (confirmed separately with a RANDOM() body); the specific test case fails only because Trino's own function-body analysis flags this exact trivial body as deterministic despite the NOT DETERMINISTIC declaration, the same class of issue already noted for SECURITY/WITH (...) above. Updated the comment accordingly. * fix(trino): address review feedback on BEGIN/DECLARE/SET [CLAUDE] - DECLARE isn't a reserved word in Trino (confirmed: SELECT declare FROM t was already broken the same way for the pre-existing BigQuery/T-SQL/ Spark DECLARE keyword, since none of them add it to ID_VAR_TOKENS either). Adding the tokenizer keyword without this made DECLARE unusable as a plain column/alias identifier. Added DECLARE to Trino's own ID_VAR_TOKENS/TABLE_ALIAS_TOKENS rather than the shared base sets, since other dialects may have it genuinely reserved. - block_sql unconditionally prefixing BEGIN assumed every exp.Block is a Trino routine body, but Block is shared machinery other dialects also construct (e.g. via a fallback Command node when their own BEGIN handling doesn't fully parse). Traced the reported repro down to that: the BEGIN in the correct main output was already baked into a Command node's preserved text, not added by block_sql at all, so Trino's override doubled it. Fixed at the root instead of guessing by isinstance: added a begin arg to the shared exp.Block (mirroring the identical begin flag exp.Create already carries for this same purpose), handled generically in the base block_sql, and set explicitly by _parse_routine_block, which is the only place that should synthesize one. No other dialect's Block ever sets this, so their rendering is unaffected. Both confirmed against the exact repros from review, full suite (1262 tests) green, and BigQuery/Snowflake/T-SQL/Spark's own Block-based tests unaffected by the base generator change. * Taking suggestion on comments... Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com> * fix(trino): incorporate georgesittas's review feedback [CLAUDE] - DECLARE-as-identifier is valid in BigQuery, Presto, Trino, Athena, and Spark, only T-SQL genuinely reserves it. Moved TokenType.DECLARE into base ID_VAR_TOKENS/STATEMENT_PARSERS instead of a Trino-only override, so all of those dialects pick it up (T-SQL becomes more permissive parse-wise, which is fine per review). TABLE_ALIAS_TOKENS needs no separate change since it derives from ID_VAR_TOKENS in base already. - Reworded the _parse_routine_block comment per suggestion, and restructured its loop into a while/else (matching the suggested diff): the loop condition itself now checks for END, so the else clause naturally fires only when END was actually found rather than on any other break. - Replaced the STATEMENT_PARSERS-lookup fallback in _parse_routine_statement with explicit DECLARE/SET branches per review: none of the later phases (WHILE, REPEAT, ...) go through STATEMENT_PARSERS either, so keeping generic dispatch for just these two was unnecessary indirection. - Rebuilt the block_sql regression test to construct the AST directly instead of parsing through BigQuery's Command fallback, removing the unrelated parser warning noise from the test output. Full suite (1262 tests) green, including T-SQL/BigQuery/Spark/Presto/ Athena's own test suites specifically re-run for the ID_VAR_TOKENS change. --------- Co-authored-by: Jo <46752250+georgesittas@users.noreply.github.com>
I'm back! :)
PR 2 of 7-ish, continuing to build on #7934.
Adds parsing/generation for the routine characteristics that can follow a Trino inline
WITH FUNCTIONUDF'sRETURNSclause:LANGUAGE,DETERMINISTIC/NOT DETERMINISTIC,CALLED ON NULL INPUT/RETURNS NULL ON NULL INPUT,SECURITY DEFINER/INVOKER, andCOMMENT(https://trino.io/docs/current/udf/sql.html):Most of this already worked for free:
_parse_function_specification(from #7934) calls the base_parse_properties()betweenRETURNSandRETURN, soLANGUAGE,CALLED/RETURNS NULL ON NULL INPUT,SECURITY DEFINER/INVOKER, andCOMMENTall parsed and generated correctly with zero new code, via existingexp.LanguageProperty/exp.CalledOnNullInputProperty/exp.SqlSecurityProperty/exp.SchemaCommentProperty+ their existing generic generators.Two things actually needed fixing:
DETERMINISTIC/NOT DETERMINISTIC— reusedexp.StabilityProperty(base already parses bareDETERMINISTICinto it), and mirrored BigQuery's existingStabilityPropertyhandling exactly: aNOT DETERMINISTICentry inTrinoParser.PROPERTY_PARSERS, the sameTrinoGenerator.TRANSFORMSlambda BigQuery uses, and the same tokenizer-level"NOT DETERMINISTIC"keyword-merge BigQuery has (_match_textsonly inspects one token, so the two words need to merge into one before the parser can dispatch on them). Before this, bareDETERMINISTICround-tripped as the invalid-for-TrinoIMMUTABLE, andNOT DETERMINISTICdidn't parse at all.LANGUAGE SQLimmediately followed by aSECURITYclause — the base tokenizer already mergesSQL SECURITYinto oneTokenType.SQL_SECURITYtoken (for MySQL/StarRocks'... SQL SECURITY DEFINER VIEW), which greedily ate theSQLthat was supposed to beLANGUAGE's value, e.g.LANGUAGE SQL SECURITY DEFINERtokenized asLANGUAGE/SQL SECURITY/DEFINERinstead ofLANGUAGE/SQL/SECURITY/DEFINER. Trino has noSQL SECURITYphrase in its own grammar (only bareSECURITY DEFINER/INVOKER), and the base parser'sPROPERTY_PARSERSalready maps plain"SECURITY"to the identical_parse_sql_securitythe merged token maps to, soTrino.Tokenizerjust pops the merged keyword (same idiom as its existingKEYWORDS.pop("/*+")on the Presto side, or BigQuery'sKEYWORDS.pop("DIV")). Confirmed this doesn't affect MySQL/StarRocks, whose tokenizers are untouched.Next steps (remaining follow-up PRs, each gated on the previous)
Core(feat(trino): parse WITH FUNCTION ... RETURNS ... RETURN inline UDFs [CLAUDE] #7934)WITH FUNCTION ... RETURNS ... RETURN ...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.IfBlockCASE...WHEN...END CASEWHILE...DO...END WHILE— reusing/extendingexp.WhileBlockwith alabelargLOOP/REPEAT/ITERATE/LEAVERelated: apache/superset#26162 (Superset issue asking for Trino inline SQL UDF support end-to-end).
Disclosure: implemented with Claude Code, reviewed, tested (
make unit,make style) and understood by me.