- Notifications
You must be signed in to change notification settings - Fork691
Make Oracle integration tests run and fix Oracle single statement execution code#2126
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Make Oracle integration tests run and fix Oracle single statement execution code#2126
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| /// <summary> | ||
| /// Splits a SQL script into individual statements, taking into account Oracle-specific syntax rules. | ||
| /// </summary> | ||
| privatestaticList<string>SplitOracleSqlStatements(stringsqlScript) |
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.
I just noticed (again) the existence of SemicolonSearcher, SqlBatchParser etc, I should use it, what do you think ?
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.
I tried to use it (with the same implementation as SnowflakeBatchParser) but it seems that it matches semicolons inside "ranges", in my case : strings 🫠
I'm too afraid to change it 😨
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.
I am not a big fan of the batch parser. sql is a very complicated dialect
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.
Oh I think I understood ... My string spans multiple lines (I'm in the long string tests for Oracle)
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.
I am not a big fan of the batch parser. sql is a very complicated dialect
Yeah, neither am I, but it still a lot better than the Regex previously used
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.
The simplest regex would be if the line ends in a;\s*$
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.
how to handle strings spaning multiple lines, containing a semicolon ? I'm sure the batch parser could be solid, but it needs to handle "ranges" spanning multiple lines, which it does not ATM.
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.
I finally gave up on using the SqlBatchParser, its handling of ranges line-by-line seems broken
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.
my regex hack makes it so long as the last non whitespace character on a line is a; , it will treat it as a separator.
| returnfalse; | ||
| } | ||
| if(_keywords.Contains(name)) |
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.
Why not just always quote identifiers?
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.
Because it would be too breaking. Quoted identifiers in Oracle become case sensitive 🙃
Needless to say it breaks everything
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.
Got it. I would put that in a separate PR though with a separate issue, right? Because currently we're already in case sensitive behavior, which I can see as non-desirable. We would need to list that in the release notes as a breaking change to move to case sensitive by default. We may also want a way to easily allow forcing case sensitive identifiers to maintain backward compatibility, right?
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.
Why not, but without it the integration tests on Oracle won't work, or I ignore them for now ?
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.
Ok, can you create a separate Issue and I will put it in the 7.2 milestone. I think GitHub will just generate release notes correctly.
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.
| /// <inheritdoc /> | ||
| publicoverrideIList<string>DatabaseTypeAliases{get;}=newList<string>() | ||
| {"Oracle-12c-Manager","Oracle 12c Managed","Oracle Managed","Oracle"}; | ||
| {"Oracle-12c-Managed","Oracle 12c Managed","Oracle Managed","Oracle"}; |
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.
😰
| /// <summary> | ||
| /// Splits a SQL script into individual statements, taking into account Oracle-specific syntax rules. | ||
| /// </summary> | ||
| privatestaticList<string>SplitOracleSqlStatements(stringsqlScript) |
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.
I am not a big fan of the batch parser. sql is a very complicated dialect
No description provided.