Skip to content
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

Merged
merged 2 commits into from
Sep 15, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Sep 12, 2024

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:

> create table  foo(x int) cluster by x;
0 row(s) fetched.
Elapsed 0.009 seconds.

We should reject this with an explicit error rather than silently ignoring it

What changes are included in this PR?

Changes:

  1. Change sqlparser to error on fields which are currently ignored

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Sep 12, 2024
@@ -230,8 +239,149 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
with_options,
if_not_exists,
or_replace,
..
Copy link
Contributor Author

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.

Copy link
Contributor

@comphead comphead left a 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

@comphead
Copy link
Contributor

What do you think if I create a ticket so we can cover this in user doc as well?

@alamb
Copy link
Contributor Author

alamb commented Sep 13, 2024

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 TEMPORARY tables?

@alamb
Copy link
Contributor Author

alamb commented Sep 13, 2024

I plan to hold off on merging this PR until the 42 release is made, to avoid introducing some regression at the last moment

@comphead
Copy link
Contributor

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 TEMPORARY tables?

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

@alamb
Copy link
Contributor Author

alamb commented Sep 13, 2024

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 TEMPORARY tables?

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.

@phillipleblanc
Copy link
Contributor

strange syntaxes that sqlparser-rs might accept in the future

😆

@alamb
Copy link
Contributor Author

alamb commented Sep 14, 2024

strange syntaxes that sqlparser-rs might accept in the future

😆

One of my conclusions after reviewing many Prs in sqlparser-rs is that the breadth of SQL dialects is staggering. Staggering!!!

@alamb alamb merged commit 1e31093 into apache:main Sep 15, 2024
24 checks passed
@alamb
Copy link
Contributor Author

alamb commented Sep 15, 2024

Thanks again everyone!

@alamb alamb deleted the alamb/create_table_change branch September 15, 2024 12:01
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sql SQL Planner sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some CREATE TABLE and CREATE VIEW clauses are silently ignored by DataFusion
4 participants