Replace type_id() by trait method to allow wrapping dialects
#1065
Conversation
| let res1 = Parser::parse_sql(&MySqlDialect {}, statement); | ||
| let res2 = Parser::parse_sql(&WrappedDialect(MySqlDialect {}), statement); | ||
| assert!(res1.is_ok()); | ||
| assert_eq!(res1, res2); |
There was a problem hiding this comment.
Without overriding dialect(), this assertion fails with
Err(
TokenizerError(
"Unterminated string literal at Line: 1, Column 23",
),
)
Some of the dialect-specific parsing logic did not apply because we use a wrapped dialect `DialectWithParameters`, which has the wrong `type_id()`. Until apache/datafusion-sqlparser-rs#1065 gets merged, use a fork of `sqlparser` to make `DialectWithParameters` behave like the underlying dialect.
There was a problem hiding this comment.
This makes sense to me -- thank you @jjbayer
Ideally it would be nicer to have trait functions like supports_create_view() that control the parsers behavior rather than checking for specific dialects, as you mention, but that is a much larger change
|
|
||
| #[test] | ||
| fn parse_with_wrapped_dialect() { | ||
| /// Wrapper for a dialect. In a real-world example, this wrapper |
There was a problem hiding this comment.
👨🍳 👌 -- very nice example of creating a test illustrating the use case
Pull Request Test Coverage Report for Build 7210682589Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
|
@alamb thank you for accepting this PR, much appreciated! |
The `sqlparser` lib has been on a fork since #2846. We can now go back to the official version because apache/datafusion-sqlparser-rs#1065 has been merged & released.
The `sqlparser` lib has been on a fork since #2846. We can now go back to the official version because apache/datafusion-sqlparser-rs#1065 has been merged & released.
Part of the dialect-specific behavior of the parser is defined in the methods of the
Dialecttrait, but other parts of the behavior depend on thedialect_of!macro, which in turn checks theAny::type_idof the current dialect.Because if this, it is currently not possible to define variation of a dialect that is treated as the base dialect (e.g. MySQL) by the parser.
Ideally, all dialect-specific behavior would be defined in the implementation of the
Dialecttrait. As a workaround, this PR solves the problem by introducing an overridable trait method that returns theTypeId.