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

[Robert Collins]

[r=stevenk][bug=736008] Another optimisation attempt for Product:+code-index branch merge proposal counting

Take 3 at fixing this bug: separate clauses that should apply to both source and target branches and those for just the source branch. Then use with to tell sql we're looking for common constraints on both source and target. This lowers the reported cost from 50K to 20K even for launchpad, on qastaging.

There is a new storm needed as part of this - I hadn't made select statements usable in WITH clauses before.

[Steve Kowalik (stevenk)]

After discussing my concerns with Robert on IRC, I'm happy with this branch.

review: Approve

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-03-28 20:54:50 +0000
+++ lib/lp/code/model/branchcollection.py	2011-03-28 22:49:01 +0000
@@ -19,7 +19,9 @@
     LeftJoin,
     Or,
     Select,
+    SQL,
     Union,
+    With,
     )
 from storm.info import ClassAlias
 from zope.component import getUtility
@@ -81,7 +83,8 @@
     implements(IBranchCollection)
 
     def __init__(self, store=None, branch_filter_expressions=None,
-                 tables=None, exclude_from_search=None):
+                 tables=None, exclude_from_search=None,
+                 asymmetric_filter_expressions=None, asymmetric_tables=None):
         """Construct a `GenericBranchCollection`.
 
         :param store: The store to look in for branches. If not specified,
@@ -93,14 +96,25 @@
         :param tables: A dict of Storm tables to the Join expression.  If an
             expression in branch_filter_expressions refers to a table, then
             that table *must* be in this list.
+        :param asymmetric_filter_expressions: As per branch_filter_expressions
+            but only applies to one side of reflexive joins.
+        :param asymmetric_tables: As per tables, for
+            asymmetric_filter_expressions.
         """
         self._store = store
         if branch_filter_expressions is None:
             branch_filter_expressions = []
-        self._branch_filter_expressions = branch_filter_expressions
+        self._branch_filter_expressions = list(branch_filter_expressions)
         if tables is None:
             tables = {}
         self._tables = tables
+        if asymmetric_filter_expressions is None:
+            asymmetric_filter_expressions = []
+        self._asymmetric_filter_expressions = list(
+            asymmetric_filter_expressions)
+        if asymmetric_tables is None:
+            asymmetric_tables = {}
+        self._asymmetric_tables = asymmetric_tables
         if exclude_from_search is None:
             exclude_from_search = []
         self._exclude_from_search = exclude_from_search
@@ -132,25 +146,42 @@
             return self._store
 
     def _filterBy(self, expressions, table=None, join=None,
-                  exclude_from_search=None):
-        """Return a subset of this collection, filtered by 'expressions'."""
+                  exclude_from_search=None, symmetric=True):
+        """Return a subset of this collection, filtered by 'expressions'.
+        
+        :param symmetric: If True this filter will apply to both sides of merge
+            proposal lookups and any other lookups that join Branch back onto
+            Branch.
+        """
         # NOTE: JonathanLange 2009-02-17: We might be able to avoid the need
         # for explicit 'tables' by harnessing Storm's table inference system.
         # See http://paste.ubuntu.com/118711/ for one way to do that.
-        tables = self._tables.copy()
         if table is not None:
             if join is None:
                 raise InvalidFilter("Cannot specify a table without a join.")
-            tables[table] = join
+        if expressions is None:
+            expressions = []
+        tables = self._tables.copy()
+        asymmetric_tables = self._asymmetric_tables.copy()
+        if symmetric:
+            if table is not None:
+                tables[table] = join
+            symmetric_expr = self._branch_filter_expressions + expressions
+            asymmetric_expr = list(self._asymmetric_filter_expressions)
+        else:
+            if table is not None:
+                asymmetric_tables[table] = join
+            symmetric_expr = list(self._branch_filter_expressions)
+            asymmetric_expr = self._asymmetric_filter_expressions + expressions
         if exclude_from_search is None:
             exclude_from_search = []
-        if expressions is None:
-            expressions = []
         return self.__class__(
             self.store,
-            self._branch_filter_expressions + expressions,
+            symmetric_expr,
             tables,
-            self._exclude_from_search + exclude_from_search)
+            self._exclude_from_search + exclude_from_search,
+            asymmetric_expr,
+            asymmetric_tables)
 
     def _getBranchIdQuery(self):
         """Return a Storm 'Select' for the branch IDs in this collection."""
