Default to 'and' operator for match queries. Remove slug search, prioritize exact matches more. by EnTeQuAk · Pull Request #7303 · mozilla/addons-server

EnTeQuAk · 2018-01-15T18:19:56Z

prioritize a match on name.raw higher by moving it up in the rules list
Add operator: and to non fuzzy match queries, this doesn't do anything to match_phrase but this should already improve matching quite a bit
Removed keyword filtering from legacy search api, e.g "delicious platform:mac" doesn't work anymore.
Added a new flag "search-use-dfs-query-then-fetch" to make use of support for dfs-query-then-fetch search type: https://www.elastic.co/blog/understanding-query-then-fetch-vs-dfs-query-then-fetch
Made sure we only have one shard during testing to make our search-scoring tests more predictable

References many "component: search" issues. What I tested with a
database of all public add-ons:

Example searches:

tab center redux - should find "Tab Center Redux" while "Tab Mix Plus" is probably second and "Redux DevTools" 4th or so
Open Image in New Tab -> should find "Open Image in New Tab" while "Open Bookmarks in New Tab" should be 2nd or 3rd
CoinHive -> Finds "Coinhive Blocker", "CoinBlock" (prefix search) and "NoMiners" (description)
Privacy -> Finds "Privacy Badger", "Privacy Pass", "Privacy Settings", "Google Privacy" (probably 4th or so) and "Blur" (summary + description + many users). Scores "Ghostery" on the first page but ranks it in the middle
firebu -> Finds "Firebug", "Firebug Autocompleter", "Firefinder for Firebug"
fireb -> Scores "Fire Drag" first, puts "Firebug" approximately 3rd or so
Menu Wizzard -> Finds "Menu Wizard" (fuzzy, typo) first, then "Add-ons Manager Context Menu" apparently because it matches good in the title and has many users
Frame Demolition -> Finds "Frame Demolition"
Demolition -> Finds only "Frame Demolition", same for "Demolation" (typo)
reStyle -> Finds "reStyle" and scores a few add-ons that match on "restore" next since the term is similar
MegaUpload DownloadHelper -> finds "MegaUpload DownloadHelper" first, scores "Video * DownloadHelper" and "RadpidShare DownloadHelper" next. Doesn't find "Popup Blocker" anymore as currently happening on -prod
MegaUpload -> only finds "MegaUpload DownloadHelper" and nothing else
No Flash -> Scores "No Flash" first, then depending on users "Download Flash and Video", "YouTube Flash Video Player" and "YouTube Flash Player" (not necessarily in that order)
Disable Hello, Pocket & Reader+ -> finds "Disable Hello, Pocket & Reader+" first (yeay!), then scores "Reader", "Disable WebRTC" and "In My Pocket" next similarly to what's happening on -prod currently

Please note that these are more "Guidelines", actual results in production may differ depending on shard configuration and document spreading. We might want to think about using https://www.elastic.co/guide/en/elasticsearch/reference/master/search-request-search-type.html#dfs-query-then-fetch which doesn't sound too bad imho, might hurt performance though. One step at a time.

Not working yet:

privacybadger -> "Privacy Badger" -> will probably need some kind of ngram filtering and analyzing (Auto sign beta files and log them (bug 1172035) #591)
eyes -> 'decentraleyes' -> Not sure this should actually work, will probably need some more analyzing too (Auto sign beta files and log them (bug 1172035) #591)

Not sure if it's specifically only because of these changes but mozilla/addons#3378 is fixed.

This potentially fixes mozilla/addons#5141, mozilla/addons#5004, mozilla/addons#4977, mozilla/addons#4778, mozilla/addons#359.

Not sure if this fixes mozilla/addons#567 but the results look much more promising and the amount of results doesn't explode here. I only have 2.8k add-ons for testing though so I'm not too sure.

And might be relevant to mozilla/addons#4652.

This is a big step towards mozilla/addons#3097, I doubt we can call this fixed though.

Script used to download public add-ons locally and test above searches: https://gist.github.com/EnTeQuAk/7f74c3018466bbffbaa4427842a451f8

As a next step we could try making the (query.Match, {'query': search_query, 'boost': 3, 'standard', 'operator': 'and'}) part a must instead of linking it all together with a should. But that's just a random thought.

…ritize exact matches more. References many "component: search" issues. What I tested with a database of all public add-ons: Example searches: tab center redux - should find "Tab Center Redux" while "Tab Mix Plus" is probably second and "Redux DevTools" 4th or so Open Image in New Tab -> should find "Open Image in New Tab" while "Open Bookmarks in New Tab" should be 2nd or 3rd CoinHive -> Finds "Coinhive Blocker", "CoinBlock" (prefix search) and "NoMiners" (description) Privacy -> Finds "Privacy Badger", "Privacy Pass", "Privacy Settings", "Google Privacy" (probably 4th or so) and "Blur" (summary + description + many users). Scores "Ghostery" on the first page but ranks it in the middle firebu -> Finds "Firebug", "Firebug Autocompleter", "Firefinder for Firebug" fireb -> Scores "Fire Drag" first, puts "Firebug" approximately 3rd or so Menu Wizzard -> Finds "Menu Wizard" (fuzzy, typo) first, then "Add-ons Manager Context Menu" apparently because it matches good in the title and has many users Frame Demolition -> Finds "Frame Demolition" Demolition -> Finds only "Frame Demolition", same for "Demolation" (typo) reStyle -> Finds "reStyle" and scores a few add-ons that match on "restore" next since the term is similar MegaUpload DownloadHelper -> finds "MegaUpload DownloadHelper" first, scores "Video DownloadHelper" and "RadpidShare DownloadHelper" next. Doesn't find "Popup Blocker" anymore as currently happening on -prod MegaUpload -> only finds "MegaUpload DownloadHelper" and nothing else No Flash -> Scores "No Flash" first, then depending on users "Download Flash and Video", "YouTube Flash Video Player" and "YouTube Flash Player" (not necessarily in that order) Disable Hello, Pocket & Reader+ -> finds "Disable Hello, Pocket & Reader+" first (yeay!), then scores "Reader", "Disable WebRTC" and "In My Pocket" next similarly to what's happening on -prod currently Not working yet: privacybadger -> "Privacy Badger" -> will probably need some kind of ngram filtering and analyzing (#591) eyes -> 'decentraleyes' -> Not sure this should actually work, will probably need some more analyzing too (#591) Not sure if it's specifically only because of these changes but #3248 is fixed. This potentially fixes #7244, #6891, #6837, #6417, mozilla/addons#359. Not sure if this fixes #mozilla/addons#567 but the results look much more promising and the amount of results doesn't explode here. I only have 2.8k add-ons for testing though so I'm not too sure. And might be relevant to #6137. This is a big step towards #2661, I doubt we can call this fixed though.

…arch-operator

EnTeQuAk · 2018-01-16T06:37:12Z

Note: I'm working on figuring out that legacy api test failure, looks kinda weird since the actual problem is that for some reason GrApple Yummy isn't found when searching for grapple. Might be some analyzing or something weird since it works in master. Still investigating.

eviljeff · 2018-01-16T12:19:06Z

What's the argument in favour of merging this without unittests? It's not a large changeset currently - is one of the issues a particularly urgent fix?

diox · 2018-01-16T13:12:58Z

I'm with @eviljeff : I don't think it's particularly urgent, so we should wait for tests (well they are broken currently so we have to wait anyway). Just a simple search view test proving that all the words are now used vs only a few should be enough.

PS: why removing the slug? The rest of the changes seem to make sense at first glance.

EnTeQuAk · 2018-01-16T21:41:22Z

Alrighty, I'll spare some more time then and work on proper tests. I'll try to update the PR tomorrow (Wed) and see how that goes.

EnTeQuAk · 2018-01-16T21:43:58Z

PS: why removing the slug? The rest of the changes seem to make sense at first glance.

There were more than a handful scenarios where removing the slug immensely improved the overall scoring. As we already discussed some time ago I still believe that searching on the slug might not be the best idea since it's more or less one word for ElasticSearch right now with some fancy punctuation in there. Sometimes it's even truncated. It sure messes up fuzzy searches on which we rely quite a bit.

I can try to write some more tests to prove that though, this is mainly from the search-scenarios above and a few other tests.

diox · 2018-01-16T21:55:28Z

There were more than a handful scenarios where removing the slug immensely improved the overall scoring. As we already discussed some time ago I still believe that searching on the slug might not be the best idea since it's more or less one word for ElasticSearch right now with some fancy punctuation in there. Sometimes it's even truncated. It sure messes up fuzzy searches on which we rely quite a bit.

I can try to write some more tests to prove that though, this is mainly from the search-scenarios above and a few other tests.

That explanation is enough for me. Maybe add a comment somewhere so that we're not tempted to add it back?

EnTeQuAk · 2018-01-16T21:58:39Z

I'll do that

On Tue, Jan 16, 2018, at 10:55 PM, Mathieu Pillard wrote: > There were more than a handful scenarios where removing the slug > immensely improved the overall scoring. As we already discussed > some time ago I still believe that searching on the slug might not > be the best idea since it's more or less one word for > ElasticSearch right now with some fancy punctuation in there. > Sometimes it's even truncated. It sure messes up fuzzy searches on > which we rely quite a bit.>> I can try to write some more tests to prove that though, this is > mainly from the search-scenarios above and a few other tests.> That explanation is enough for me. Maybe add a comment somewhere so that we're not tempted to add it back?> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub[1], or mute the thread[2].>

Links: 1. #7303 (comment) 2. https://github.com/notifications/unsubscribe-auth/AAIfGcOIUUkORSGEJRgXmDJjYYAPjMmpks5tLRrRgaJpZM4ReyWe

…arch-operator

…y one shard and one replica

EnTeQuAk · 2018-01-23T10:01:19Z

Alrighty, I added tests and re-configured our test settings to only have one replica and one shard to make the scoring more reliable.

Alternatively we might want to think about using https://www.elastic.co/guide/en/elasticsearch/reference/master/search-request-search-type.html#dfs-query-then-fetch which doesn't sound too bad imho, might hurt performance though.

…arch-operator

… allow us to test this later too via a waffle flag.

…sts to be able to handle the waffle-flag

…operly. More serializer fixes

EnTeQuAk · 2018-01-26T06:13:42Z

FYI: There are a few TODOs in some tests still, I'd like to get them work more incrementally and focus on the big picture first maybe, some are obscure but should work eventually.

I'll also squash this before merging, I usually don't like squashing while in a pull request

diox · 2018-01-29T14:30:04Z

+        view.request = self.request
+        qset = view.get_queryset()
+
+        return qset.filter('term', id=self.addon.pk).execute()[0]


nit : we use qs pretty much everywhere. Applies to a lot of other parts below.

diox

👍

diox · 2018-01-29T14:33:07Z

+            self.request,
+            'search-use-dfs-query-then-fetch')
+        if dfs_query_then_fetch:
+            qset = qset.params(search_type='dfs_query_then_fetch')


I understand why you're doing this after reading ES doc, but it deserves a comment.

diox · 2018-01-29T14:36:12Z

+            name='Amazon 1-Click Lock', type=amo.ADDON_EXTENSION,
+            average_daily_users=50, weekly_downloads=0),
+
+        cls.refresh()


An improvement for later: prevent the individual index calls while you're adding all these add-ons, then call reindex instead of refresh.

Good idea, I'll add it to my list of future improvements and will open an issue about it

diox · 2018-01-29T14:37:17Z

+
+    def test_scenario_megaupload(self):
+        self._check_scenario('MegaUpload', (
+            # I have litterally NO idea :-/


add TODO :)

diox · 2018-01-29T14:39:01Z

+
+    def test_scenario_no_flash(self):
+        # TODO: Doesn't put "No Flash" on first line, does the "No"
+        # do something special here?


no is a stopword FWIW

That's true but the "no idea" part comes where we have a raw term match and IMHO (correct me if I'm wrong) that raw term match should not make use of any analyzing, stopword filtering etc

EnTeQuAk added 2 commits January 15, 2018 19:05

Merge branch 'master' of github.com:mozilla/addons-server into and-se…

b907e46

…arch-operator

EnTeQuAk requested a review from diox January 15, 2018 18:23

Fix name tests

f0088c7

EnTeQuAk added 3 commits January 18, 2018 10:19

Merge branch 'master' of github.com:mozilla/addons-server into and-se…

64ffbf3

…arch-operator

Speed up ES tests, make scoring results more predictable by using onl…

27f0d75

…y one shard and one replica

Add tests

1274830

EnTeQuAk added 10 commits January 23, 2018 11:02

Only test on new apiv3 based search.

f0d83d1

Fixup 'get_results'

455a205

Remove debug print

be411eb

Isort imports

88a2fd9

Fix test settings, I'm blind.

23ae5af

Merge branch 'master' of github.com:mozilla/addons-server into and-se…

8b3591e

…arch-operator

Adapt number of shards to what we define in settings_test

c37c8d2

Test search for grapple

78ef9f4

Remove property filtering in legacy api search, fix tests, fix flake8

1953673

Add comment explaining shard config

f6857e7

EnTeQuAk mentioned this pull request Jan 24, 2018

Different results after refresh in search API response for relevance mozilla/addons#3531

Closed

EnTeQuAk added 4 commits January 24, 2018 17:25

Fix tests again, add default platform, fix total counts again.

ca80307

Try to do some fixture cleanup, let's see...

8cb0bdb

More test refactoring, make use of dfs-query-then-fetch during tests,…

902d8da

… allow us to test this later too via a waffle flag.

Don't use waffle flag for legacy search, only for apiv3 searches.

a7be496

EnTeQuAk added 5 commits January 25, 2018 13:17

Fix unused import, fix usage of 'params'

2d30d8c

Use query-then-fetch in a regular addons-view tool, fix a few more te…

7bcdf00

…sts to be able to handle the waffle-flag

Create the dfs query then fetch flag only in ESTestCase, delete it pr…

6497357

…operly. More serializer fixes

precache the waffle flag for autocomplete tests too

ed5aa40

Minor cleanups

aa27d67

diox reviewed Jan 29, 2018

View reviewed changes

diox approved these changes Jan 29, 2018

View reviewed changes

EnTeQuAk added 3 commits January 29, 2018 18:52

Add docs, fix code style

6c405c6

Add a todo

769b209

Fix codestyle

106869d

EnTeQuAk merged commit d52015d into master Jan 30, 2018

EnTeQuAk deleted the and-search-operator branch January 30, 2018 05:26

EnTeQuAk mentioned this pull request Jan 30, 2018

Several add-ons are not displayed as suggestions in search list mozilla/addons#4638

Closed

Uh oh!

Conversation

EnTeQuAk commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EnTeQuAk commented Jan 16, 2018

Uh oh!

eviljeff commented Jan 16, 2018

Uh oh!

diox commented Jan 16, 2018

Uh oh!

EnTeQuAk commented Jan 16, 2018

Uh oh!

EnTeQuAk commented Jan 16, 2018

Uh oh!

diox commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EnTeQuAk commented Jan 16, 2018 via email

Uh oh!

EnTeQuAk commented Jan 23, 2018

Uh oh!

EnTeQuAk commented Jan 26, 2018

Uh oh!

diox Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diox left a comment

Choose a reason for hiding this comment

Uh oh!

diox Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

diox Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

EnTeQuAk Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

diox Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

diox Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

EnTeQuAk Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EnTeQuAk commented Jan 15, 2018 •

edited

Loading

diox commented Jan 16, 2018 •

edited

Loading

diox Jan 29, 2018 •

edited

Loading