Merge lp:~reedobrien/charmworld/ngrams into lp:charmworld

[Reed O'Brien]

adds ngrams as multifield index to name. skips destructive test -- see Bug #1317567

Allow finding sugarcrm by searching for crm, or postgresql, cf-mysql, and mysql by searching for sql.

Changes:
 - Adds a multifield index for name with exact field and 3-20 ngrams analyzer.
 - Fixes and adds tests for new index.
 - Updates api_search with autocomplete=True to use the ngrams field.

Secondary changes:
 - removes one test which deleted all ES data when run
 - updates ppa:juju/pkgs -> ppa:juju/stable
 - removes svn from sysdeps

Caveats:
I think that this will analyze the query which may mean that searching with no minimum matching set may be greedier than desired. AFAIU, this accomplishes what was desired.
Relevance is subjective so I didn't try tuning for relevance. The results I get look good to me at a glance:)

Also, the minimum ngram is 3 so there *isn't likely any good reason for the gui to send requests at 1 or 2 chars in the search box*.

Once built ingest charms as follows for testing:

# ttyn
$ make run

# ttyn+1
$ bin/es-update
$ bin/ingest-queued --prefix=~cabs-team/ ## gets sugarcrm
$ bin/ingest-queued --prefix=~charmers/charms/ ## gets a bunch of charms
$ bin/ingest-queued --prefix=~bac ## gets a bundle

[j.c.sackett (jcsackett)]

Code looks good, I'm qaing now.

Can you file a bug about the test you had to comment out?

[j.c.sackett (jcsackett)]

I cannot QA this successfully on my machine, but others have been able to.

review: Approve

[Reed O'Brien (reedobrien)]

> Code looks good, I'm qaing now.
>
> Can you file a bug about the test you had to comment out?

Bug #1317567 - http://bit.ly/1lsXCfv

[Richard Harding (rharding)]

QA is mixed. The results seem no worse than current so let's try this out. I did try out search for 'couch' which I would have expected to ngram match for 'couchdb' but it did not. I also tried 'build' which found 'buildbot'. I'm not sure if we hit on other matching criteria though.

As for review, let's take advantage of the ability to skip a test. I think nose will respect @unittest.skip("") rather than comment out a large block of code.

review: Needs Fixing

[Richard Harding (rharding)]

review: Approve

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
No commit message was specified in the merge proposal. Hit 'Add commit message' on the merge proposal web page or follow the link below. You can approve the merge proposal yourself to rerun.
https://code.launchpad.net/~reedobrien/charmworld/ngrams/+merge/218821/+edit-commit-message

review: Needs Fixing (continuous-integration)

[Richard Harding (rharding)]

reapprove

review: Approve

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

=== modified file 'Makefile'
--- Makefile	2014-02-10 21:27:57 +0000
+++ Makefile	2014-05-14 15:51:33 +0000
@@ -12,7 +12,7 @@
 PSERVE := bin/pserve
 SYSTEM_DEPS := \
 	build-essential bzr libyaml-dev python-dev python-virtualenv\
-	subversion nodejs npm
+	nodejs npm
 
 
 # Default ini file to use for all scripted commands. Can be overridden by
@@ -64,7 +64,7 @@
 sysdeps:
 	sudo apt-get update
 	sudo apt-get install --yes python-software-properties
-	sudo add-apt-repository -y ppa:juju/pkgs
+	sudo add-apt-repository -y ppa:juju/stable
 	sudo add-apt-repository -y ppa:ce-orange-squad/experimental
 	sudo apt-get update
 	sudo apt-get install -y $(SYSTEM_DEPS) mongodb-server \
 
=== modified file 'charmworld/search.py'
--- charmworld/search.py	2014-04-14 18:09:22 +0000
+++ charmworld/search.py	2014-05-14 15:51:33 +0000
@@ -32,6 +32,7 @@
 ]
 charm_free_text_fields = {
     'name': 10,
+    'ngrams': None,
     'summary': 5,
     'description': 3,
     'config.options.description': None,
@@ -45,6 +46,7 @@
 ]
 bundle_free_text_fields = {
     'name': 10,
+    'ngrams': None,
     'basket_name': 5,
     'description': 3,
     'title': None,
@@ -160,6 +162,62 @@
                 "index": {
                     "number_of_shards": settings['es_shards'],
                     "number_of_replicas": settings['es_replicas'],
+                    "analysis": {
+                        "filter": {
+                            "n3_20grams_filter": {
+                                "type": "ngram",
+                                "min_gram": 3,
+                                "max_gram": 20
+                            }
+                        },
+                        "analyzer": {
+                            "n3_20grams": {
+                                "type": "custom",
+                                "tokenizer": "standard",
+                                "filter": [
+                                    "lowercase",
+                                    "n3_20grams_filter"
+                                ]
+                            }
+                        }
+                    },
+                    "mappings": {
+                        "charm": {
+                            "properties": {
+                                "name": {
+                                    "type": "multi_field",
+                                    "fields": {
+                                        "ngrams": {
+                                            "type": "string",
+                                            "analyzer": "n3_20grams"
+                                        },
+                                        "name": {
+                                            "type": "string",
+                                            "index": "not_analyzed"
+                                        }
+                                    }
+                                }
+                            }
+
+                        },
+                        "bundle": {
+                            "properties": {
+                                "name": {
+                                    "type": "multi_field",
+                                    "fields": {
+                                        "ngrams": {
+                                            "type": "string",
+                                            "analyzer": "n3_20grams"
+                                        },
+                                        "name": {
+                                            "type": "string",
+                                            "index": "not_analyzed"
+                                        }
+                                    }
+                                }
+                            }
+                        }
+                    }
                 }
             })
         # The ES server may need some time to actually create the index
