Skip to content

Change term filter into match query - #89

Merged
nickstenning merged 2 commits into
openannotation:masterfrom
Treora:change_term_match
Sep 25, 2014
Merged

Change term filter into match query#89
nickstenning merged 2 commits into
openannotation:masterfrom
Treora:change_term_match

Conversation

@Treora

@Treora Treora commented Aug 7, 2014

Copy link
Copy Markdown
Contributor

Another approach to issue #73, using the idea suggested in #77: instead of using a filter with exact terms, the approach is now to use a match query. This way fields by default are still analyzed when indexed (unlike in #88), but now queries are analyzed too. Search on any field that is not set to be not_analyzed in the mapping will do a case-insensitive full text search:
search?myfield=Alice will return annotations with myfield value alice
search?myfield=bla will return annotations with myfield value bla bla

(by the way, ignore the changes in annotation.py, they're just adaptation to the new query structure (although being more code than the main change))

@Treora Treora mentioned this pull request Aug 7, 2014
@nickstenning

Copy link
Copy Markdown
Member

I think I prefer the direction you're going in #88. This exposes more of elasticsearch's complexity through to the search API, but I'm not exactly clear on the user story.

@Treora

Treora commented Aug 10, 2014

Copy link
Copy Markdown
Contributor Author

I prefer #88 too. I rather have a very predictable default.

@Treora Treora closed this Aug 10, 2014
@Treora Treora reopened this Aug 18, 2014
@Treora

Treora commented Aug 18, 2014

Copy link
Copy Markdown
Contributor Author

I realised that also when using choosing to default to exact matches, it is still totally possible to - and there is still reason to - use a match query rather than a term filter. Fields that do specify a different analyser in their mapping will then work as intended, e.g. if you want case insensitivity for some fields, or full text search.
Only reason against may be performance, it could be that match queries are slower than term filters.

@tilgovi

tilgovi commented Aug 18, 2014

Copy link
Copy Markdown
Member

They are definitely slower because the analysis step is performed. I don't think that's a problem.

@gergely-ujvari

Copy link
Copy Markdown
Contributor

@Treora: A quick question here. I've checked the generated query, i.e.

{
  'sort': [{'updated': {'order': 'desc', 'ignore_unmapped': True}}], 
  'query': {
  'bool': {
   'must': [{'match_all': {}}, {'match': {'text': u'foo'}}, {'match': {'tag': u'bar'}}]}},
  'from': 0, 'size': 20}

And I don't think we should include the match_all if we have query k,v values to the generated query.
So here I would include an if statement about len(query) and only include the match_all part if it is empty.

@Treora

Treora commented Sep 24, 2014

Copy link
Copy Markdown
Contributor Author

Yes perhaps that is a nicer solution indeed. Fixed.

@gergely-ujvari

Copy link
Copy Markdown
Contributor

LGTM 👍

@nickstenning

Copy link
Copy Markdown
Member

This is looking pretty good, but I'm slightly nervous about merging this without any idea of what the performance impact is. Running the analysers on search seems like it could potentially be a pretty big overhead. I don't suppose either of you would be up for loading a test corpus and benchmarking an equivalent search before and after, would you?

@gergely-ujvari

Copy link
Copy Markdown
Contributor

@nickstenning: I agree that it can be an overhead, but it really depends on the analyzer. A wrongly-declared analyzer can be very slow, but my gutt feeling is, that, in normal cases, analyzing the search terms is not that costly. (Because the data in which we're searching is already indexed so alreay tokenized, and match query is just an inverse index lookup for the data)

But, if we want to make performance tests, we can maybe use this ES benchmark component

@Treora

Treora commented Sep 25, 2014

Copy link
Copy Markdown
Contributor Author

I don't expect a significant performance impact, because the default analyser is keyword (#88), which means not doing any analysis, so its behaviour should be just like doing a term query. Only when searching for the quote and text fields, the search strings will be tokenised and lowercased.

Of course, when different analysers are being specified, performance will change. But being able to specify different analysers is exactly the thing we want to enable with this PR.

@tilgovi

tilgovi commented Sep 25, 2014

Copy link
Copy Markdown
Member

👍 to what @Treora said.

@nickstenning

Copy link
Copy Markdown
Member

In which case...

nickstenning added a commit that referenced this pull request Sep 25, 2014
Change term filter into match query
@nickstenning
nickstenning merged commit dd86336 into openannotation:master Sep 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants