Skip to content

Add TrinoDialect - #2485

Open
rexminnis wants to merge 1 commit into
apache:mainfrom
rexminnis:trino-dialect
Open

rexminnis wants to merge 1 commit into
apache:mainfrom
rexminnis:trino-dialect

Conversation

@rexminnis

@rexminnis rexminnis commented Sep 6, 2026 •

Copy link
Copy Markdown

I'm building a governance platform (DRLS, say "drills") that puts a policy door in front of Trino: every query is
parsed, checked against row and column policies, rewritten, and re-serialized before Trino sees it.
We've been doing that with GenericDialect, which accepts far more than Trino does and rejects some
of what Trino accepts, so errors surface in the wrong place and rewrites don't always round-trip. This
PR adds a TrinoDialect so the parser and the engine agree.

What it does:

  • Double quotes are the only identifier delimiter; backquotes are a syntax error, as in Trino.
  • Enables the parser features Trino's grammar has: FILTER (WHERE …) on aggregates, expressions and
    grouping sets in GROUP BY, -> lambdas, MATCH_RECOGNIZE, parenthesised EXPLAIN options,
    => named arguments, COMMENT ON, and SHOW … FROM x LIKE ordering.
  • Lets words Trino does not reserve be aliases (FROM orders QUALIFY, SELECT 1 TOP), the way
    PostgreSqlDialect does. Only words that can't follow a table reference in Trino's own grammar are
    released; LIMIT, WINDOW, TABLESAMPLE and the like stay reserved because this parser doesn't
    backtrack.
  • Nothing else. Table versioning is deliberately not enabled: the existing hook accepts SQL Server's
    FOR SYSTEM_TIME AS OF and the TIMESTAMP AS OF forms, which Trino rejects. Trino's own
    FOR TIMESTAMP AS OF / FOR VERSION AS OF are a separate PR with a hook of their own (Parse FOR TIMESTAMP AS OF and FOR VERSION AS OF table versions #2486).

How it was checked, since I can't expect reviewers to know Trino:

  • Every statement in tests/sqlparser_trino.rs was run through Trino 483 as EXPLAIN (TYPE VALIDATE),
    which parses and analyses without executing. A SYNTAX_ERROR counts as a reject; anything else
    (including "table not found") means Trino parsed it. The verdicts are recorded in the test file in
    four tables: statements Trino parses (the dialect must too), statements Trino rejects (the dialect
    must too), statements Trino rejects that this parser accepts for every dialect with no hook to
    refuse them, and statements Trino parses that this parser can't — the last two listed so the gap is
    stated rather than discovered, each with an assertion that flips the day one is fixed.
  • Each enabled feature has at least one accepted and one rejected statement.
  • A mutation fuzzer (kept in my project, not here — it needs a running Trino) took those statements as
    seeds, generated 1,100 mutants over three runs (1,328 statements with the seeds), and compared both
    parsers. It found the alias gap above, now fixed. What's left: 5 statements Trino parses that the
    dialect rejects, all three of the listed kinds (an alias that is also a clause keyword, Trino's
    identifier 'string' typed literal, FINAL outside MATCH_RECOGNIZE), and 66 statements Trino
    rejects that the parser accepts, all parser-wide leniencies of the same kind as the listed ones
    (SHOW SHOW …, FROM 'orders', x x -> …).
    The ignored test differential_verdicts_from_file re-runs that comparison from the fuzzer's output
    file, so anyone with a Trino can repeat it: TRINO_VERDICTS=<file> cargo test --test sqlparser_trino -- --ignored --nocapture.

Known limits: ROW(name type, …) named-field types and inline WITH FUNCTION routines are not
covered; the misses and leniencies above are parser-wide, not something a dialect can change.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.99%. Comparing base (6862e71) to head (fd150b0).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2485      +/-   ##
==========================================
+ Coverage   80.96%   80.99%   +0.02%     
==========================================
  Files          42       43       +1     
  Lines       33386    33424      +38     
  Branches    33386    33424      +38     
==========================================
+ Hits        27032    27071      +39     
+ Misses       2789     2788       -1     
  Partials     3565     3565              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@edmondop edmondop left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Also this is an additive change that provides implementation of a dialect that many other people have implemented, so we should be good to merge

@rexminnis

Copy link
Copy Markdown
Author

Hi @LucaCappelletti94, could this get a committer review when you have a moment? All checks pass on the current head (fd150b0) and @edmondop approved it on Oct 1. It only adds a new dialect (TrinoDialect, registered as "trino") through the existing dialect hooks, plus a test corpus in tests/sqlparser_trino.rs; existing dialects are unchanged.

A note on #2486 (FOR TIMESTAMP AS OF / FOR VERSION AS OF): it's stacked on this PR. Its CI runs expired while waiting for workflow approval, so it hasn't been tested here yet. Once this one merges I'll rebase #2486 onto main so it shows only its own commit, and its CI can run then.

Thanks!

@LucaCappelletti94

Copy link
Copy Markdown
Contributor

Hi @rexminnis. I help maintain this project as a volunteer in my free time and I'm not paid for it. With 70+ open PRs, I spend my time on the ones I can actually review and that matter to my own work, i.e. PRs on dialects I use or dialects derived from those, where I know enough to judge what's right or wrong. I'm not familiar with Trino, so I can't give a new dialect the detailed review it needs.

Furthermore, this PR reads to me as largely LLM-generated without enough human review, which makes it require more scrutiny, not less.

I'm currently focusing on bug fixes, which need attention more urgently. Other reviewers, such as @alamb, which I have already pinged about this matter, are welcome to pick this up.

What would help any reviewer:

  • Rewrite the PR description in your own words.
  • Remove comments and documentation that don't add information.
  • Only enable features that are tested, and confirm each one against Trino's actual behavior.
  • Add tests for syntax Trino rejects, not only syntax it accepts.
  • If a reference Trino lexer/parser exists, a differential fuzzing harness plus seed corpus additions would be strong evidence of correctness.

Identifiers, FILTER, GROUP BY expressions, lambdas, MATCH_RECOGNIZE,
EXPLAIN options, named arguments, COMMENT ON, SHOW ordering and the
alias words Trino does not reserve. Every test statement carries the
verdict of a running Trino; an ignored differential test replays a
fuzzer's verdict file.
@rexminnis

Copy link
Copy Markdown
Author

Thanks — that's fair, and I appreciate you saying it plainly. I use this library in a product that sits
in front of Trino, so the dialect matters to me enough to do this properly rather than quickly.

Since your comment I've: removed the table-versioning hook (it was accepting syntax Trino rejects);
run every statement in the test file through a real Trino and recorded its verdict next to each; added
a rejected form for every enabled feature; listed the parser-wide leniencies Trino doesn't share
instead of leaving them to be found; stripped the comments that restated the code; and built a
mutation fuzzer against Trino whose output the ignored differential_verdicts_from_file test checks —
1,100 mutants; it found one real gap (Trino allows aliases this parser reserved), which is fixed, and
what remains is listed in the test file as known misses rather than left to be found. The description
is rewritten.

No rush, and no need for you to pick it up yourself — I understand the review budget. If @alamb or
anyone else has time, it should now be reviewable without knowing Trino in detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants