-
Notifications
You must be signed in to change notification settings - Fork 670
Patch for supporting backticks in GenericDialect #652
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
Patch for supporting backticks in GenericDialect #652
Conversation
Pull Request Test Coverage Report for Build 3275858885
💛 - Coveralls |
96c9b0d to
2da0843
Compare
|
I've pushed a cleaned up version here that should be fine. |
alamb
left a comment
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 the contribution @bitemyapp
| } | ||
|
|
||
| #[test] | ||
| fn parse_describe() { |
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.
Given this was a change to the generic Dialect (the default impl) can you also please add a test in parser_generic.rs as well?
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.
Yep, I think that makes sense. Thank you! 3rd kid in as many years born Monday (first daughter!), will get to it in a few days I think.
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.
3rd kid in as many years born Monday (first daughter!
Congratulations and good luck!
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.
can you also please add a test in parser_generic.rs as well?
I've added a generic test in the Hive test. parser_common was mostly about "all dialects", this change is just hive and generic. There are other Hive tests testing generic such that I factored out the dialect value creation into a function.
I think the remaining thing here is lifting the backtick out of the trait default implementation out to Hive and Generic's trait impls. What do you think?
|
Marking as draft so I know it isn't waiting on review anymore |
2da0843 to
271c332
Compare
271c332 to
0ec2794
Compare
|
Closing this PR as it is more than 6 months old without activity. Please reopen or make another PR if you plan to keep working on this. |
This was needed for the Athena SQL parsing I've been doing.
Fixes #632 632