clusters: refactor covers to materialize

Signed-off-by: Varun Patil <radialapps@gmail.com>
pulsejet · Mar 31, 2024 · 36dcf1c · 36dcf1c
1 parent 9f2bdd2
commit 36dcf1c
Show file tree

Hide file tree

Showing 8 changed files with 118 additions and 55 deletions.
diff --git a/lib/ClustersBackend/AlbumsBackend.php b/lib/ClustersBackend/AlbumsBackend.php
@@ -82,9 +82,17 @@ public function transformDayQuery(IQueryBuilder &$query, bool $aggregate): void
 
     public function getClustersInternal(int $fileid = 0): array
     {
-        // Transformation to add covers
-        $transformOwned = function (IQueryBuilder &$query): void {
-            $this->joinCovers(
+        // Materialize the query
+        $materialize = static fn (IQueryBuilder & $query): IQueryBuilder => TimelineQuery::materialize($query, 'pa');
+
+        // Function to add etag
+        $etag = static fn (string $name): \Closure => static function (IQueryBuilder &$query) use ($name): void {
+            TimelineQuery::selectEtag($query, $name, "{$name}_etag");
+        };
+
+        // Add cover from self user
+        $ownCover = function (IQueryBuilder &$query): void {
+            $this->selectCover(
                 query: $query,
                 clusterTable: 'pa',
                 clusterTableId: 'album_id',
@@ -96,9 +104,8 @@ public function getClustersInternal(int $fileid = 0): array
         };
 
         // Transformation for shared albums
-        $transformShared = function (IQueryBuilder &$query) use ($transformOwned): void {
-            $transformOwned($query);
-            $this->joinCovers(
+        $shareCover = function (IQueryBuilder &$query): void {
+            $this->selectCover(
                 query: $query,
                 clusterTable: 'pa',
                 clusterTableId: 'album_id',
@@ -111,6 +118,18 @@ public function getClustersInternal(int $fileid = 0): array
             );
         };
 
+        // Transformations to apply to own albums
+        $transformOwned = [
+            $materialize, $ownCover,
+            $materialize, $etag('last_added_photo'), $etag('cover'),
+        ];
+
+        // Transformations to apply to shared albums
+        $transformShared = [
+            $materialize, $ownCover, $shareCover,
+            $materialize, $etag('last_added_photo'), $etag('cover'), $etag('cover_owner'),
+        ];
+
         // Get personal and shared albums
         $list = array_merge(
             $this->albumsQuery->getList(Util::getUID(), false, $fileid, $transformOwned),

diff --git a/lib/ClustersBackend/Backend.php b/lib/ClustersBackend/Backend.php
@@ -23,6 +23,7 @@
 
 namespace OCA\Memories\ClustersBackend;
 
+use OCA\Memories\Db\TimelineQuery;
 use OCA\Memories\Util;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\Files\SimpleFS\ISimpleFile;
@@ -223,7 +224,7 @@ final public function setCover(array $photo, bool $manual = false): void
     }
 
     /**
-     * Join the list query to get covers.
+     * Select the list query to get covers.
      *
      * @param IQueryBuilder $query                Query builder
      * @param string        $clusterTable         Alias name for the cluster list
@@ -235,7 +236,7 @@ final public function setCover(array $photo, bool $manual = false): void
      * @param bool          $validateFilecache    Whether to validate the filecache
      * @param mixed         $user                 Query expression for user ID to use for the covers
      */
-    final protected function joinCovers(
+    final protected function selectCover(
         IQueryBuilder &$query,
         string $clusterTable,
         string $clusterTableId,
@@ -247,30 +248,23 @@ final protected function joinCovers(
         string $field = 'cover',
         mixed $user = null,
     ): void {
-        // Create aliases for the tables
-        $mcov = "m_cov_{$field}";
-        $mcov_f = "{$mcov}_f";
-
-        // Default to current user
-        $user = $user ?? $query->expr()->literal(Util::getUser()->getUID());
-
-        // Clauses for the JOIN
-        $joinClauses = [
-            $query->expr()->eq("{$mcov}.uid", $user),
-            $query->expr()->eq("{$mcov}.clustertype", $query->expr()->literal($this->clusterType())),
-            $query->expr()->eq("{$mcov}.clusterid", "{$clusterTable}.{$clusterTableId}"),
+        // Clauses for the WHERE
+        $clauses = [
+            $query->expr()->eq('mcov.uid', $user ?? $query->expr()->literal(Util::getUser()->getUID())),
+            $query->expr()->eq('mcov.clustertype', $query->expr()->literal($this->clusterType())),
+            $query->expr()->eq('mcov.clusterid', "{$clusterTable}.{$clusterTableId}"),
         ];
 
         // Subquery if the preview is still valid for this cluster
         if ($validateCluster) {
             $validSq = $query->getConnection()->getQueryBuilder();
             $validSq->select($validSq->expr()->literal(1))
                 ->from($objectTable, 'cov_objs')
-                ->where($validSq->expr()->eq($query->expr()->castColumn("cov_objs.{$objectTableObjectId}", IQueryBuilder::PARAM_INT), "{$mcov}.objectid"))
+                ->where($validSq->expr()->eq($query->expr()->castColumn("cov_objs.{$objectTableObjectId}", IQueryBuilder::PARAM_INT), 'mcov.objectid'))
                 ->andWhere($validSq->expr()->eq("cov_objs.{$objectTableClusterId}", "{$clusterTable}.{$clusterTableId}"))
             ;
 
-            $joinClauses[] = $query->createFunction("EXISTS ({$validSq->getSQL()})");
+            $clauses[] = $query->createFunction("EXISTS ({$validSq->getSQL()})");
         }
 
         // Subquery if the file is still in the user's timeline tree
@@ -282,21 +276,22 @@ final protected function joinCovers(
                     $treeSq->expr()->eq('cov_cte_f.fileid', 'cov_f.parent'),
                     $treeSq->expr()->eq('cov_cte_f.hidden', $treeSq->expr()->literal(0, \PDO::PARAM_INT)),
                 ))
-                ->where($treeSq->expr()->eq('cov_f.fileid', "{$mcov}.fileid"))
+                ->where($treeSq->expr()->eq('cov_f.fileid', 'mcov.fileid'))
             ;
 
-            $joinClauses[] = $query->createFunction("EXISTS ({$treeSq->getSQL()})");
+            $clauses[] = $query->createFunction("EXISTS ({$treeSq->getSQL()})");
         }
 
-        // LEFT JOIN to get all the covers that we can
-        $query->leftJoin($clusterTable, 'memories_covers', $mcov, $query->expr()->andX(...$joinClauses));
-
-        // JOIN with filecache to get the etag
-        $query->leftJoin($mcov, 'filecache', $mcov_f, $query->expr()->eq("{$mcov_f}.fileid", "{$mcov}.fileid"));
+        // Make subquery to select the cover
+        $cvQ = $query->getConnection()->getQueryBuilder();
+        $cvQ->select('mcov.objectid')
+            ->from('memories_covers', 'mcov')
+            ->where($cvQ->expr()->andX(...$clauses))
+            ->setMaxResults(1)
+        ;
 
         // SELECT the cover
-        $query->selectAlias($query->createFunction("MAX({$mcov}.objectid)"), $field);
-        $query->selectAlias($query->createFunction("MAX({$mcov_f}.etag)"), "{$field}_etag");
+        $query->selectAlias(TimelineQuery::subquery($query, $cvQ), $field);
     }
 
     /**

diff --git a/lib/ClustersBackend/FaceRecognitionBackend.php b/lib/ClustersBackend/FaceRecognitionBackend.php
@@ -287,8 +287,9 @@ private function getFaceRecognitionClusters(int $fileid = 0): array
         // It is not worth displaying all unnamed clusters. We show 15 to name them progressively,
         $query->setMaxResults(15);
 
-        // JOIN to get all covers
-        $this->joinCovers(
+        // SELECT covers
+        $query = $this->tq->materialize($query, 'frp');
+        $this->selectCover(
             query: $query,
             clusterTable: 'frp',
             clusterTableId: 'id',
@@ -297,6 +298,10 @@ private function getFaceRecognitionClusters(int $fileid = 0): array
             objectTableClusterId: 'person',
         );
 
+        // SELECT etag for the cover
+        $query = $this->tq->materialize($query, 'frp');
+        $this->tq->selectEtag($query, 'cover', 'cover_etag');
+
         // FETCH all faces
         return $this->tq->executeQueryWithCTEs($query)->fetchAll() ?: [];
     }
@@ -340,8 +345,9 @@ private function getFaceRecognitionPersons(int $fileid = 0): array
         $query->orderBy('count', 'DESC');
         $query->addOrderBy('frp.name', 'ASC');
 
-        // JOIN to get all covers
-        $this->joinCovers(
+        // SELECT to get all covers
+        $query = $this->tq->materialize($query, 'frp');
+        $this->selectCover(
             query: $query,
             clusterTable: 'frp',
             clusterTableId: 'id',
@@ -350,6 +356,10 @@ private function getFaceRecognitionPersons(int $fileid = 0): array
             objectTableClusterId: 'person',
         );
 
+        // SELECT etag for the cover
+        $query = $this->tq->materialize($query, 'frp');
+        $this->tq->selectEtag($query, 'cover', 'cover_etag');
+
         // FETCH all faces
         return $this->tq->executeQueryWithCTEs($query)->fetchAll() ?: [];
     }

diff --git a/lib/ClustersBackend/PlacesBackend.php b/lib/ClustersBackend/PlacesBackend.php
@@ -148,16 +148,21 @@ public function getClustersInternal(int $fileid = 0): array
         // GROUP BY everything
         $query->addGroupBy('sub.osm_id', 'e.osm_id', 'sub.count', 'e.name', 'e.other_names');
 
-        // JOIN to get all covers
+        // SELECT to get all covers
         if ($covers) {
-            $this->joinCovers(
+            $query = $this->tq->materialize($query, 'sub');
+            $this->selectCover(
                 query: $query,
                 clusterTable: 'sub',
                 clusterTableId: 'osm_id',
                 objectTable: 'memories_places',
                 objectTableObjectId: 'fileid',
                 objectTableClusterId: 'osm_id',
             );
+
+            // SELECT etag for the cover
+            $query = $this->tq->materialize($query, 'sub');
+            $this->tq->selectEtag($query, 'cover', 'cover_etag');
         }
 
         // FETCH all tags

diff --git a/lib/ClustersBackend/RecognizeBackend.php b/lib/ClustersBackend/RecognizeBackend.php
@@ -170,8 +170,9 @@ public function getClustersInternal(int $fileid = 0): array
         $query->addOrderBy('count', 'DESC');
         $query->addOrderBy('rfc.id'); // tie-breaker
 
-        // JOIN to get all covers
-        $this->joinCovers(
+        // SELECT to get all covers
+        $query = $this->tq->materialize($query, 'rfc');
+        $this->selectCover(
             query: $query,
             clusterTable: 'rfc',
             clusterTableId: 'id',
@@ -180,6 +181,10 @@ public function getClustersInternal(int $fileid = 0): array
             objectTableClusterId: 'cluster_id',
         );
 
+        // SELECT etag for the cover
+        $query = $this->tq->materialize($query, 'rfc');
+        $this->tq->selectEtag($query, 'cover', 'cover_etag');
+
         // FETCH all faces
         $faces = $this->tq->executeQueryWithCTEs($query)->fetchAll() ?: [];
 

diff --git a/lib/ClustersBackend/TagsBackend.php b/lib/ClustersBackend/TagsBackend.php
@@ -95,8 +95,9 @@ public function getClustersInternal(int $fileid = 0): array
         $query->orderBy($query->createFunction('LOWER(st.name)'), 'ASC');
         $query->addOrderBy('st.id'); // tie-breaker
 
-        // JOIN to get all covers
-        $this->joinCovers(
+        // SELECT cover photo
+        $query = $this->tq->materialize($query, 'st');
+        $this->selectCover(
             query: $query,
             clusterTable: 'st',
             clusterTableId: 'id',
@@ -105,6 +106,10 @@ public function getClustersInternal(int $fileid = 0): array
             objectTableClusterId: 'systemtagid',
         );
 
+        // SELECT etag for the cover
+        $query = $this->tq->materialize($query, 'st');
+        $this->tq->selectEtag($query, 'cover', 'cover_etag');
+
         // FETCH all tags
         $tags = $this->tq->executeQueryWithCTEs($query)->fetchAll() ?: [];
 

diff --git a/lib/Db/AlbumsQuery.php b/lib/Db/AlbumsQuery.php
@@ -14,16 +14,16 @@ public function __construct(private IDBConnection $connection) {}
     /**
      * Get list of albums.
      *
-     * @param string   $uid         User ID
-     * @param bool     $shared      Whether to get shared albums
-     * @param int      $fileid      File to filter by
-     * @param \Closure $transformCb Callback to transform the query
+     * @param string $uid        User ID
+     * @param bool   $shared     Whether to get shared albums
+     * @param int    $fileid     File to filter by
+     * @param ?array $transforms Callbacks to transform the query
      */
     public function getList(
         string $uid,
         bool $shared = false,
         int $fileid = 0,
-        ?\Closure $transformCb = null,
+        ?array $transforms = null,
     ): array {
         $query = $this->connection->getQueryBuilder();
 
@@ -35,7 +35,6 @@ public function getList(
             'pa.name',
             'pa.user',
             'pa.created',
-            'pa.created',
             'pa.location',
             'pa.last_added_photo',
             $maxPafId,
@@ -82,13 +81,9 @@ public function getList(
             $query->andWhere($query->createFunction("EXISTS ({$fSq})"));
         }
 
-        // Get the etag of the last added photo
-        $query->leftJoin('pa', 'filecache', 'pa_fc', $query->expr()->eq('pa.last_added_photo', 'pa_fc.fileid'));
-        $query->selectAlias($query->createFunction('MAX(pa_fc.etag)'), 'last_added_photo_etag');
-
         // Apply further transformations
-        if (null !== $transformCb) {
-            $transformCb($query);
+        foreach ($transforms ?? [] as $cb) {
+            $query = $cb($query) ?? $query;
         }
 
         // FETCH all albums

diff --git a/lib/Db/TimelineQuery.php b/lib/Db/TimelineQuery.php
@@ -5,6 +5,7 @@
 namespace OCA\Memories\Db;
 
 use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\DB\QueryBuilder\IQueryFunction;
 use OCP\IDBConnection;
 use OCP\IRequest;
 
@@ -93,9 +94,37 @@ public static function materialize(IQueryBuilder $query, string $alias): IQueryB
         $outer->setParameters($query->getParameters(), $query->getParameterTypes());
 
         // Create the subquery function for selecting from it
-        $sqf = $outer->createFunction("({$query->getSQL()})");
-        $outer->select("{$alias}.*")->from($sqf, $alias);
+        $outer->select("{$alias}.*")->from(self::subquery($outer, $query), $alias);
 
         return $outer;
     }
+
+    /**
+     * Create a subquery function.
+     *
+     * @param IQueryBuilder $query    The query to create the function on
+     * @param IQueryBuilder $subquery The subquery to use
+     */
+    public static function subquery(IQueryBuilder $query, IQueryBuilder $subquery): IQueryFunction
+    {
+        return $query->createFunction("({$subquery->getSQL()})");
+    }
+
+    /**
+     * Add etag for a field in a query.
+     *
+     * @param IQueryBuilder $query The query to add the etag to
+     * @param string        $field The field to add the etag for
+     * @param string        $alias The alias to use for the etag
+     */
+    public static function selectEtag(IQueryBuilder &$query, string $field, string $alias): void
+    {
+        $sub = $query->getConnection()->getQueryBuilder();
+        $sub->select('etag')
+            ->from('filecache', 'etag_f')
+            ->where($sub->expr()->eq('etag_f.fileid', $field))
+            ->setMaxResults(1)
+        ;
+        $query->selectAlias(self::subquery($query, $sub), $alias);
+    }
 }