-
Notifications
You must be signed in to change notification settings - Fork 821
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
ENH Do not use placeholders for int ID filters #10861
ENH Do not use placeholders for int ID filters #10861
Conversation
b39185c
to
94438aa
Compare
f5b0b09
to
5c213f6
Compare
6297fb3
to
eaeba64
Compare
3a427a5
to
676d2d4
Compare
2a17c51
to
c17ba22
Compare
src/ORM/Filters/ExactMatchFilter.php
Outdated
private function usePlaceholders(string $column, array $values): bool | ||
{ | ||
if ($this->config()->get('use_placeholders_for_integer_ids') | ||
|| !preg_match('#^"(.+)"."(.+)"$#', $column, $matches)) { |
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.
It's obviously doing more than just getting that value, since we're explicitly saying to use placeholders if the regex doesn't match. What scenarios will there be a column that doesn't match that pattern that we're 'protecting' against?
does this PR handle filtering against "MyRelation.ID"?
@GuySartorelli I added more explanation |
c17ba22
to
62542ed
Compare
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.
Works well locally.
Just one more test scenario to add please.
I've also created a separate but related issue: #10887
62542ed
to
6723968
Compare
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.
Sweet, merge on green
Issue #10860
Quering time on SC hosting doing a
->filter['ID' => $ids]
with 10,000 records - pretty consistent with initial laptop testing in parent issue where it's roughly 3x faster with placeholders turned offPlaceholders on - avg 0.1175s
0.1378s
0.0884s
0.1123s
0.1408s
0.1080s
Placesholders off - avg 0.0348s
0.0498s
0.0232s
0.0259s
0.0550s
0.0202s