Add support for snowflake exclusive create table options#1233
Add support for snowflake exclusive create table options#1233alamb merged 14 commits intoapache:mainfrom
Conversation
| .transient(transient) | ||
| .hive_formats(Some(Default::default())); |
There was a problem hiding this comment.
| .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.
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.
| } | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
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 |
alamb
left a comment
There was a problem hiding this comment.
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.
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.
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 |
iffyio
left a comment
There was a problem hiding this comment.
@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
alamb
left a comment
There was a problem hiding this comment.
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