-
Notifications
You must be signed in to change notification settings - Fork 8.5k
Remove simple_query_string hack now that multi_match supports * properly #13285
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
Conversation
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.
Looks good 👍 While looking at the semantics of multi_match I also stumbled across zero_terms_query, which looks interesting. Without it searching for things like "" and "." yields no results even though every document "contains" the empty string. That is not really intuitive unless you know how the analyzer works in the background. @Bargs what do you think?
|
My gut reaction, which could be totally wrong, is that I like the fact that no results are returned. It at least feels consistent to me. Analyzers definitely trip people up, I've had to explain them a number of times when users had queries behaving in unexpected ways. But zero_terms seems like an additional layer of magic we'd have to explain when things are acting funky. |
| all_fields: true | ||
| multi_match: { | ||
| query: value, | ||
| fields: ['*'], |
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.
If we rewrite this as fields: [fieldName], then couldn't we remove this piece of code? Or is there some benefit to using a match_phrase over a multi_match?
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.
There are 2 potential reasons I can think of to hold off on that for now:
lenientisn't necessary, and is perhaps undesirable when querying against a single field- Passing the field name straight through to multi_match would allow people to use leading wildcards and boosting which I don't think we should support just yet
lukasolson
left a comment
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.
LGTM!
Previously
fields: ["*"]on a multi_match query would throw errors because it tried to search non-searchable fields. We agreed to use the simple_query_string query in all_fields mode as a workaround until that issue was fixed in ES. That fix has landed so I'm removing the workaround and using the multi_match query instead.I also tried to make a few tests that broke with this change a bit more robust.