@@ -160,16 +191,28 @@
 
     def _getBranchExpressions(self):
         """Return the where expressions for this collection."""
-        return (self._branch_filter_expressions
-            + self._getBranchVisibilityExpression())
+        return (self._branch_filter_expressions +
+            self._asymmetric_filter_expressions +
+            self._getBranchVisibilityExpression())
 
     def _getBranchVisibilityExpression(self, branch_class=None):
         """Return the where clauses for visibility."""
         return []
 
+    def _getCandidateBranchesWith(self):
+        """Return WITH clauses defining candidate branches.
+        
+        These are defined in terms of scope_branches which should be separately
+        calculated.
+        """
+        return [
+            With("candidate_branches", SQL("SELECT id from scope_branches"))]
+
     def getBranches(self, eager_load=False):
         """See `IBranchCollection`."""
-        tables = [Branch] + self._tables.values()
+        all_tables = set(
+            self._tables.values() + self._asymmetric_tables.values())
+        tables = [Branch] + list(all_tables)
         expressions = self._getBranchExpressions()
         resultset = self.store.using(*tables).find(Branch, *expressions)
         if not eager_load:
@@ -221,15 +264,25 @@
     def getMergeProposals(self, statuses=None, for_branches=None,
                           target_branch=None, merged_revnos=None):
         """See `IBranchCollection`."""
-        Target = ClassAlias(Branch, "target")
-        tables = [Branch] + self._tables.values() + [
-            Join(BranchMergeProposal, And(
-                Branch.id==BranchMergeProposal.source_branchID,
-                *self._branch_filter_expressions)),
-            Join(Target, Target.id==BranchMergeProposal.target_branchID)
-            ]
-        expressions = self._getBranchVisibilityExpression()
-        expressions.extend(self._getBranchVisibilityExpression(Target))
+        # teams = SQL("teams as (SELECT team from teamparticipation where person=%s)" % sqlvalues
+        scope_tables = [Branch] + self._tables.values()
+        scope_expressions = self._branch_filter_expressions
+        select = self.store.using(*scope_tables).find(
+            (Branch.id, Branch.private, Branch.ownerID), *scope_expressions)
+        branches_query = select._get_select()
+        with_expr = [With("scope_branches", branches_query)
+            ] + self._getCandidateBranchesWith()
+        expressions = [SQL("""
+            source_branch IN (SELECT id FROM candidate_branches) AND
+            target_branch IN (SELECT id FROM candidate_branches)""")]
+        tables = [BranchMergeProposal]
+        if self._asymmetric_filter_expressions:
+            # Need to filter on Branch beyond the with constraints.
+            expressions += self._asymmetric_filter_expressions
+            expressions.append(
+                BranchMergeProposal.source_branchID == Branch.id)
+            tables.append(Branch)
+            tables.extend(self._asymmetric_tables.values())
         if for_branches is not None:
             branch_ids = [branch.id for branch in for_branches]
             expressions.append(
@@ -243,7 +296,8 @@
         if statuses is not None:
             expressions.append(
                 BranchMergeProposal.queue_status.is_in(statuses))
-        return self.store.using(*tables).find(BranchMergeProposal, *expressions)
+        return self.store.with_(with_expr).using(*tables).find(
+            BranchMergeProposal, *expressions)
 
     def getMergeProposalsForPerson(self, person, status=None):
         """See `IBranchCollection`."""
@@ -420,7 +474,7 @@
 
     def ownedBy(self, person):
         """See `IBranchCollection`."""
-        return self._filterBy([Branch.owner == person])
+        return self._filterBy([Branch.owner == person], symmetric=False)
 
     def ownedByTeamMember(self, person):
         """See `IBranchCollection`."""
@@ -429,11 +483,11 @@
             where=TeamParticipation.personID==person.id)
         filter = [In(Branch.ownerID, subquery)]
 
-        return self._filterBy(filter)
+        return self._filterBy(filter, symmetric=False)
 
     def registeredBy(self, person):
         """See `IBranchCollection`."""
-        return self._filterBy([Branch.registrant == person])
+        return self._filterBy([Branch.registrant == person], symmetric=False)
 
     def relatedTo(self, person):
         """See `IBranchCollection`."""