@@ -221,7 +279,6 @@
         """
         charm_exact_index = [
             'categories',
-            'name',
             'owner',
             'i_provides',
             'i_requires',
@@ -229,16 +286,37 @@
             'store_url',
             '_id',
         ]
+
         inner_charm_properties = dict(
             (name, {'type': 'string', 'index': 'not_analyzed'})
             for name in charm_exact_index)
 
-        CHARM_TEXT_INDEX = (
+        charm_n3_20gram_index = [
+            'name'
+        ]
+
+        charm_text_index = (
             'summary',
             'description',
         )
+
         inner_charm_properties.update(dict(
-            (name, {'type': 'string'}) for name in CHARM_TEXT_INDEX))
+            (name, {'type': 'string'}) for name in charm_text_index))
+
+        inner_charm_properties.update(
+            dict((name, {
+                "type": "multi_field",
+                "fields": {
+                    "ngrams": {
+                        "type": "string",
+                        "analyzer": "n3_20grams"
+                    },
+                    "name": {
+                        "type": "string",
+                        "index": "not_analyzed"
+                    }
+                }
+            }) for name in charm_n3_20gram_index))
 
         CHARM_INTEGER_INDEX = (
             'downloads',
@@ -290,19 +368,37 @@
         }
 
         bundle_exact_index = [
-            'name',
             'owner',
             'series',
             'title',
         ]
 
+        bundle_n3_20gram_index = [
+            "name",
+        ]
+
         bundle_properties = {
             'data': {
                 'properties':  # XXX The linter won't let me indent this well.
                 dict((name, {'type': 'string', 'index': 'not_analyzed'})
-                for name in bundle_exact_index)
+                     for name in bundle_exact_index)
             }}
 
+        bundle_properties['data']['properties'].update(
+            dict((name, {
+                "type": "multi_field",
+                "fields": {
+                    "ngrams": {
+                        "type": "string",
+                        "analyzer": "n3_20grams"
+                    },
+                    "name": {
+                        "type": "string",
+                        "index": "not_analyzed"
+                    }
+                }
+            }) for name in bundle_n3_20gram_index))
+
         with translate_error():
             try:
                 for (name, properties, dynamic) in (
@@ -407,7 +503,7 @@
                 text = text[:-1]
 
             if autocomplete:
-                return {'prefix': {'data.name': text}}
+                return {'match': {'ngrams': text}}
 
         charm_fields = [field + ('' if boost is None else '^%d' % boost)
                         for field, boost in charm_free_text_fields.items()]
@@ -425,8 +521,8 @@
                 fields.extend(other_fields)
                 return {
                     'query_string': {
-                    'query': text,
-                    'fields': fields,
+                        'query': text,
+                        'fields': fields,
                     }}
 
             charm_dsl = {'filtered': {
 
=== modified file 'charmworld/tests/test_search.py'
--- charmworld/tests/test_search.py	2014-04-02 14:12:22 +0000
+++ charmworld/tests/test_search.py	2014-05-14 15:51:33 +0000
@@ -8,6 +8,8 @@
 from textwrap import dedent
 from time import sleep
 
+import unittest
+
 from pyelasticsearch import ElasticSearch
 from pyelasticsearch.exceptions import (
     ElasticHttpError,
@@ -154,6 +156,46 @@
         # it's present.
         self.assertItemsEqual(['data', 'weight'], results[0].keys())
 
+    def test_ngrams_found(self):
+        # This tests that the underlying client search works
+        # not that the charmworld search API does _client != client.
+        client = self.index_client
+        test_value = "blarglefoafblatherinfinatum"
+        Charm(self.makeCharm(name=test_value))
+        query = {"query": {"match": {"ngrams": "bla"}}}
+        start, mingram = 0, 3
+        while start < len(test_value) - 3:
+            while mingram < len(test_value):
+                query['query']['match']['ngrams'] = test_value[start:mingram]
+                results = client._client.search(query, index=client.index_name)
+                self.assertEquals(results['hits']['total'], 1)
+                mingram += 1
+            start += 1
+
+    def test_ngrams_too_short(self):
+        client = self.index_client
+        test_value = "blarg"
+        Charm(self.makeCharm(name=test_value))
+        query = {"query": {"match": {"ngrams": "bl"}}}
+        start, mingram = 0, 2
+        while start < len(test_value) - 3:
+            while mingram < len(test_value):
+                query['query']['match']['ngrams'] = test_value[start:mingram]
+                results = client._client.search(query, index=client.index_name)
+                self.assertEquals(results['hits']['total'], 0)
+                mingram += 1
+                start += 1
+
+    def test_ngrams_no_matching_ngrams(self):
+        client = self.index_client
+        test_value = "blarglefoafblatherinfinatum"
+        foo_charm = Charm(self.makeCharm(name=test_value))
+        foo_charm = foo_charm  # shut it pyflakes
+        query = {"query": {"match":
+                {"ngrams": "blvlsdkjfa;lsdkghoifdhvsoidufhvsdececewv"}}}
+        results = client._client.search(query, index=client.index_name)
+        self.assertEquals(results['hits']['total'], 0)
+
     def test_search_bundle(self):
         bundle = Bundle(self.makeBundle())
         client = self.index_client
@@ -516,14 +558,26 @@
         ids = [r['data']['_id'] for r in result]
         self.assertEqual([matching_charm['_id']], ids)
 
-    def test_autocomplete_matches_on_prefix(self):
+    def test_autocomplete_matches_on_ngram(self):
         charm1 = self.makeCharm(name='foo')
         charm2 = self.makeCharm(name='foobar')
+        charm3 = self.makeCharm(name='barfoo')
+        result = self.index_client.api_search(
+            'foo', autocomplete=True, min_score=0)
+        ids = [r['data']['_id'] for r in result]
+        self.assertItemsEqual([
+            charm1['_id'],
+            charm2['_id'],
+            charm3['_id']],
+            ids)
+
+    def test_autocomplete_misses_ngram(self):
+        self.makeCharm(name='foo')
+        self.makeCharm(name='foobar')
         self.makeCharm(name='barfoo')
         result = self.index_client.api_search(
-            'foo', autocomplete=True, min_score=0)
-        ids = [r['data']['_id'] for r in result]
-        self.assertItemsEqual([charm1['_id'], charm2['_id']], ids)
+            'bla', autocomplete=True, min_score=0)
+        self.assertEquals(result, [])
 
     def test_special_characters_return_no_results(self):
         self.makeCharm(name='foo')
@@ -1104,7 +1158,7 @@
 
 def put_incompatible_mapping(index_client):
     put_mapping(index_client, {
-        'name': {'type': 'string', 'index': 'analyzed'},
+        '_id': {'type': 'string', 'index': 'analyzed'},
     })
 
 
@@ -1131,8 +1185,12 @@
         put_incompatible_mapping(index_client)
         update(index_client)
         mapping = index_client.get_mapping()[CHARM]['properties']['data']
-        self.assertEqual('not_analyzed',
-                         mapping['properties']['name']['index'])
+        self.assertEqual(
+            'not_analyzed',
+            mapping['properties']['name']['fields']['name']['index'])
+        self.assertEqual(
+            'n3_20grams',
+            mapping['properties']['name']['fields']['ngrams']['analyzer'])
 
     def test_update_no_index(self):
         index_client = ElasticSearchClient.from_settings(
@@ -1143,8 +1201,12 @@
         self.assertEqual([actual_client.index_name],
                          index_client.get_aliased())
         mapping = index_client.get_mapping()[CHARM]['properties']['data']
-        self.assertEqual('not_analyzed',
-                         mapping['properties']['name']['index'])
+        self.assertEqual(
+            'not_analyzed',
+            mapping['properties']['name']['fields']['name']['index'])
+        self.assertEqual(
+            'n3_20grams',
+            mapping['properties']['name']['fields']['ngrams']['analyzer'])
 
     def update_to_static_mapping(self, force_reindex):
         index_client = self.use_index_client(put_mapping=False)
@@ -1218,9 +1280,14 @@
         new_aliased = reindex(alias)
         self.assertEqual([new_aliased.index_name], alias.get_aliased())
         mapping = alias.get_mapping()[CHARM]['properties']['data']
-        self.assertEqual('not_analyzed',
-                         mapping['properties']['name']['index'])
+        self.assertEqual(
+            'n3_20grams',
+            mapping['properties']['name']['fields']['ngrams']['analyzer'])
+        self.assertEqual(
+            'not_analyzed',
+            mapping['properties']['name']['fields']['name']['index'])
 
+    @unittest.skip("XXX: Bug #1317567 - http://bit.ly/1lsXCfv")
     def test_reindexed_no_client_charms(self):
         client = ElasticSearchClient.from_settings(get_ini())
         # This should not raise an exception, even though the index does not