Merge ~cjwatson/launchpad:refactor-publisher-queries into launchpad:master

[Colin Watson]

Refactor queries in PublishingSet.getBuildsForSourceIds

`PublishingSet.getBuildsForSourceIds` made one database query to work out the set of build IDs (in an inefficient way that pulled a lot more data back into Python than needed), and then a second query to get the full result set to return. Refactor this to do all the work in a single query.

The approach I've taken here also moves the `BinaryPackageBuild` parts of the query into a left join, which should soon make it easier to extend this method to support CI builds as well.

[Simone Pelosi (pelpsi)]

LGTM!

review: Approve

diff --git a/lib/lp/soyuz/doc/publishing.rst b/lib/lp/soyuz/doc/publishing.rst
index 22ac0df..a711765 100644
--- a/lib/lp/soyuz/doc/publishing.rst
+++ b/lib/lp/soyuz/doc/publishing.rst
@@ -1132,8 +1132,8 @@ its context.
     >>> cprov_builds = publishing_set.getBuildsForSources(cprov_sources)
 
 It returns a `ResultSet` and it contains 3-element tuples as
-`SourcePackagePublishingHistory`, `Build` and `DistroArchseries` for
-each build found.
+`SourcePackagePublishingHistory`, `BinaryPackageBuild` and
+`DistroArchSeries` for each build found.
 
     >>> cprov_builds.count()
     7
diff --git a/lib/lp/soyuz/interfaces/publishing.py b/lib/lp/soyuz/interfaces/publishing.py
index f0689a0..a481aa3 100644
--- a/lib/lp/soyuz/interfaces/publishing.py
+++ b/lib/lp/soyuz/interfaces/publishing.py
@@ -1210,7 +1210,8 @@ class IPublishingSet(Interface):
         :param need_build_farm_job: whether to include the `PackageBuild`
             and `BuildFarmJob` in the result.
         :return: a storm ResultSet containing tuples as
-            (`SourcePackagePublishingHistory`, `Build`, `DistroArchSeries`)
+            (`SourcePackagePublishingHistory`, `BinaryPackageBuild`,
+            `DistroArchSeries`)
         :rtype: `storm.store.ResultSet`.
         """
 
@@ -1232,7 +1233,7 @@ class IPublishingSet(Interface):
             be limited. Defaults to BuildStatus.FULLYBUILT if none are
             specified.
         :return: a storm ResultSet containing tuples of
-            (`SourcePackagePublishingHistory`, `Build`)
+            (`SourcePackagePublishingHistory`, `BinaryPackageBuild`)
         """
 
     def getBinaryFilesForSources(one_or_more_source_publication):
diff --git a/lib/lp/soyuz/model/publishing.py b/lib/lp/soyuz/model/publishing.py
index 470032d..b5ded2d 100644
--- a/lib/lp/soyuz/model/publishing.py
+++ b/lib/lp/soyuz/model/publishing.py
@@ -18,7 +18,18 @@ from operator import attrgetter, itemgetter
 from pathlib import Path
 
 from storm.databases.postgres import JSON
-from storm.expr import And, Cast, Desc, Join, LeftJoin, Not, Or, Sum
+from storm.expr import (
+    And,
+    Cast,
+    Desc,
+    Join,
+    LeftJoin,
+    Not,
+    Or,
+    Select,
+    Sum,
+    Union,
+)
 from storm.info import ClassAlias
 from storm.properties import DateTime, Int, Unicode
 from storm.references import Reference
@@ -1719,9 +1730,26 @@ class PublishingSet:
         self, source_publication_ids, archive=None, build_states=None
     ):
         """See `IPublishingSet`."""
+        # We're interested in the binaries resulting from builds in the same
+        # distroseries as the SPPH.
+        bpb_tables = [
+            LeftJoin(
+                BinaryPackageBuild,
+                And(
+                    SourcePackagePublishingHistory.distroseries_id
+                    == BinaryPackageBuild.distro_series_id,
+                    SourcePackagePublishingHistory.sourcepackagerelease_id
+                    == BinaryPackageBuild.source_package_release_id,
+                ),
+            ),
+        ]
+
+        extra_exprs = [
+            SourcePackagePublishingHistory.id.is_in(source_publication_ids)
+        ]
+
         # If an archive was passed in as a parameter, add an extra expression
         # to filter by archive:
