Service "Group by: Project" in Maniphest out of a local index

Summary:
See discussion in D6955. Currently, the logic for "Group by: Project" is roughly:

  - Load every possible result.
  - Lots of in-process garbage.

Instead, use the new local project name index (from D6957) to service this query more reasonably. Basically:

  - Join a table which has keyed project names.
  - Order by that table.

Test Plan: {F58033}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D6958

src/applications/maniphest/controller/ManiphestTaskListController.php

@@ -631,25 +631,20 @@ final class ManiphestTaskListController extends ManiphestController {
         }
         break;
       case 'project':
-        $grouped = array();
+        $data = mgroup($data, 'getGroupByProjectPHID');
-        foreach ($query->getGroupByProjectResults() as $project => $tasks) {
-          foreach ($tasks as $task) {
-            $group = 'No Project';
-            if ($project && isset($handles[$project])) {
-              $group = $handles[$project]->getName();
-            }
-            $grouped[$group][$task->getID()] = $task;
-          }
-        }
-        $data = $grouped;
-        ksort($data);
 
-        // Move "No Project" to the end of the list.
+        $out = array();
-        if (isset($data['No Project'])) {
+        foreach ($data as $phid => $tasks) {
-          $noproject = $data['No Project'];
+          $name = pht('No Project');
-          unset($data['No Project']);
+          if ($phid) {
-          $data += array('No Project' => $noproject);
+            $handle = idx($handles, $phid);
+            if ($handle) {
+              $name = $handles[$phid]->getFullName();
+            }
+          }
+          $out[$name] = $tasks;
         }
+        $data = $out;
         break;
       default:
         $data = array(

src/applications/maniphest/query/ManiphestTaskQuery.php

@@ -64,8 +64,6 @@ final class ManiphestTaskQuery
 
   private $rowCount         = null;
 
-  private $groupByProjectResults = null; // See comment at bottom for details
-
   public function withAuthors(array $authors) {
     $this->authorPHIDs = $authors;
     return $this;
@@ -171,10 +169,6 @@ final class ManiphestTaskQuery
     return $this->rowCount;
   }
 
-  public function getGroupByProjectResults() {
-    return $this->groupByProjectResults;
-  }
-
   public function withAnyProjects(array $projects) {
     $this->anyProjectPHIDs = $projects;
     return $this;
@@ -248,30 +242,8 @@ final class ManiphestTaskQuery
 
     $where = $this->formatWhereClause($where);
 
-    $join = array();
-    $join[] = $this->buildProjectJoinClause($conn);
-    $join[] = $this->buildAnyProjectJoinClause($conn);
-    $join[] = $this->buildXProjectJoinClause($conn);
-    $join[] = $this->buildSubscriberJoinClause($conn);
-
-    $join = array_filter($join);
-    if ($join) {
-      $join = implode(' ', $join);
-    } else {
-      $join = '';
-    }
-
     $having = '';
     $count = '';
-    $group = '';
-
-    if (count($this->projectPHIDs) > 1 || count($this->anyProjectPHIDs) > 1) {
-      // If we're joining multiple rows, we need to group the results by the
-      // task IDs.
-      $group = 'GROUP BY task.id';
-    } else {
-      $group = '';
-    }
 
     if (count($this->projectPHIDs) > 1) {
       // We want to treat the query as an intersection query, not a union
@@ -292,20 +264,25 @@ final class ManiphestTaskQuery
       $this->setLimit(self::DEFAULT_PAGE_SIZE);
     }
 
-    if ($this->groupBy == self::GROUP_PROJECT) {
+    $group_column = '';
-      $this->setLimit(PHP_INT_MAX);
+    switch ($this->groupBy) {
-      $this->setOffset(0);
+      case self::GROUP_PROJECT:
+        $group_column = qsprintf(
+          $conn,
+          ', projectGroupName.indexedObjectPHID projectGroupPHID');
+        break;
     }
 
-    $data = queryfx_all(
+    $rows = queryfx_all(
       $conn,
-      'SELECT %Q * %Q FROM %T task %Q %Q %Q %Q %Q %Q',
+      'SELECT %Q task.* %Q %Q FROM %T task %Q %Q %Q %Q %Q %Q',
       $calc,
       $count,
+      $group_column,
       $task_dao->getTableName(),
-      $join,
+      $this->buildJoinsClause($conn),
       $where,
-      $group,
+      $this->buildGroupClause($conn),
       $having,
       $order,
       $this->buildLimitClause($conn));
@@ -319,10 +296,27 @@ final class ManiphestTaskQuery
       $this->rowCount = null;
     }
 
+    switch ($this->groupBy) {
+      case self::GROUP_PROJECT:
+        $data = ipull($rows, null, 'id');
+        break;
+      default:
+        $data = $rows;
+        break;
+    }
+
     $tasks = $task_dao->loadAllFromArray($data);
 
-    if ($this->groupBy == self::GROUP_PROJECT) {
+    switch ($this->groupBy) {
-      $tasks = $this->applyGroupByProject($tasks);
+      case self::GROUP_PROJECT:
+        $results = array();
+        foreach ($rows as $row) {
+          $task = clone $tasks[$row['id']];
+          $task->attachGroupByProjectPHID($row['projectGroupPHID']);
+          $results[] = $task;
+        }
+        $tasks = $results;
+        break;
     }
 
     return $tasks;
@@ -514,19 +508,6 @@ final class ManiphestTaskQuery
     return '('.implode(') OR (', $parts).')';
   }
 
-  private function buildProjectJoinClause(AphrontDatabaseConnection $conn) {
-    if (!$this->projectPHIDs && !$this->includeNoProject) {
-      return null;
-    }
-
-    $project_dao = new ManiphestTaskProject();
-    return qsprintf(
-      $conn,
-      '%Q JOIN %T project ON project.taskPHID = task.phid',
-      ($this->includeNoProject ? 'LEFT' : ''),
-      $project_dao->getTableName());
-  }
-
   private function buildAnyProjectWhereClause(AphrontDatabaseConnection $conn) {
     if (!$this->anyProjectPHIDs) {
       return null;
@@ -559,18 +540,6 @@ final class ManiphestTaskQuery
       $any_user_project_phids);
   }
 
-  private function buildAnyProjectJoinClause(AphrontDatabaseConnection $conn) {
-    if (!$this->anyProjectPHIDs && !$this->anyUserProjectPHIDs) {
-      return null;
-    }
-
-    $project_dao = new ManiphestTaskProject();
-    return qsprintf(
-      $conn,
-      'JOIN %T anyproject ON anyproject.taskPHID = task.phid',
-      $project_dao->getTableName());
-  }
-
   private function buildXProjectWhereClause(AphrontDatabaseConnection $conn) {
     if (!$this->xprojectPHIDs) {
       return null;
@@ -581,32 +550,6 @@ final class ManiphestTaskQuery
       'xproject.projectPHID IS NULL');
   }
 
-  private function buildXProjectJoinClause(AphrontDatabaseConnection $conn) {
-    if (!$this->xprojectPHIDs) {
-      return null;
-    }
-
-    $project_dao = new ManiphestTaskProject();
-    return qsprintf(
-      $conn,
-      'LEFT JOIN %T xproject ON xproject.taskPHID = task.phid
-        AND xproject.projectPHID IN (%Ls)',
-      $project_dao->getTableName(),
-      $this->xprojectPHIDs);
-  }
-
-  private function buildSubscriberJoinClause(AphrontDatabaseConnection $conn) {
-    if (!$this->subscriberPHIDs) {
-      return null;
-    }
-
-    $subscriber_dao = new ManiphestTaskSubscriber();
-    return qsprintf(
-      $conn,
-      'JOIN %T subscriber ON subscriber.taskPHID = task.phid',
-      $subscriber_dao->getTableName());
-  }
-
   private function buildCustomOrderClause(AphrontDatabaseConnection $conn) {
     $order = array();
 
@@ -623,8 +566,7 @@ final class ManiphestTaskQuery
         $order[] = 'status';
         break;
       case self::GROUP_PROJECT:
-        // NOTE: We have to load the entire result set and apply this grouping
+        $order[] = '<group.project>';
-        // in the PHP process for now.
         break;
       default:
         throw new Exception("Unknown group query '{$this->groupBy}'!");
@@ -662,6 +604,12 @@ final class ManiphestTaskQuery
         case 'title':
           $order[$k] = "task.{$column} ASC";
           break;
+        case '<group.project>':
+          // Put "No Project" at the end of the list.
+          $order[$k] =
+            'projectGroupName.indexedObjectName IS NULL ASC, '.
+            'projectGroupName.indexedObjectName ASC';
+          break;
         default:
           $order[$k] = "task.{$column} DESC";
           break;
@@ -671,86 +619,125 @@ final class ManiphestTaskQuery
     return 'ORDER BY '.implode(', ', $order);
   }
 
+  private function buildJoinsClause(AphrontDatabaseConnection $conn_r) {
+    $project_dao = new ManiphestTaskProject();
 
-  /**
+    $joins = array();
-   * To get paging to work for "group by project", we need to do a bunch of
-   * server-side magic since there's currently no way to sort by project name on
-   * the database.
-   *
-   * As a consequence of this, moreover, because the list we return from here
-   * may include a single task multiple times (once for each project it's in),
-   * sorting gets screwed up in the controller unless we tell it which project
-   * to put the task in each time it appears. Hence the magic field
-   * groupByProjectResults.
-   *
-   * TODO: Move this all to the database.
-   */
-  private function applyGroupByProject(array $tasks) {
-    assert_instances_of($tasks, 'ManiphestTask');
 
-    $project_phids = array();
+    if ($this->projectPHIDs || $this->includeNoProject) {
-    foreach ($tasks as $task) {
+      $joins[] = qsprintf(
-      foreach ($task->getProjectPHIDs() as $phid) {
+        $conn_r,
-        $project_phids[$phid] = true;
+        '%Q JOIN %T project ON project.taskPHID = task.phid',
-      }
+        ($this->includeNoProject ? 'LEFT' : ''),
+        $project_dao->getTableName());
     }
 
-    $handles = id(new PhabricatorHandleQuery())
+    if ($this->anyProjectPHIDs || $this->anyUserProjectPHIDs) {
-      ->setViewer($this->getViewer())
+      $joins[] = qsprintf(
-      ->withPHIDs(array_keys($project_phids))
+        $conn_r,
-      ->execute();
+        'JOIN %T anyproject ON anyproject.taskPHID = task.phid',
-
+        $project_dao->getTableName());
-    $max = 1;
-    foreach ($handles as $handle) {
-      $max = max($max, strlen($handle->getName()));
     }
 
-    $items = array();
+    if ($this->xprojectPHIDs) {
-    $ii = 0;
+      $joins[] = qsprintf(
-    foreach ($tasks as $key => $task) {
+        $conn_r,
-      $phids = $task->getProjectPHIDs();
+        'LEFT JOIN %T xproject ON xproject.taskPHID = task.phid
-      if ($this->projectPHIDs) {
+          AND xproject.projectPHID IN (%Ls)',
-        $phids = array_diff($phids, $this->projectPHIDs);
+        $project_dao->getTableName(),
-      }
+        $this->xprojectPHIDs);
-      if ($phids) {
+    }
-        foreach ($phids as $phid) {
+
-          $items[] = array(
+    if ($this->subscriberPHIDs) {
-            'key'  => $key,
+      $subscriber_dao = new ManiphestTaskSubscriber();
-            'proj' => $phid,
+      $joins[] = qsprintf(
-            'seq'  => sprintf(
+        $conn_r,
-              '%'.$max.'s%09d',
+        'JOIN %T subscriber ON subscriber.taskPHID = task.phid',
-              $handles[$phid]->getName(),
+        $subscriber_dao->getTableName());
-              $ii),
+    }
-          );
+
+    switch ($this->groupBy) {
+      case self::GROUP_PROJECT:
+        $ignore_group_phids = $this->getIgnoreGroupedProjectPHIDs();
+        if ($ignore_group_phids) {
+          $joins[] = qsprintf(
+            $conn_r,
+            'LEFT JOIN %T projectGroup ON task.phid = projectGroup.taskPHID
+              AND projectGroup.projectPHID NOT IN (%Ls)',
+            $project_dao->getTableName(),
+            $ignore_group_phids);
+        } else {
+          $joins[] = qsprintf(
+            $conn_r,
+            'LEFT JOIN %T projectGroup ON task.phid = projectGroup.taskPHID',
+            $project_dao->getTableName());
         }
-      } else {
+        $joins[] = qsprintf(
-        // Sort "no project" tasks first.
+          $conn_r,
-        $items[] = array(
+          'LEFT JOIN %T projectGroupName
-          'key'  => $key,
+            ON projectGroup.projectPHID = projectGroupName.indexedObjectPHID',
-          'proj' => null,
+          id(new ManiphestNameIndex())->getTableName());
-          'seq'  => sprintf(
+        break;
-            '%'.$max.'s%09d',
-            '',
-            $ii),
-        );
-      }
-      ++$ii;
     }
 
-    $items = isort($items, 'seq');
+    return implode(' ', $joins);
-    $items = array_slice(
-      $items,
-      nonempty($this->getOffset()),
-      nonempty($this->getLimit(), self::DEFAULT_PAGE_SIZE));
-
-    $result = array();
-    $projects = array();
-    foreach ($items as $item) {
-      $result[] = $projects[$item['proj']][] = $tasks[$item['key']];
-    }
-    $this->groupByProjectResults = $projects;
-
-    return $result;
   }
 
+  private function buildGroupClause(AphrontDatabaseConnection $conn_r) {
+    $joined_multiple_project_rows = (count($this->projectPHIDs) > 1) ||
+                                    (count($this->anyProjectPHIDs) > 1);
+
+    $joined_project_name = ($this->groupBy == self::GROUP_PROJECT);
+
+    // If we're joining multiple rows, we need to group the results by the
+    // task IDs.
+    if ($joined_multiple_project_rows) {
+      if ($joined_project_name) {
+        return 'GROUP BY task.id, projectGroup.projectPHID';
+      } else {
+        return 'GROUP BY task.id';
+      }
+    } else {
+      return '';
+    }
+  }
+
+  /**
+   * Return project PHIDs which we should ignore when grouping tasks by
+   * project. For example, if a user issues a query like:
+   *
+   *   Tasks in all projects: Frontend, Bugs
+   *
+   * ...then we don't show "Frontend" or "Bugs" groups in the result set, since
+   * they're meaningless as all results are in both groups.
+   *
+   * Similarly, for queries like:
+   *
+   *   Tasks in any projects: Public Relations
+   *
+   * ...we ignore the single project, as every result is in that project. (In
+   * the case that there are several "any" projects, we do not ignore them.)
+   *
+   * @return list<phid> Project PHIDs which should be ignored in query
+   *                    construction.
+   */
+  private function getIgnoreGroupedProjectPHIDs() {
+    $phids = array();
+
+    if ($this->projectPHIDs) {
+      $phids[] = $this->projectPHIDs;
+    }
+
+    if (count($this->anyProjectPHIDs) == 1) {
+      $phids[] = $this->anyProjectPHIDs;
+    }
+
+    // Maybe we should also exclude the "excludeProjectPHIDs"? It won't
+    // impact the results, but we might end up with a better query plan.
+    // Investigate this on real data? This is likely very rare.
+
+    return array_mergev($phids);
+  }
+
+
 }

src/applications/maniphest/storage/ManiphestTask.php

@@ -36,6 +36,7 @@ final class ManiphestTask extends ManiphestDAO
 
   private $auxiliaryAttributes = self::ATTACHABLE;
   private $auxiliaryDirty = array();
+  private $groupByProjectPHID = self::ATTACHABLE;
 
   public function getConfiguration() {
     return array(
@@ -115,6 +116,15 @@ final class ManiphestTask extends ManiphestDAO
     return $this;
   }
 
+  public function attachGroupByProjectPHID($phid) {
+    $this->groupByProjectPHID = $phid;
+    return $this;
+  }
+
+  public function getGroupByProjectPHID() {
+    return $this->assertAttached($this->groupByProjectPHID);
+  }
+
   public function attachAuxiliaryAttributes(array $attrs) {
     if ($this->auxiliaryDirty) {
       throw new Exception(