-
Notifications
You must be signed in to change notification settings - Fork 407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DuckDB Dialect Support #738
base: master
Are you sure you want to change the base?
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Thanks for the PR. A few quick questions and thoughts:
I'm pretty busy this week... not sure how much time I have to properly review this. |
For bonus points, you can update the wiki with information about DuckDB. That will also make it easier for me to review this. Otherwise I'll have to go and figure out all of this about DuckDB by myself. |
I was about to submit a PR also, I will make some comment in your code. |
Thanks for the comments above. The errors from the test suite are now down to five:
@nene - I'll look into filling out the wiki 👍 |
To fix
To fix the
I would guess the builtin PS. Make sure to run |
The test suite is passing completely now 🎉 I'll collate some notes for the wiki over the next week. Are there any other steps needed before merging? |
Thanks. I don't think there's anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay...
Took a bit of a look at this and noticed that several things (most notably all these operators) are not actually supported by DuckDB.
Also, when I simply compare it to PostgreSQL implementation, it looks almost the same (ignoring keywords and function names lists). But my very brief scanning of DuckDB documentation revealed several things that are different in DuckDB.
'EXPLAIN', | ||
'FETCH', | ||
'GRANT', | ||
'INSTALL', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like INSTALL
is the only name added to this list compared to PostgreSQL. I would suspect there are more differences by the statements supported by PostgreSQL and DuckDB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nene - you're correct. DuckDB uses the PostgreSQL parser. That's why DuckDB’s SQL dialect closely follows the conventions of the PostgreSQL dialect with only a few exceptions as listed here.
Many features are unsupported so I've removed unsupported elements in the formatter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nene - let me know if it's OK to resolve this conversation.
I've also added some specific DuckDB features below.
Hello, |
Thanks for the interest. This thing has indeed been sitting here for a while now. Will need to dig back into this to see if there's any reasons why it hasn't been merged yet. I think one of the main reasons was that it seemed really-really similar to PostgreSQL. So a question to you @Zank94, if you just configure SQL Formatter to treat your SQL as PostgreSQL, would that solve your problems (e.g. the |
I see, thank you for the advice I will give it a try 👍 |
This pull request adds support for the DuckDB SQL dialect to the SQL Formatter library.
Description:
Benefits:
Testing:
Please review the changes and provide feedback.