-
Notifications
You must be signed in to change notification settings - Fork 601
MsSQL TRY_CONVERT #1477
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
MsSQL TRY_CONVERT #1477
Conversation
src/parser/mod.rs
Outdated
@@ -1023,7 +1023,8 @@ impl<'a> Parser<'a> { | |||
self.parse_time_functions(ObjectName(vec![w.to_ident()])) | |||
} | |||
Keyword::CASE => self.parse_case_expr(), | |||
Keyword::CONVERT => self.parse_convert_expr(), | |||
Keyword::CONVERT => self.parse_convert_expr(false), | |||
Keyword::TRY_CONVERT if dialect_of!(self is MsSqlDialect) => self.parse_convert_expr(true), |
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 we have the guard as a dialect method e.g. self.dialect.supports_try_convert()
? We can probably include support for the generic dialect as well
tests/sqlparser_mssql.rs
Outdated
@@ -495,6 +497,8 @@ fn parse_convert() { | |||
ParserError::ParserError("Expected: an expression, found: )".to_owned()), | |||
ms().parse_sql_statements(error_sql).unwrap_err() | |||
); | |||
|
|||
ms().verified_expr("TRY_CONVERT(VARCHAR(MAX), 'foo')"); |
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.
with the dialect method we can move this over to common.rs as a generic test over the dialects that support the feature
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.
@yoavcloud can we also add the flag to generic dialect? Other than that LGTM!
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.
LGTM! cc @alamb
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 @yoavcloud and @iffyio
a179a21
to
fa8e9c8
Compare
Thanks agian @yoavcloud and @iffyio |
Pull Request Test Coverage Report for Build 11429342019Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
This PR adds support to parse the MsSQL TRY_CONVERT function, similar to CONVERT