feat(autocomplete): filter out address records for character counts below threshold

[missinglink]

READY TO MERGE

This PR will allow us to provide a negative layer filter so that autocomplete will not search on the address layer for one or two character inputs.

After testing this I discovered that must_not conditions do not create a bitmask as filter conditions do, so what ends up happening is that the same amount of hits are returned and then an iterator is generated to run over the results of the must and enforce the must_not.

tl;dr must_not is super slow for large hit counts and lots of people on the internet are complaining about how it's not very well implemented.

So I've rewritten the query to use a positive filter, we compute the difference between all layers and the excluded layers. Fortunately this is now super easy to do since the work I did a couple months back to clean up the type_mapping module :)

Well covered with unit tests and fixtures, I played with it a bit on North America and couldn't see any real difference in perceived quality for one-or-two char inputs.

The effect of this will be a huge improvement in performance for one and two character queries, the address layer accounts for ~90% of matches.

[missinglink]

@orangejulius I'm happy with this as-is, ready to merge unless you have any concerns?

[orangejulius]

oh, fascinating regarding the performance of must_not. Can you share some of the sources you found on the performance? I'm sure we'll look at them again.

[missinglink]

here are a few of the top hits I found on Google for "elasticsearch must_not slow"

• https://discuss.elastic.co/t/boolean-must-not-query-is-having-slow-performance/64781
• https://discuss.elastic.co/t/must-not-in-bool-filter-much-slower-than-must-for-same-terms-filter/1479
• https://stackoverflow.com/questions/42166384/elastic-search-must-not-queries-are-slow

[missinglink]

In particular this comment: https://discuss.elastic.co/t/must-not-in-bool-filter-much-slower-than-must-for-same-terms-filter/1479/13

must_not does indeed not translate to "must": {"not":{}}.

If you run a bool query with must/should and must_not clauses, Lucene will first create an iterator that matches the must/should clauses, and then if you have must_not clauses, this iterator will be wrapped in order to exclude documents that match any of these must_not clauses.

Let me take an example: you have 1M documents in your index, and 1000 of them contain bar in the foo field. If you want to find all documents that match foo:bar, Lucene will just iterate over the postings list of foo:bar and call the collector on it. So you would decode 1000 documents from your postings list and call the collector 1000 times. Now if you execute the same clause as a must_not filter and have a match_all query as a must clause, Lucene will iterate over all documents matching the match_all query, and for each of them check if they match foo:bar and should be excluded. So you have to check 1M times if the document matches foo:bar and call the collector 999000 times.

This is why when you have a boolean field, it is more efficient to encode true and false explicitely instead of only building an index for true and then searching for documents that have false as a value by searching for documents that don't have true.

So essentially if you want fast must_nots it would probably be quicker to do that noting in application code and query elasticsearch with a must with a list of all possible values excluding the ones in must_nots?

This depends on how many possible values you are. If there are only a handful of them then this could help, but if there are thousands of possible values, this would not be an option.

[orangejulius]

I had a chance to test this out against a full planet build yesterday. It looks super promising, the highest number of Elasticsearch hits I saw was 10 million, down from ~130M without. I'm working on some code to let us configure the threshold before which address results are filtered out. Once that looks good, we can and should merge this!

[orangejulius]

I just pushed a change to this branch that drives text length value from config rather than hardcoding it to two. It defaults to zero, making this PR a no-op by default.

Some new helper methods from pelias/config#104 were required to test this, so the pelias-config version requirement is bumped as well.

[orangejulius]

After some testing, this is looking very good!

Take a look at those awesome drops in p50 and p99 latency using this branch 🎉 🎈

[orangejulius]

Another update, after testing with autocomplete acceptance tests, it appears setting the threshold to 3 on this PR is also safe. After a few hours of running some traffic through it, the results once again look great!

Notably, this change brings all p99 latencies to well below 1000ms