Merge lp:~lifeless/launchpad/bug-717394 into lp:launchpad

[Robert Collins]

[r=stub][bug=516050,717394] Use an aggregate function rather than python iteration to summaries milestone bug counts.

Another step on bug search performance, this branch uses the relatively new counting API for bugs to get aggregates for milestones rather than querying once per milestone. This should save nearly a second on bug searches in the Ubuntu context and help make bug searches in smaller contexts just that little bit snappier.

[Robert Collins (lifeless)]

I should note that as I was just refactoring I've again not added explicit tests; all the behaviour is tested, with one caveat - I improved the strictness of the aggregate function, and there isn't a test that it was doing the wrong thing before - but it should be pretty obvious to anyone coming along later that it was incorrect before.

[Stuart Bishop (stub)]

The following two lines leave me scratching my head looking for side effects:

reduce(lambda _, series:milestones.extend(series.milestones),
self.series_list, [])

Better written as:

for series in self.series_list:
    milestones.extend(series.milestones)

Does the bug search api allow you to group by BugTask.milestone instead of BugTask.milestoneID? I think the former would be clearer.

It would be nice if countBugs() in lp/bugs/model/bugtask.py retained its Storm syntax, but I don't know how to spell the COUNT(DISTINCT...) construct either.

I notice that lp/bugs/interfaces/bugtask.py is growing a pretty complex class. This seems wrong - IBugTaskSearchParams can be declared in this file, but the implementation should exist elsewhere. Your 'Yay circular deps' is just a symptom. Worth opening a Bug and adding a referencing XXX: to this class for the next time someone extends or refactors this?

review: Approve

[Robert Collins (lifeless)]

The reduce goes in the next branch or two anyhow.

BugTask.milestone is a storm reference column, they compile poorly - so no.

I tried a storm version for the distinct and it compiled to select count((distinct BugTask.bug)) which is not valid sql.

The params is in the interface class so it can be imported around the import fascist which model code can't AIUI - so I'm not sure there is an option to move it substantially. I don't think XXX 'this is a good obvious idea' bugs are worth their time: they generally just add up.

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2011-02-17 09:14:19 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2011-02-21 23:28:14 +0000
@@ -24,7 +24,10 @@
 import cgi
 from cStringIO import StringIO
 from datetime import datetime
-from operator import itemgetter
+from operator import (
+    attrgetter,
+    itemgetter,
+    )
 import urllib
 
 from lazr.restful.interface import copy_field
@@ -1188,9 +1191,11 @@
         """
         series_buglistings = []
         bug_task_set = getUtility(IBugTaskSet)
-        series = any(*self.series_list)
+        series_list = self.series_list
+        if not series_list:
+            return series_buglistings
         open_bugs = bug_task_set.open_bugtask_search
-        open_bugs.setTarget(series)
+        open_bugs.setTarget(any(*series_list))
         # This would be better as delegation not a case statement.
         if IDistribution(self.context, None):
             backlink = BugTask.distroseriesID
@@ -1203,7 +1208,7 @@
         else:
             raise AssertionError("illegal context %r" % self.context)
         counts = bug_task_set.countBugs(open_bugs, (backlink,))
-        for series in self.series_list:
+        for series in series_list:
             series_bug_count = counts.get((series.id,), 0)
             if series_bug_count > 0:
                 series_buglistings.append(
@@ -1218,16 +1223,24 @@
     def milestone_buglistings(self):
         """Return a buglisting for each milestone."""
         milestone_buglistings = []
-        for series in self.series_list:
-            for milestone in series.milestones:
-                milestone_bug_count = milestone.open_bugtasks.count()
-                if milestone_bug_count > 0:
-                    milestone_buglistings.append(
-                        dict(
-                            title=milestone.name,
-                            url=canonical_url(milestone),
-                            count=milestone_bug_count,
-                            ))
+        bug_task_set = getUtility(IBugTaskSet)
+        milestones = []
+        reduce(lambda _, series:milestones.extend(series.milestones),
+            self.series_list, [])
+        if not milestones:
+            return milestone_buglistings
+        open_bugs = bug_task_set.open_bugtask_search
+        open_bugs.setTarget(any(*milestones))
+        counts = bug_task_set.countBugs(open_bugs, (BugTask.milestoneID,))
+        for milestone in milestones:
+            milestone_bug_count = counts.get((milestone.id,), 0)
+            if milestone_bug_count > 0:
+                milestone_buglistings.append(
+                    dict(
+                        title=milestone.name,
+                        url=canonical_url(milestone),
+                        count=milestone_bug_count,
+                        ))
         return milestone_buglistings
 
 

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-02-18 02:58:59 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-02-21 23:28:14 +0000
@@ -3078,12 +3078,11 @@
 
         # XXX flacoste 2008/04/24 This should be moved to a
         # BugTaskSearchParams.setTarget().
-        if IDistroSeries.providedBy(self.context):
-            search_params.setDistroSeries(self.context)
+        if (IDistroSeries.providedBy(self.context) or
+            IProductSeries.providedBy(self.context)):
+            search_params.setTarget(self.context)
         elif IDistribution.providedBy(self.context):
             search_params.setDistribution(self.context)
-        elif IProductSeries.providedBy(self.context):
-            search_params.setProductSeries(self.context)
         elif IProduct.providedBy(self.context):
             search_params.setProduct(self.context)
         elif IProjectGroup.providedBy(self.context):

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2011-02-17 09:14:19 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2011-02-21 23:28:14 +0000
@@ -1281,6 +1281,7 @@
         # Yay circular deps.
         from lp.registry.interfaces.distroseries import IDistroSeries
         from lp.registry.interfaces.productseries import IProductSeries
+        from lp.registry.interfaces.milestone import IMilestone
         if isinstance(target, (any, all)):
             assert len(target.query_values), \
                 'cannot determine target with no targets'
@@ -1291,6 +1292,8 @@
             self.setDistroSeries(target)
         elif IProductSeries.providedBy(instance):
             self.setProductSeries(target)
+        elif IMilestone.providedBy(instance):
+            self.milestone = target
         else:
             raise AssertionError("unknown target type %r" % target)
 

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-02-17 04:40:55 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-02-21 23:28:14 +0000
@@ -2502,7 +2502,7 @@
     def countBugs(self, params, group_on):
         """See `IBugTaskSet`."""
         resultset = self._search(
-            group_on + (Count(BugTask.bugID),), [], None, params).result_set
+            group_on + (SQL("COUNT(Distinct BugTask.bug)"),), [], None, params).result_set
         # We group on the related field:
         resultset.group_by(*group_on)
         resultset.order_by()