Merge lp:~rvb/launchpad/revert-14355 into lp:launchpad

[Raphaël Badin]

Revert r14355.

This branch reverts r14355. The refactored query (which uses 2 CTEs, see OOPS-8c4f00a21be3b28698de236433c26fd3) as a worse performance than the original one. Reverted MP: https://code.launchpad.net/~rvb/launchpad/activereviews-bug-867941-hugequery/+merge/82650.

[Raphaël Badin (rvb)]

This is a straight revert so I'm self reviewing.

review: Approve

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-11-22 10:18:22 +0000
+++ lib/lp/code/model/branchcollection.py	2011-11-23 16:10:42 +0000
@@ -128,16 +128,6 @@
         if exclude_from_search is None:
             exclude_from_search = []
         self._exclude_from_search = exclude_from_search
-        self._with_dict = {}
-
-    @property
-    def store_with_with(self):
-        store = self.store
-        if len(self._with_dict) != 0:
-            with_expr = [With(name, expr)
-                for (name, expr) in self._with_dict.items()]
-            store = store.with_(with_expr)
-        return store
 
     def count(self):
         """See `IBranchCollection`."""
@@ -275,8 +265,7 @@
             self._tables.values() + self._asymmetric_tables.values())
         tables = [Branch] + list(all_tables)
         expressions = self._getBranchExpressions()
-        resultset = self.store_with_with.using(
-            *tables).find(Branch, *expressions)
+        resultset = self.store.using(*tables).find(Branch, *expressions)
         if not eager_load:
             return resultset
 
@@ -297,15 +286,6 @@
                           target_branch=None, merged_revnos=None,
                           eager_load=False):
         """See `IBranchCollection`."""
-        return self._getMergeProposals(
-            statuses=statuses, for_branches=for_branches,
-            target_branch=target_branch, merged_revnos=merged_revnos,
-            eager_load=eager_load)
-
-    def _getMergeProposals(self, statuses=None, for_branches=None,
-                          target_branch=None, merged_revnos=None,
-                          eager_load=False, return_ids=False,
-                          include_with=True):
         if for_branches is not None and not for_branches:
             # We have an empty branches list, so we can shortcut.
             return EmptyResultSet()
@@ -317,19 +297,15 @@
             target_branch is not None or
             merged_revnos is not None):
             return self._naiveGetMergeProposals(statuses, for_branches,
-                target_branch, merged_revnos, eager_load,
-                return_ids=return_ids, include_with=include_with)
+                target_branch, merged_revnos, eager_load)
         else:
             # When examining merge proposals in a scope, this is a moderately
             # effective set of constrained queries. It is not effective when
             # unscoped or when tight constraints on branches are present.
-            return self._scopedGetMergeProposals(
-                statuses, return_ids=return_ids)
+            return self._scopedGetMergeProposals(statuses)
 
     def _naiveGetMergeProposals(self, statuses=None, for_branches=None,
-                                target_branch=None, merged_revnos=None,
-                                eager_load=False, return_ids=False,
-                                include_with=True):
+        target_branch=None, merged_revnos=None, eager_load=False):
 
         def do_eager_load(rows):
             branch_ids = set()
@@ -386,20 +362,14 @@
         if statuses is not None:
             expressions.append(
                 BranchMergeProposal.queue_status.is_in(statuses))
-        if include_with:
-            store = self.store_with_with
-        else:
-            store = self.store
-        returned_obj = (
-            BranchMergeProposal.id if return_ids else BranchMergeProposal)
-        resultset = store.using(*tables).find(
-            returned_obj, *expressions)
+        resultset = self.store.using(*tables).find(
+            BranchMergeProposal, *expressions)
         if not eager_load:
             return resultset
         else:
             return DecoratedResultSet(resultset, pre_iter_hook=do_eager_load)
 
-    def _scopedGetMergeProposals(self, statuses, return_ids=False):
+    def _scopedGetMergeProposals(self, statuses):
         scope_tables = [Branch] + self._tables.values()
         scope_expressions = self._branch_filter_expressions
         select = self.store.using(*scope_tables).find(
@@ -422,23 +392,16 @@
         if statuses is not None:
             expressions.append(
                 BranchMergeProposal.queue_status.is_in(statuses))
-        returned_obj = (
-            BranchMergeProposal.id if return_ids else BranchMergeProposal)
         return self.store.with_(with_expr).using(*tables).find(
-            returned_obj, *expressions)
+            BranchMergeProposal, *expressions)
 
     def getMergeProposalsForPerson(self, person, status=None):
         """See `IBranchCollection`."""
         # We want to limit the proposals to those where the source branch is
         # limited by the defined collection.
-        owned = self.ownedBy(person)._getMergeProposals(
-            status, include_with=False, return_ids=True)
-        reviewing = self._getMergeProposalsForReviewer(
-            person, status, include_with=False, return_ids=True)
-        result = owned.union(reviewing)
-        resultset = self.store_with_with.using([BranchMergeProposal]).find(
-            BranchMergeProposal,
-            In(BranchMergeProposal.id, result._get_select()))
+        owned = self.ownedBy(person).getMergeProposals(status)
+        reviewing = self.getMergeProposalsForReviewer(person, status)
+        resultset = owned.union(reviewing)
 
         def do_eager_load(rows):
             # Load the related diffs.
@@ -468,10 +431,6 @@
 
     def getMergeProposalsForReviewer(self, reviewer, status=None):
         """See `IBranchCollection`."""
-        return self._getMergeProposalsForReviewer(reviewer, status=status)
-
-    def _getMergeProposalsForReviewer(self, reviewer, status=None,
-                                     include_with=True, return_ids=False):
         tables = [
             BranchMergeProposal,
             Join(CodeReviewVoteReference,
@@ -491,14 +450,8 @@
         if status is not None:
             expressions.append(
                 BranchMergeProposal.queue_status.is_in(status))
-        if include_with:
-            store = self.store_with_with
-        else:
-            store = self.store
-        returned_obj = (
-            BranchMergeProposal.id if return_ids else BranchMergeProposal)
-        proposals = store.using(*tables).find(
-            returned_obj, *expressions)
+        proposals = self.store.using(*tables).find(
+            BranchMergeProposal, *expressions)
         # Apply sorting here as we can't do it in the browser code.  We need
         # to think carefully about the best places to do this, but not here
         # nor now.
@@ -900,29 +853,21 @@
                        Branch.transitively_private == True)))
         return private_branches
 
-    def _addVisibleBranchesCTE(self):
-        """Add the 'visible_branches' CTE to self._with_dict.
-
-        """
-        public_branches = Branch.transitively_private == False
-        if self._private_branch_ids is not None:
-            branches = Or(
-                public_branches,
-                Branch.id.is_in(self._private_branch_ids))
-        else:
-            branches = public_branches
-        branches_select = self.store.using(
-            [Branch]).find(Branch.id, branches)._get_select()
-        self._with_dict.update({'visible_branches': branches_select})
-
     def _getBranchVisibilityExpression(self, branch_class=Branch):
         """Return the where clauses for visibility.
 
         :param branch_class: The Branch class to use - permits using
             ClassAliases.
         """
-        self._addVisibleBranchesCTE()
-        return [branch_class.id.is_in(SQL("SELECT id FROM visible_branches"))]
+        public_branches = branch_class.transitively_private == False
+        if self._private_branch_ids is None:
+            # Public only.
+            return [public_branches]
+        else:
+            public_or_private = Or(
+                public_branches,
+                branch_class.id.is_in(self._private_branch_ids))
+            return [public_or_private]
 
     def _getCandidateBranchesWith(self):
         """Return WITH clauses defining candidate branches.