Merge lp:~wallyworld/launchpad/remove-personpicker-fti-query into lp:launchpad

[Ian Booth]

[r=sinzui][bug=618356] Remove fti query from the person picker searches.

The person picker search can time out due to there being no ftq index.

== Implementation ==

As part of the disclosure work, the picker is being revamped. It was agreed that there is little benefit in doing a fti match and so it is reasonable to remove that for now.

== Tests ==

Run existing person picker tests, eg test_person_picker_widget

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/vocabularies.py

./lib/lp/registry/vocabularies.py
     584: E261 at least two spaces before inline comment

[Curtis Hovey (sinzui)]

Thank you for making the search faster and more accurate.

review: Approve (code)

[Ian Booth (wallyworld)]

Rejected. Subsequent testing shows that matching on displayname requires fti. Adding fti index to prod instead.

[Curtis Hovey (sinzui)]

Why cannot we change the code to check displayname. I think the masses of bad matches in search is caused by words in the summary and description.

[Curtis Hovey (sinzui)]

person.fti contains name and displayname. The query singles out name (Launchpad Id) to give it the highest rank. Is faster and clearer to just check the displayname column? I think fti provides stem and case normalisation, which may make searching for teams easier.

[Ian Booth (wallyworld)]

I asked about doing this but was told that using fti is preferred since
a search for "Fred Bloggs" will still match "Fred J Bloggs" and hence a
simple displayname comparison is not sufficient. It was also mentioned
that since the fti only includes name and displayname, there should not
be crap results based on other "bad" matches.

On 25/05/11 23:57, Curtis Hovey wrote:
> Why cannot we change the code to check displayname. I think the masses of bad matches in search is caused by words in the summary and description.

[Robert Collins (lifeless)]

On Thu, May 26, 2011 at 10:37 AM, Ian Booth <email address hidden> wrote:
> I asked about doing this but was told that using fti is preferred since
> a search for "Fred Bloggs" will still match "Fred J Bloggs" and hence a
> simple displayname comparison is not sufficient. It was also mentioned
> that since the fti only includes name and displayname, there should not
> be crap results based on other "bad" matches.

And display name isn't indexed. So we would need to add an index on
that instead of the one we're meant to have on person_fti.

-Rob

Unmerged revisions

13109. By Ian Booth on 2011-05-24

Lint

13108. By Ian Booth on 2011-05-24

Remove fti matching for picker seaches

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2011-05-14 15:02:13 +0000
+++ lib/lp/registry/vocabularies.py	2011-05-24 12:53:42 +0000
@@ -50,7 +50,6 @@
     'SourcePackageNameVocabulary',
     'UserTeamsParticipationPlusSelfVocabulary',
     'UserTeamsParticipationVocabulary',
-    'ValidPersonOrClosedTeamVocabulary',
     'ValidPersonOrTeamVocabulary',
     'ValidPersonVocabulary',
     'ValidTeamMemberVocabulary',
@@ -543,11 +542,11 @@
                 ]
 
             # Create an inner query that will match public persons and teams
-            # that have the search text in the fti, at the start of the email
-            # address, or as their full IRC nickname.
+            # that have the search text at the start of the email address,
+            # or as their full IRC nickname.
             # Since we may be eliminating results with the limit to improve
             # performance, we sort by the rank, so that we will always get
-            # the best results. The fti rank will be between 0 and 1.
+            # the best results.
             # Note we use lower() instead of the non-standard ILIKE because
             # ILIKE doesn't hit the indexes.
             # The '%%' is necessary because storm variable substitution
@@ -558,10 +557,6 @@
                     FROM Person
                     WHERE name = ?
                     UNION ALL
-                    SELECT Person.id, rank(fti, ftq(?))
-                    FROM Person
-                    WHERE Person.fti @@ ftq(?)
-                    UNION ALL
                     SELECT Person.id, 10 AS rank
                     FROM Person, IrcId
                     WHERE IrcId.person = Person.id
@@ -575,12 +570,11 @@
                     ) AS public_subquery
                 ORDER BY rank DESC
                 LIMIT ?
-                """, (text, text, text, text, text,
+                """, (text, text, text,
                       EmailAddressStatus.VALIDATED.value,
                       EmailAddressStatus.PREFERRED.value,
                       self.LIMIT))
 
-
             public_result = self.store.using(*public_tables).find(
                 Person,
                 And(
@@ -606,13 +600,9 @@
             # but we're electing to ignore them here.
             private_result = self.store.using(*private_tables).find(
                 Person,
-                And(
-                    SQL('Person.fti @@ ftq(?)', [text]),
-                    private_query,
-                    )
+                private_query,
                 )
 
-            private_result.order_by(SQL('rank(fti, ftq(?)) DESC', [text]))
             private_result.config(limit=self.LIMIT)
 
             combined_result = public_result.union(private_result)
@@ -676,7 +666,7 @@
     allow_null_search = True
 
     def _doSearch(self, text=""):
-        """Return the teams whose fti, IRC, or email address match :text:"""
+        """Return the teams whose IRC, or email address match :text:"""
 
         private_query, private_tables = self._privateTeamQueryAndTables()
         base_query = And(
@@ -695,8 +685,6 @@
                         self.extra_clause)
             result = self.store.using(*tables).find(Person, query)
         else:
-            name_match_query = SQL("Person.fti @@ ftq(%s)" % quote(text))
-
             email_storm_query = self.store.find(
                 EmailAddress.personID,
                 EmailAddress.email.lower().startswith(text))
@@ -709,9 +697,7 @@
             result = self.store.using(*tables).find(
                 Person,
                 And(base_query,
-                    self.extra_clause,
-                    Or(name_match_query,
-                       EmailAddress.person != None)))
+                    self.extra_clause))
 
         # XXX: BradCrittenden 2009-05-07 bug=373228: A bug in Storm prevents
         # setting the 'distinct' and 'limit' options in a single call to
@@ -1433,7 +1419,7 @@
 
     def __iter__(self):
         series = self._table.select(
-            DistroSeries.q.distributionID==Distribution.q.id,
+            DistroSeries.q.distributionID == Distribution.q.id,
             orderBy=self._orderBy, clauseTables=self._clauseTables)
         for series in sorted(series, key=attrgetter('sortkey')):
             yield self.toTerm(series)