Support "with" identifiers surrounded by backticks in GenericDialect
#1010
Conversation
Pull Request Test Coverage Report for Build 6577799521
💛 - Coveralls |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @bitemyapp -- I have just a small test request
|
|
||
| fn generic(options: Option<ParserOptions>) -> TestedDialects { | ||
| TestedDialects { | ||
| dialects: vec![Box::new(GenericDialect {})], |
There was a problem hiding this comment.
Can you also please test HiveDialect given this is in the sqlparser_hive.rs module?
There was a problem hiding this comment.
Of course, good point. Thank you.
There was a problem hiding this comment.
I didn't find any test-cases that tested generic and not hive. Everything was either hive alone or hive and generic, e.g. the parse_create_function test. I'm not sure it makes sense to blend them given how they get used in the tests. I think it makes sense to test hive-isms that generic should be able to handle in the module that tests hive-isms rather than duplicating everything into the generic test module.
There was a problem hiding this comment.
How about something like this in sqlparser_common.rs?
(maybe we can do this as a follow on PR)
There was a problem hiding this comment.
I don't mind doing a follow-up PR.
Perhaps I could do something like parse_deeply_nested_expr_hits_recursion_limits in the common module but parameterized over the dialect, then invoke the helper in the generic & hive test modules? If that sounds acceptable let me know and I will pursue that.
There was a problem hiding this comment.
What I was suggesting was to remove generic() and test parse_describe with both hive and generic
alamb
changed the title
in GenericDialect`
alamb
changed the title
in GenericDialect`\ in GenericDialect`
alamb
changed the title
\ in GenericDialect` in GenericDialect`
alamb
changed the title
in GenericDialect` in GenericDialect`
alamb
changed the title
in GenericDialect`GenericDialect
bitemyapp
changed the title
GenericDialectGenericDialect
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thank you @bitemyapp
|
|
||
| fn generic(options: Option<ParserOptions>) -> TestedDialects { | ||
| TestedDialects { | ||
| dialects: vec![Box::new(GenericDialect {})], |
There was a problem hiding this comment.
How about something like this in sqlparser_common.rs?
(maybe we can do this as a follow on PR)
3 participants
I'm reviving this patch from #652
Please let me know what I'd need to do to make this merge-able. I'd like to be able to stop operating a fork of
sqlparser-rsfor parsing our Hive/Athena SQL so any direction you can give is much appreciated.parse_describevalidates the behavior for the hive and generic dialects.