Better statement splitting - #816
Open
icedtoast wants to merge 4 commits into
Open
Conversation
Collaborator
|
Hi, Thanks for taking an interest in this. Two quick thoughts while you're working:
|
Help make the SQL Server Batch Splitting more robust by adding test cases which don't work well with the current regex-based replacement. These are taken from chucknorris/roundhouse#82
icedtoast
force-pushed
the
better-statement-splitting
branch
from
August 18, 2026 02:31
c19099f to
12422f1
Compare
icedtoast
marked this pull request as ready for review
August 18, 2026 06:42
Contributor
Author
|
I've updated the description and I believe this is ready for review and merging (hopefully :-) ). |
icedtoast
marked this pull request as draft
August 18, 2026 06:54
Contributor
Author
|
Looks like there is an issue with the oracle tests, looks like I need to split on ';' as well. whoops |
icedtoast
force-pushed
the
better-statement-splitting
branch
2 times, most recently
from
August 18, 2026 20:57
b02ce3b to
4ae2040
Compare
icedtoast
marked this pull request as ready for review
August 18, 2026 21:01
Contributor
Author
|
I've changed this PR to be just SQL Server and the general refactoring, as the Oracle cases were becoming more complicated and need some more work. |
Contributor
Author
|
@wokket - this is ready for review :-) |
Collaborator
|
LGTM. Anyone else have feedback? |
wokket
reviewed
Aug 24, 2026
| "; | ||
| _testOutput.WriteLine(sql_to_match); | ||
| var result = Splitter.Split(sql_to_match).ToList(); | ||
| Assert.Equal(["--\n"], result); |
Collaborator
There was a problem hiding this comment.
This test (and the equiv in Oracle) are failing under windows because it receives \r\n... update with Environment.NewLine?
Contributor
Author
There was a problem hiding this comment.
I've updated these tests to be parameterized with the different line endings.
wokket
reviewed
Aug 24, 2026
| public class should_replace_on | ||
| { | ||
| private ITestOutputHelper _testOutput; | ||
| private SqlServerStatementSplitter Splitter; |
Collaborator
There was a problem hiding this comment.
nit: can this be _splitter for consistency please?
icedtoast
added a commit
to icedtoast/grate
that referenced
this pull request
Aug 24, 2026
Hide the implementation details that it is implemented by regular expressions to allow easier implementation of different techniques in the future. Also a note on the terminology, it is probably better named batch splitting, but I wanted to keep the implementation as close to how it was originally; hence the same naming. Furthermore, dependent code doesn't care *how* the statements are split, just that they are - so this further encapsulates that decision and is a better abstraction point. Notably some tests have changed from using raw string literals, instead being parameterized to accept the line endings in different environments (i.e. Windows and everything else). This helps the tests be more consistent. (Thanks to @wokket for pointing this issue out: grate-devs#816 (comment) )
icedtoast
added a commit
to icedtoast/grate
that referenced
this pull request
Aug 24, 2026
Although the Splitter name was inherited from the BatchSplitterReplacer tests, we want to more to more consistent naming used in the rest of grate. Thanks @wokket for the comment: grate-devs#816 (comment)
icedtoast
force-pushed
the
better-statement-splitting
branch
from
August 24, 2026 22:12
4ae2040 to
2194995
Compare
Hide the implementation details that it is implemented by regular expressions to allow easier implementation of different techniques in the future. Also a note on the terminology, it is probably better named batch splitting, but I wanted to keep the implementation as close to how it was originally; hence the same naming. Furthermore, dependent code doesn't care *how* the statements are split, just that they are - so this further encapsulates that decision and is a better abstraction point. Notably some tests have changed from using raw string literals, instead being parameterized to accept the line endings in different environments (i.e. Windows and everything else). This helps the tests be more consistent. (Thanks to @wokket for pointing this issue out: grate-devs#816 (comment) )
Although the Splitter name was inherited from the BatchSplitterReplacer tests, we want to more to more consistent naming used in the rest of grate. Thanks @wokket for the comment: grate-devs#816 (comment)
This handles nested comments and strings more reliably that a solely regex-based approach. It is also clearer. Regular expressions used to tokenize the sql statement, which is then parsed to ensure that it is only split when outside comments and string literals. A small behavior change is that tabs will longer be a batch on their own , i.e. whitespace only batches are dropped.
icedtoast
force-pushed
the
better-statement-splitting
branch
from
August 24, 2026 23:07
2194995 to
7e077e5
Compare
Contributor
Author
|
Updated based on your comments @wokket |
Contributor
Author
|
Note: It looks like the test failure is a connection timeout, unfortunately I can't rerun the tests on github. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is my PR chucknorris/roundhouse#82 to grate.
Briefly, it does cause minor behavior changes around whitespace at the beginning and ending of batches, however I don't think that will have any material affect.
In terms of the commits themselves I added the new failing tests seperate from the tokenizer implementations, to make it a bit clearer what the new cases that are covered are. Please let me know if I should split out the commits differently etc.
Thanks in advance for reviewing.