launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/remove-personpicker-fti-query into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #618356 in Launchpad itself: "Person-picker takes a very long time consistently due to missing full text index"
  https://bugs.launchpad.net/launchpad/+bug/618356

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/remove-personpicker-fti-query/+merge/62120

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
-- 
https://code.launchpad.net/~wallyworld/launchpad/remove-personpicker-fti-query/+merge/62120
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/remove-personpicker-fti-query into lp:launchpad.

=== 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:25 +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)