-
Notifications
You must be signed in to change notification settings - Fork 566
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 support for snowflake exclusive create table options #1233
Conversation
src/dialect/snowflake.rs
Outdated
.transient(transient) | ||
.hive_formats(Some(Default::default())); |
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.
.transient(transient) | |
.hive_formats(Some(Default::default())); | |
.transient(transient); |
Is hive_formats
needed in this case or it can be set to None?
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.
The hive_formats
is tested in a generic test against all dialects, since I don't know which dialect uses the hive_format
I added the default value in the return struct to keep that test happy.
@@ -40,6 +40,213 @@ fn test_snowflake_create_table() { | |||
} | |||
} | |||
|
|||
#[test] |
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.
general comment: can we include some negative tests as well? examples like incompatible keywords being used, incomplete keyword sequence etc
Pull Request Test Coverage Report for Build 9423048544Details
💛 - Coveralls |
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 @balliegojr, I think this is looking really close to me.
A few more comments and I think it will be ready to go
src/ast/mod.rs
Outdated
} | ||
} | ||
|
||
/// Helper to indicate if a collection should be wrapped by a symbol when displaying |
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.
Some examples would be nice here too.
Also perhaps we should implement Display for this enum which would simplify the callsites
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 have implemented Display
for Vec<T>
only, I believe the HashMap
may have different representation depending on the dialect/use case, but let me know if I should implement Display for the HashMap as well
Thank you @iffyio for the review |
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.
@balliegojr sorry for the delay getting back to this PR
The changes LGTM! only the minor comment regarding the clickhouse_generic test and a conflicts to resolve from main
a48498f
to
a5b771d
Compare
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.
@alamb , I think we can merge this
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.
Epic work @balliegojr -- thank you so much
Thank you for the review @lovasoa
🤔 looks like there are some CI failures,. |
a5b771d
to
e1452bc
Compare
@alamb I believe I solved the CI issues |
Thanks again @iffyio @balliegojr and @lovasoa |
Add support for snowflake exclusive create table options
Since snowflake does not require a strict order in the statement, the create table logic was moved to inside the snowflake dialect