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

fix: dont expand query on regular named arguments #6598

Draft
wants to merge 1 commit into
base: 3.9.x
Choose a base branch
from

Conversation

shyim
Copy link
Contributor

@shyim shyim commented Nov 14, 2024

Q A
Type bug/
Fixed issues

Summary

The SQLParser is always a bottleneck in Shopware Projects. I am optimizing it by preparing our queries so they do not use ArrayParameterType. While doing that, I discovered that it does that when any named parameter is set. Which is not necessary on MySQL. IDK about other.

Here the diff for our queries: (removing arrayparameters completely will remove the ~200ms on any req)
image

@shyim shyim changed the base branch from 4.2.x to 3.9.x November 14, 2024 03:12
@shyim shyim force-pushed the dont-expand-query-on-regular-named-arguments branch from 93859fc to ef1dafb Compare November 14, 2024 03:14
@morozov
Copy link
Member

morozov commented Nov 14, 2024

This is similar to #3744.

While doing that, I discovered that it does that when any named parameter is set. Which is not necessary on MySQL.

If you're saying that the conversion of named parameters to positional is unnecessary for MySQL, this is not entirely true. It's unnecessary for pdo_mysql (as well for any other PDO drivers) but is necessary for mysqli.

A proper fix is to have the DBAL driver (not the wrapper) convert the parameters to the format that the underlying driver understands, if necessary.

@shyim
Copy link
Contributor Author

shyim commented Nov 14, 2024

You mean move the method needsArrayParameterConversion kinda to the driver class and toggle it there?

@morozov
Copy link
Member

morozov commented Nov 14, 2024

You mean move the method needsArrayParameterConversion kinda to the driver class and toggle it there?

No, have the driver statement accept the parameters as is and do the conversion if necessary.

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.

2 participants