Repository navigation
Add TrinoDialect - #2485
Add TrinoDialect#2485rexminnis wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
|
Hi @LucaCappelletti94, could this get a committer review when you have a moment? All checks pass on the current head ( A note on #2486 ( Thanks! |
|
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:
|
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.
fd150b0 to
d0b6a6f
Compare
|
Thanks — that's fair, and I appreciate you saying it plainly. I use this library in a product that sits Since your comment I've: removed the table-versioning hook (it was accepting syntax Trino rejects); No rush, and no need for you to pick it up yourself — I understand the review budget. If @alamb or |
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 someof what Trino accepts, so errors surface in the wrong place and rewrites don't always round-trip. This
PR adds a
TrinoDialectso the parser and the engine agree.What it does:
FILTER (WHERE …)on aggregates, expressions andgrouping sets in
GROUP BY,->lambdas,MATCH_RECOGNIZE, parenthesisedEXPLAINoptions,=>named arguments,COMMENT ON, andSHOW … FROM x LIKEordering.FROM orders QUALIFY,SELECT 1 TOP), the wayPostgreSqlDialectdoes. Only words that can't follow a table reference in Trino's own grammar arereleased;
LIMIT,WINDOW,TABLESAMPLEand the like stay reserved because this parser doesn'tbacktrack.
FOR SYSTEM_TIME AS OFand theTIMESTAMP AS OFforms, which Trino rejects. Trino's ownFOR TIMESTAMP AS OF/FOR VERSION AS OFare 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:
tests/sqlparser_trino.rswas run through Trino 483 asEXPLAIN (TYPE VALIDATE),which parses and analyses without executing. A
SYNTAX_ERRORcounts 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.
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,FINALoutsideMATCH_RECOGNIZE), and 66 statements Trinorejects 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_filere-runs that comparison from the fuzzer's outputfile, 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 inlineWITH FUNCTIONroutines are notcovered; the misses and leniencies above are parser-wide, not something a dialect can change.