Support formatting Trino queries #297
Conversation
| */ | ||
| // TODO | ||
| // https://github.com/trinodb/trino/blob/432d2897bdef99388c1a47188743a061c4ac1f34/core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4#L41 | ||
| const reservedCommands = [ |
There was a problem hiding this comment.
this needs more work, I mostly just went through the Spark formatter and kept the ones Trino shares with SparkSQL
There was a problem hiding this comment.
What's the relationship between Spark and Trino?
There was a problem hiding this comment.
There isn't one, I chose it pretty arbitrarily.
There was a problem hiding this comment.
In that case it would be better to base it on the sql.formatter.ts which mostly follows standard SQL.
There was a problem hiding this comment.
In my first comment, I am only referring to reservedCommands, the other variables I did by replacing the array completely with only Trino specific functions and keywords. I also added the commands I ran on the Trino code base to extract these lists, in hopes that you would double check my work before merging it :)
I guess I simplified what I said about what I did for reservedCommands, I didn't just remove the ones I didn't see in Trino. If I saw the "ALTER" keyword I grep'd for "ALTER" in the grammar and also added other Trino "ALTER" statements that SparkSQL doesn't have. On your suggestion, I just read through reservedCommands in sql.formatter.ts and all the statements in there I have already added. The only question I have is about SET and SET SCHEMA, what are those for? Trino has a bunch of "SET" like SET AUTHORIZATION, SET PROPERTIES, SET ROLE etc. and even UPDATE qualifiedName SET updateAssignment. Should all those be included in this list? I have no idea what makes sense here.
What really needs to be done is reading through Trino's grammar definition of statement
and figuring out where the formatter should split. Should I just list every top level keyword in that rule and any keywords that are are on their own in each rule? So like for
| CREATE ROLE name=identifier
(WITH ADMIN grantor)?
(IN catalog=identifier)? #createRoleI should add CREATE ROLE, WITH ADMIN and IN? Maybe not the last one (IN) though?
There was a problem hiding this comment.
I think, don't worry too much about these less used statements like CREATE ROLE, SET AUTHORIZATION etc. The formatter is currently lacking in its support for lots of these utility statements in various dialects. There are plans to review all these for all the dialects. So, feel free to either add them all or none of them.
The SET is for the SET in UPDATE table SET assignments.
Looks like Trino doesn't support SET SCHEMA, so don't add it :)
IN is best not to be added to the reservedCommands list. It would conflict with expressions like NOT IN.
verhovsky
changed the title
verhovsky
changed the title
|
Additionally, can you also add language tests (and documentation, if possible) |
| const language = 'trino'; | ||
| const format: FormatFn = (query, cfg = {}) => originalFormat(query, { ...cfg, language }); | ||
|
|
||
| // behavesLikeSqlFormatter(format); |
There was a problem hiding this comment.
Do we have a reason why these tests are commented out ?
There was a problem hiding this comment.
because they fail
|
As this PR seemed to have stalled a little and at the same time I did some cleanup and merged with latest changes in master in #325. It's not perfect, but neither is the support for all other dialects. A general support for Trino dialect is now present. Further improvements can be made in future PRs. |
|
Thanks for the effort you've put into this. 👍 |
|
This is now released in 9.0.0-beta3 |
No description provided.