-        extra_exprs = []
         if archive is not None:
             extra_exprs.append(
                 SourcePackagePublishingHistory.archive == archive
@@ -1734,71 +1762,69 @@ class PublishingSet:
 
         store = IStore(SourcePackagePublishingHistory)
 
-        # We'll be looking for builds in the same distroseries as the
-        # SPPH for the same release.
-        builds_for_distroseries_expr = (
-            SourcePackagePublishingHistory.distroseries_id
-            == BinaryPackageBuild.distro_series_id,
-            SourcePackagePublishingHistory.sourcepackagerelease_id
-            == BinaryPackageBuild.source_package_release_id,
-            SourcePackagePublishingHistory.id.is_in(source_publication_ids),
-            DistroArchSeries.id == BinaryPackageBuild.distro_arch_series_id,
-        )
-
-        # First, we'll find the builds that were built in the same
-        # archive context as the published sources.
-        builds_in_same_archive = store.find(
-            BinaryPackageBuild,
-            builds_for_distroseries_expr,
-            (
+        # First, we'll find the binary package builds that were built in the
+        # same archive context as the published sources.
+        build_ids_in_same_archive = Select(
+            BinaryPackageBuild.id,
+            tables=(SourcePackagePublishingHistory, *bpb_tables),
+            where=And(
                 SourcePackagePublishingHistory.archive_id
-                == BinaryPackageBuild.archive_id
+                == BinaryPackageBuild.archive_id,
+                *extra_exprs,
             ),
-            *extra_exprs,
         )
 
-        # Next get all the builds that have a binary published in the
-        # same archive... even though the build was not built in
-        # the same context archive.
-        builds_copied_into_archive = store.find(
-            BinaryPackageBuild,
-            builds_for_distroseries_expr,
-            (
+        # Next get all the binary package builds that have a binary
+        # published in the same archive... even though the build was not
+        # built in the same context archive.
+        build_ids_copied_into_archive = Select(
+            BinaryPackageBuild.id,
+            tables=(
+                SourcePackagePublishingHistory,
+                *bpb_tables,
+                Join(
+                    BinaryPackageRelease,
+                    BinaryPackageRelease.build == BinaryPackageBuild.id,
+                ),
+                Join(
+                    BinaryPackagePublishingHistory,
+                    And(
+                        BinaryPackagePublishingHistory.archive
+                        == SourcePackagePublishingHistory.archive_id,
+                        BinaryPackagePublishingHistory.binarypackagerelease
+                        == BinaryPackageRelease.id,
+                    ),
+                ),
+            ),
+            where=And(
                 SourcePackagePublishingHistory.archive_id
-                != BinaryPackageBuild.archive_id
+                != BinaryPackageBuild.archive_id,
+                *extra_exprs,
             ),
-            BinaryPackagePublishingHistory.archive
-            == SourcePackagePublishingHistory.archive_id,
-            BinaryPackagePublishingHistory.binarypackagerelease
-            == BinaryPackageRelease.id,
-            BinaryPackageRelease.build == BinaryPackageBuild.id,
-            *extra_exprs,
         )
 
-        builds_union = builds_copied_into_archive.union(
-            builds_in_same_archive
-        ).config(distinct=True)
-
-        # Now that we have a result_set of all the builds, we'll use it
-        # as a subquery to get the required publishing and arch to do
-        # the ordering. We do this in this round-about way because we
-        # can't sort on SourcePackagePublishingHistory.id after the
-        # union. See bug 443353 for details.
-        find_spec = (
+        # Now that we have select expressions for all the builds, we'll use
+        # them as subqueries to get the required publishing and arch to do
+        # the ordering. We do this in this round-about way because we can't
+        # sort on SourcePackagePublishingHistory.id after the union. See bug
+        # 443353 for details.
+        result_set = store.using(
             SourcePackagePublishingHistory,
-            BinaryPackageBuild,
-            DistroArchSeries,
-        )
-
-        # Storm doesn't let us do builds_union.values('id') -
-        # ('Union' object has no attribute 'columns'). So instead
-        # we have to instantiate the objects just to get the id.
-        build_ids = [build.id for build in builds_union]
-
-        result_set = store.find(
-            find_spec,
-            builds_for_distroseries_expr,
-            BinaryPackageBuild.id.is_in(build_ids),
+            *bpb_tables,
+            Join(
+                DistroArchSeries,
+                BinaryPackageBuild.distro_arch_series == DistroArchSeries.id,
+            ),
+        ).find(
+            (
+                SourcePackagePublishingHistory,
+                BinaryPackageBuild,
+                DistroArchSeries,
+            ),
+            BinaryPackageBuild.id.is_in(
+                Union(build_ids_in_same_archive, build_ids_copied_into_archive)
+            ),
+            *extra_exprs,
         )
 
         return result_set.order_by(
diff --git a/lib/lp/soyuz/tests/test_publishing_models.py b/lib/lp/soyuz/tests/test_publishing_models.py
index 6339b71..755d797 100644
--- a/lib/lp/soyuz/tests/test_publishing_models.py
+++ b/lib/lp/soyuz/tests/test_publishing_models.py
@@ -40,7 +40,8 @@ class TestPublishingSet(BaseTestCaseWithThreeBuilds):
         self.publishing_set = getUtility(IPublishingSet)
 
     def _getBuildsForResults(self, results):
-        # The method returns (SPPH, Build) tuples, we just want the build.
+        # The method returns (SPPH, BinaryPackageBuild, DistroArchSeries)
+        # tuples.  We just want the binary package build.
         return [result[1] for result in results]
 
     def test_getUnpublishedBuildsForSources_none_published(self):