-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Do not silently ignore unsupported CREATE TABLE
and CREATE VIEW
syntax
#12450
Conversation
@@ -230,8 +239,149 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
with_options, | |||
if_not_exists, | |||
or_replace, | |||
.. |
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.
I removed the ..
from this match statement and then let github copilot write the checks for all the clauses.
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.
Thanks for doing this. Now it is clear what exactly not yet supported
What do you think if I create a ticket so we can cover this in user doc as well? |
Sure -- sounds like a good idea. What do you mean by "this"? Do you mean how DataFusion handles |
I plan to hold off on merging this PR until the 42 release is made, to avoid introducing some regression at the last moment |
The idea was to document which features out of Create Table are not supported. Now it can be described pretty easily from the code you created |
I see -- I think my preference would be to document what features are supported (likely because I am being lazy) as well as the fact that the number of strange syntaxes that sqlparser-rs might accept in the future is not bounded. |
😆 |
One of my conclusions after reviewing many Prs in sqlparser-rs is that the breadth of SQL dialects is staggering. Staggering!!! |
Thanks again everyone! |
Which issue does this PR close?
Closes #12449
Rationale for this change
@hailelagi fixed the bug for
TEMPORARY
in #12439 🙏While reviewing #12439 I noticed there are some other clauses that are ignored too
For example the
CLUSTER BY
clause is totally ignored:We should reject this with an explicit error rather than silently ignoring it
What changes are included in this PR?
Changes:
Are these changes tested?
Are there any user-facing changes?