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

Remove use of LOWER from city & street searches #12991

Merged
merged 1 commit into from
Oct 29, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Improve performance and reliability of searches involving street address & city (under some circumstances) by relying on mysql to handle case.

Before

We actively convert the string to use lower case in php and then use mysql LOWER for the comparison

After

We let mysql do what it does well.

Technical Details

Per #12494 mysql handles lcase.

Adding LOWER to mysql queries makes them slower. lowercasing php strings
breaks under some character sets with some server configs. Less is more

Comments

@seamuslee001 @monishdeb @colemanw I think we should push through on these since we have been chipping away for a few months after this discussion #12494 with no fallout

@civibot
Copy link

civibot bot commented Oct 23, 2018

(Standard links)

@civibot civibot bot added the master label Oct 23, 2018
Adding LOWER to mysql queries makes them slower. lowercasing php strings
breaks under some character sets with some server configs.
@seamuslee001
Copy link
Contributor

Code change looks fine to me, the unit test has enough coverage here that i believe that if there was a problem here then Jenkins would have found it

@seamuslee001
Copy link
Contributor

Merging PR as i am ok with this

@seamuslee001 seamuslee001 merged commit 72e9f2d into civicrm:master Oct 29, 2018
@seamuslee001 seamuslee001 deleted the strtolower_city branch October 29, 2018 07:11
@eileenmcnaughton
Copy link
Contributor Author

Jenkins is good like that :-)

@mfb
Copy link
Contributor

mfb commented Nov 1, 2018

Worth noting that this fix also speeds up queries on many other fields, for example contribution transaction ID.

mfb added a commit to mfb/civicrm-core that referenced this pull request Nov 1, 2018
eileenmcnaughton added a commit that referenced this pull request Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants