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

Standardize and improve search functionality #476

Merged
merged 1 commit into from
Mar 7, 2017
Merged

Conversation

tgianos
Copy link
Contributor

@tgianos tgianos commented Mar 7, 2017

This PR addresses two issues:

  1. Job search by id and name was degrading at large job database size. It was performing a LIKE no matter what string was passed in. Now the code searches for the presence of the % character and if it's not present it switches that predicate to be an EQUAL

  2. String fields across resources were treated differently. Some were defaulting to EQUAL predicates and some to LIKE predicates. Now all string fields supported by the API for search will use EQUAL by default and switch to LIKE if a % is present

@tgianos tgianos added this to the 3.0.2 milestone Mar 7, 2017
@tgianos tgianos self-assigned this Mar 7, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 89.191% when pulling b8ef4db on tgianos:3.0.x into aba7ab6 on Netflix:3.0.x.

@NotNull final Expression<String> expression,
@NotNull final String value
) {
if (StringUtils.contains(value, PERCENT)) {
Copy link
Contributor

@ajoymajumdar ajoymajumdar Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we differentiate '%' and '%'?

@tgianos tgianos merged commit 008b8bd into Netflix:3.0.x Mar 7, 2017
tgianos added a commit to tgianos/genie that referenced this pull request Mar 7, 2017
@skwslide
Copy link
Contributor

skwslide commented Mar 8, 2017

What about the underscore character? It's a valid wildcard character for LIKE.

@tgianos
Copy link
Contributor Author

tgianos commented Mar 8, 2017

@skwslide the scope of this work wasn't to address the edge cases that exist in the characters it was to:
a) improve the speed of job search
b) consistently handle all strings in search APIs

From an API perspective we shouldn't bleed through underlying implementation details like what the backend data store wildcards are. For now we've always used the % character in the UI to represent the wildcard character so this PR continues that assumption at an API level. At an API level we don't support other wildcard characters right now. To put in more advanced backend storage wildcard parsing / edge case handling we'd have to work on that in a separate body of work

@skwslide
Copy link
Contributor

skwslide commented Mar 8, 2017

@tgianos, you want to expose only % as wildcard, and I agree with that. (I'm fine with having only % and having both % and _ as wildcards.) Two points to note:

  1. Before this PR, _ was a wildcard. So, the first half of this PR is not just a performance improvement but also a change in semantics (regardless whether the old semantics is wanted or unwanted).

  2. The new semantics is that _ is a wildcard if and only if % is present in the string. Therefore, this PR is still bleeding through some of the underlying implementation details and is just partway toward exposing only % as wildcard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants