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

Option to enable case sensitive requests (performance) #238

Closed
wants to merge 3 commits into from
Closed

Option to enable case sensitive requests (performance) #238

wants to merge 3 commits into from

Conversation

nicoespeon
Copy link
Contributor

Hi there,

The goal here was to deal with the old issue of strings search. Indeed, we are currently facing slow queries in our project — large Mongo collections used there — which are due because of the RegExp parsing. We actually want to perform searchs with the string with provide, which means case sensitive requests.

After digging into issues/PR of the project, I made this modification according to what you specified in PR #235 :

  • configuration flag to be turned on to enable the feature, don't break regular functionality
  • use wlNext config convention, following the work you started over waterline sequel (see balderdashy/waterline-sequel@62ec93e)

I tested this branch against our project, and I didn't find any issue for now. Doing so I found out that you still need to perform the RegExp conversion when dealing with a modifier (e.g. $not) since Mongo doesn't allow a string but either a RegExp or a document (see http://docs.mongodb.org/manual/reference/operator/query/not/).

Please let me know if something is wrong.
I personally have one test failing, but it also fails on master branch, which I don't understand for now. I may need to add some unit tests too. Just let me know ;-)

Thanks.

It's false by default to keep consistency with current version.
Difference is you can now turn it to true, which would prevent strings to be parsed automatically into RegExp. That was necessary because of waterline SQL origin which was case insensitive. However, turning every strings into RegExp invalid any index you can set in your mongo database, which is really bad for performance.
@nicoespeon
Copy link
Contributor Author

Looks like there is a test failing since #211. It seems to be failing even on master branch for me. Maybe changed specs?

@dylanbathurst
Copy link

Has there been any updates to this PR? We're now experiencing the same issue where queries on string fields are slowing down quite a bit due to waterline applying a regexp to the query instead of just doing a string search.

I see that others have gotten around this by using the Model.native() function but then you're missing out on all the other Waterline goodness.

@devinivy
Copy link
Contributor

I'm a fan of getting this rebased, tested, and through. The work done here basically looks good to me. Can @particlebanana weigh in? Worth referencing #235. @nicoespeon if this gets rebased we can re-run the tests against a passing master branch.

@particlebanana
Copy link
Contributor

Yeah this would be great to get in. We are going to need a feature test in Waterline-Adapter-Tests that is basically the opposite of this. I love it.

@particlebanana particlebanana mentioned this pull request Jan 25, 2016
@particlebanana
Copy link
Contributor

Cherry picked these in #380

This was referenced Jan 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants