Skip to content

Fix SUBSTRING from/to argument construction for mssql#947

Merged
alamb merged 1 commit into
apache:mainfrom
jmaness:feature/iss-914
Aug 17, 2023
Merged

Fix SUBSTRING from/to argument construction for mssql#947
alamb merged 1 commit into
apache:mainfrom
jmaness:feature/iss-914

Conversation

@jmaness
Copy link
Copy Markdown
Contributor

@jmaness jmaness commented Aug 14, 2023

PR for #914. This modifies the parsing of SUBSTRING for dialects like MsSqlServer which doesn't support the SUBSTRING(expr [FROM start] [FOR len]) syntax.

Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really nicely done PR @jmaness -- thank you very much for the contribution. The comments, are clear, as is the code, and the tests thorough. 🏆

Comment thread tests/sqlparser_common.rs
#[test]
fn parse_substring() {
one_statement_parses_to("SELECT SUBSTRING('1')", "SELECT SUBSTRING('1')");
let from_for_supported_dialects = TestedDialects {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is very nice

Comment thread src/parser.rs
expr: Box::new(expr),
substring_from: from_expr.map(Box::new),
substring_for: to_expr.map(Box::new),
special: !self.dialect.supports_substring_from_for_expr(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a minor nit hard coding this to false I think would be clearer for me:

Suggested change
special: !self.dialect.supports_substring_from_for_expr(),
special: false,

Likewise below.

No changes are needed in this PR -- I'll make a follow on one with the proposed change

@alamb alamb merged commit 8bbb853 into apache:main Aug 17, 2023
@jmaness jmaness deleted the feature/iss-914 branch October 6, 2023 19:38
serprex pushed a commit to serprex/sqlparser-rs that referenced this pull request Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants