Port the SQL parser from PDO

[morozov]

Q	A
Type	improvement
BC Break	no

Fixes: #4383

Additional Advantages:

Apart from fixing the issue with parsing comments, the new parser has the following advantages:

1. It is better extensible. It allows adding the support for platform-specific syntaxes (e.g. using a backslash for escaping in string literals, escaping PDO parameter placeholders) without changing the parser design.
2. It allows to separate the parsing and the transformation logic. Currently, the same parser is reused by the wrapper level connection (conversion of named and array parameters) and the OCI8 driver (conversion of positional parameters). In the future, it can be reused to address Named statement parameters are unnecessarily converted to positional on Oracle #3744 by combining multiple transformation steps on top of just one parsing pass, and only if necessary.

TODO:

• Only MySQL supports backslash escaping and only in strings, not in identifiers.
• Test escaping ] in SQL Server identifiers. Looks like SQL Server doesn't know how to handle them itself:

  SELECT QUOTENAME('?]?');
  -- [?]]?]
  SELECT 1 AS [?]]?]
  -- Parameter #1 has not been set.

• Test missing positional parameter.
• Test comments in SQL.
• See if we can get away with not having platform-specific configuration. Somehow PDO gets away with this. Otherwise, the Parser API will have to be declared public as a dependency of the Platform API. No. In fact, it's a bug (#80340).
• When testing escaping string literals, make sure both the ANSI and MySQL escaping syntaxes are tested evenly.
• Test the cases w/o escaping in string literals in both the ANSI and MySQL modes.
• Performance testing (see below).
• Integration testing on a real app (found QueryBuilder uses 1-based positional parameter keys #4421).
• Rework the internal API of the wrapper Connection. This @return array{0: …, 1: …, 2: …} looks ugly. This looks like too big of a change to make as part of this one. There's a lot of duplicated logic: logging, binding DBAL types, etc. that needs taking care of.

[morozov]

TL;DR The new parser is ~2.5 times slower than the old one (80μs vs. 30μs per query on synthetic tests).

Testing Methodology (gist):

1. Take the 29 queries from SQLParserUtilsTest::dataExpandListParameters().
2. Expand array parameters in each of them
3. Make sure that the query gets actually processed (no early return).
4. Repeat 10000 times.
5. Calculate the average.

Additional Considerations:

1. The performance penalty is inevitable due to the statefulness of the parser. Unlike the old implementation which does preg_match_all() once per query, this one does multiple preg_match() per query. This is necessary for the correct parsing of different token combinations (e.g. ignore parameters in comments, comments in string literals, etc).
2. The parser is only used when the query uses named or array parameters. The share of such queries depends on the project.
3. In the applications that use the relational database, the database is often the performance bottleneck. SQL queries can take 10-100 ms and it's rarely less than that. Adding 50μs to some of them (which is 0.05…0.5% in this case) doesn't look like a lot. Other application components like the web framework and the ORM will likely add more.

[greg0ire]

Should this come with changes under docs? Maybe an explanation on why the DBAL needs to have an SQL Parser, what features it allows us to implement?

[morozov]

Should this come with changes under docs?

I agree it would be a good addition but I don't think it's required for this PR to be accepted. The parser is not a user-facing API, so they normally don't have to know of its existence. I'd rather work on documenting it independently.

[greg0ire]

I performed a better search than I did before and found that this is actually already documented in https://github.com/doctrine/dbal/blob/3.0.x/docs/en/reference/data-retrieval-and-manipulation.rst#list-of-parameters-conversion

Approving just in case, but please proofread it for anything that should be changed.

[morozov]

I performed a better search [...]

Thanks for digging in. I don't see any mismatch between the new implementation and the current documentation. Except perhaps "a very powerful parsing process" which is now "an even more powerful parsing process".

[greg0ire]

A parsing process the likes of which you've never seen, frankly it's the best out there, everyone says it is!