@@ -444,7 +498,8 @@
                     Select(Branch.id, Branch.registrant == person),
                     Select(Branch.id,
                            And(BranchSubscription.person == person,
-                               BranchSubscription.branch == Branch.id))))])
+                               BranchSubscription.branch == Branch.id))))],
+            symmetric=False)
 
     def _getExactMatch(self, search_term):
         """Return the exact branch that 'search_term' matches, or None."""
@@ -510,7 +565,8 @@
             [BranchSubscription.person == person],
             table=BranchSubscription,
             join=Join(BranchSubscription,
-                      BranchSubscription.branch == Branch.id))
+                      BranchSubscription.branch == Branch.id),
+            symmetric=False)
 
     def targetedBy(self, person, since=None):
         """See `IBranchCollection`."""
@@ -521,7 +577,8 @@
             clauses,
             table=BranchMergeProposal,
             join=Join(BranchMergeProposal,
-                      BranchMergeProposal.target_branch == Branch.id))
+                      BranchMergeProposal.target_branch == Branch.id),
+            symmetric=False)
 
     def visibleByUser(self, person):
         """See `IBranchCollection`."""
@@ -531,74 +588,104 @@
         if person is None:
             return AnonymousBranchCollection(
                 self._store, self._branch_filter_expressions,
-                self._tables, self._exclude_from_search)
+                self._tables, self._exclude_from_search,
+                self._asymmetric_filter_expressions, self._asymmetric_tables)
         return VisibleBranchCollection(
             person, self._store, self._branch_filter_expressions,
-            self._tables, self._exclude_from_search)
+            self._tables, self._exclude_from_search,
+            self._asymmetric_filter_expressions, self._asymmetric_tables)
 
     def withBranchType(self, *branch_types):
-        return self._filterBy([Branch.branch_type.is_in(branch_types)])
+        return self._filterBy([Branch.branch_type.is_in(branch_types)],
+            symmetric=False)
 
     def withLifecycleStatus(self, *statuses):
         """See `IBranchCollection`."""
-        return self._filterBy([Branch.lifecycle_status.is_in(statuses)])
+        return self._filterBy([Branch.lifecycle_status.is_in(statuses)],
+            symmetric=False)
 
     def modifiedSince(self, epoch):
         """See `IBranchCollection`."""
-        return self._filterBy([Branch.date_last_modified > epoch])
+        return self._filterBy([Branch.date_last_modified > epoch],
+            symmetric=False)
 
     def scannedSince(self, epoch):
         """See `IBranchCollection`."""
-        return self._filterBy([Branch.last_scanned > epoch])
+        return self._filterBy([Branch.last_scanned > epoch], symmetric=False)
 
 
 class AnonymousBranchCollection(GenericBranchCollection):
     """Branch collection that only shows public branches."""
 
-    def __init__(self, store=None, branch_filter_expressions=None,
-                 tables=None, exclude_from_search=None):
-        super(AnonymousBranchCollection, self).__init__(
-            store=store,
-            branch_filter_expressions=list(branch_filter_expressions),
-            tables=tables, exclude_from_search=exclude_from_search)
-
     def _getBranchVisibilityExpression(self, branch_class=Branch):
         """Return the where clauses for visibility."""
         return [branch_class.private == False]
 
+    def _getCandidateBranchesWith(self):
+        """Return WITH clauses defining candidate branches.
+        
+        These are defined in terms of scope_branches which should be separately
+        calculated.
+        """
+        # Anonymous users get public branches only.
+        return [
+            With("candidate_branches",
+                SQL("select id from scope_branches where not private"))
+            ]
+
 
 class VisibleBranchCollection(GenericBranchCollection):
     """A branch collection that has special logic for visibility."""
 
     def __init__(self, user, store=None, branch_filter_expressions=None,
-                 tables=None, exclude_from_search=None):
+                 tables=None, exclude_from_search=None,
+                 asymmetric_filter_expressions=None, asymmetric_tables=None):
         super(VisibleBranchCollection, self).__init__(
             store=store, branch_filter_expressions=branch_filter_expressions,
-            tables=tables, exclude_from_search=exclude_from_search)
+            tables=tables, exclude_from_search=exclude_from_search,
+            asymmetric_filter_expressions=asymmetric_filter_expressions,
+            asymmetric_tables=asymmetric_tables)
         self._user = user
         self._private_branch_ids = self._getPrivateBranchSubQuery()
 
     def _filterBy(self, expressions, table=None, join=None,
-                  exclude_from_search=None):
-        """Return a subset of this collection, filtered by 'expressions'."""
+                  exclude_from_search=None, symmetric=True):
+        """Return a subset of this collection, filtered by 'expressions'.
+        
+        :param symmetric: If True this filter will apply to both sides of merge
+            proposal lookups and any other lookups that join Branch back onto
+            Branch.
+        """
         # NOTE: JonathanLange 2009-02-17: We might be able to avoid the need
         # for explicit 'tables' by harnessing Storm's table inference system.
         # See http://paste.ubuntu.com/118711/ for one way to do that.
-        tables = self._tables.copy()
         if table is not None:
             if join is None:
                 raise InvalidFilter("Cannot specify a table without a join.")
-            tables[table] = join
+        if expressions is None:
+            expressions = []
+        tables = self._tables.copy()
+        asymmetric_tables = self._asymmetric_tables.copy()
+        if symmetric:
+            if table is not None:
+                tables[table] = join
+            symmetric_expr = self._branch_filter_expressions + expressions
+            asymmetric_expr = list(self._asymmetric_filter_expressions)
+        else:
+            if table is not None:
+                asymmetric_tables[table] = join
+            symmetric_expr = list(self._branch_filter_expressions)
+            asymmetric_expr = self._asymmetric_filter_expressions + expressions
         if exclude_from_search is None:
             exclude_from_search = []
-        if expressions is None:
-            expressions = []
         return self.__class__(
             self._user,
             self.store,
-            self._branch_filter_expressions + expressions,
+            symmetric_expr,
             tables,
-            self._exclude_from_search + exclude_from_search)
+            self._exclude_from_search + exclude_from_search,
+            asymmetric_expr,
+            asymmetric_tables)
 
     def _getPrivateBranchSubQuery(self):
         """Return a subquery to get the private branches the user can see.
@@ -649,6 +736,33 @@
                 branch_class.id.is_in(self._private_branch_ids))
             return [public_or_private]
 
+    def _getCandidateBranchesWith(self):
+        """Return WITH clauses defining candidate branches.
+        
+        These are defined in terms of scope_branches which should be separately
+        calculated.
+        """
+        person = self._user
+        if person is None:
+            # Really an anonymous sitation
+            return [
+                With("candidate_branches",
+                    SQL("select id from scope_branches where not private"))
+                ]
+        return [
+            With("teams", self.store.find(TeamParticipation.teamID,
+                TeamParticipation.personID == person.id)._get_select()),
+            With("private_branches", SQL("""
+                SELECT scope_branches.id FROM scope_branches WHERE
+                scope_branches.private AND ((scope_branches.owner in (select team from teams) OR
+                    EXISTS(SELECT true from BranchSubscription, teams WHERE
+                        branchsubscription.branch = scope_branches.id AND
+                        branchsubscription.person = teams.team)))""")),
+            With("candidate_branches", SQL("""
+                (SELECT id FROM private_branches) UNION
+                (select id FROM scope_branches WHERE not private)"""))
+            ]
+
     def visibleByUser(self, person):
         """See `IBranchCollection`."""
         if person == self._user:

=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py	2011-03-07 12:43:40 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py	2011-03-28 22:49:01 +0000
@@ -746,8 +746,8 @@
 
     def test_just_owned_branch_merge_proposals(self):
         # If the collection only includes branches owned by a person, the
-        # getMergeProposals() will only return merge proposals for branches
-        # that are owned by that person.
+        # getMergeProposals() will only return merge proposals for source
+        # branches that are owned by that person.
         person = self.factory.makePerson()
         product = self.factory.makeProduct()
         branch1 = self.factory.makeProductBranch(

=== modified file 'versions.cfg'
--- versions.cfg	2011-03-23 16:28:51 +0000
+++ versions.cfg	2011-03-28 22:49:01 +0000
@@ -72,7 +72,7 @@
 Sphinx = 1.0.7
 soupmatchers = 0.1r53
 sourcecodegen = 0.6.9
-storm = 0.18.0.99-lpwithnodatetime-r389
+storm = 0.18.0.99-lpwithnodatetime-r391
 testtools = 0.9.8
 transaction = 1.0.0
 Twisted = 10.2.0-4395fix-1-4909