Make Oracle integration tests run and fix Oracle single statement execution code#2126
Conversation
| /// <summary> | ||
| /// Splits a SQL script into individual statements, taking into account Oracle-specific syntax rules. | ||
| /// </summary> | ||
| private static List<string> SplitOracleSqlStatements(string sqlScript) |
There was a problem hiding this comment.
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.
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.
I am not a big fan of the batch parser. sql is a very complicated dialect
There was a problem hiding this comment.
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.
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.
The simplest regex would be if the line ends in a ;\s*$
There was a problem hiding this comment.
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.
I finally gave up on using the SqlBatchParser, its handling of ranges line-by-line seems broken
There was a problem hiding this comment.
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.
| return false; | ||
| } | ||
|
|
||
| if (_keywords.Contains(name)) |
There was a problem hiding this comment.
Why not just always quote identifiers?
There was a problem hiding this comment.
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.
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.
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.
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.
| /// <inheritdoc /> | ||
| public override IList<string> DatabaseTypeAliases { get; } = new List<string>() | ||
| { "Oracle-12c-Manager", "Oracle 12c Managed", "Oracle Managed", "Oracle" }; | ||
| { "Oracle-12c-Managed", "Oracle 12c Managed", "Oracle Managed", "Oracle" }; |
| /// <summary> | ||
| /// Splits a SQL script into individual statements, taking into account Oracle-specific syntax rules. | ||
| /// </summary> | ||
| private static List<string> SplitOracleSqlStatements(string sqlScript) |
There was a problem hiding this comment.
I am not a big fan of the batch parser. sql is a very complicated dialect
3 participants
No description provided.