Skip to content
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

Parse SUBSTRING FROM syntax in all dialects, reflect change in the AST #1173

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

lovasoa
Copy link
Contributor

@lovasoa lovasoa commented Mar 13, 2024

always accept both substring function syntaxes in all dialects,
and ensure SUBSTRING expression are always formatted with the same syntax as the one they were parsed with.

This both simplifies the code (making it easier to maintain as databases evolve) AND creates a higher-fidelity syntax tree for dialects that support both syntaxes

fixes #1168

always accept both substring function syntaxes in all dialects,
and ensure SUBSTRING expression are always formatted with the same syntax as the one they were parsed with.

This both simplifies the code (making it easier to maintain as databases evolve) AND creates a higher-fidelity syntax tree for dialects that support both syntaxes

fixes  sqlparser-rs#1168
@coveralls
Copy link

coveralls commented Mar 13, 2024

Pull Request Test Coverage Report for Build 8266220365

Details

  • 19 of 20 (95.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 87.87%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser/mod.rs 14 15 93.33%
Totals Coverage Status
Change from base Build 8239291082: 0.01%
Covered Lines: 20486
Relevant Lines: 23314

💛 - Coveralls

@lovasoa
Copy link
Contributor Author

lovasoa commented Mar 13, 2024

@alamb : I think this is the best solution on the long term. This creates a higher-fidelity AST, and avoids having to maintain an up to date supports_substring_from_for_expr for all databases, which leads to issues like #1168

Copy link
Collaborator

@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.

I agree @lovasoa -- this design is consistent with the rest of sqlparser-rs and the design of "parse the syntax and leave semantics to the user"

Since it is now possible to distinguish between the two syntactic forms via AST I believe users can implement whatever behavior they desire.

THank you very much

@alamb alamb changed the title fix #1168 SUBSTRING expressions in all dialects, reflect change in the AST Mar 13, 2024
@alamb
Copy link
Collaborator

alamb commented Mar 13, 2024

Thanks again @lovasoa

@alamb alamb changed the title SUBSTRING expressions in all dialects, reflect change in the AST Parse SUBSTRING FROM syntax in all dialects, reflect change in the AST Mar 13, 2024
@alamb alamb merged commit 6b03a25 into sqlparser-rs:main Mar 13, 2024
10 checks passed
@lovasoa lovasoa deleted the fix-substring branch March 13, 2024 15:45
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.

SUBSTRING function calls are always formatted as SUBSTRING(... FROM ... FOR ...)
3 participants