From 3ec4984f27cd8a515c91e9029f736151de508ff0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 3 Jul 2013 05:45:07 -0700 Subject: [PATCH] Use cursor-based paging in Differential Summary: Ref T603. Ref T2625. Use cursors to page Differential queries, not offsets. The trick here is that some queries are ordered. In these cases, we either need to pass some kind of tuple or do a cursor lookup. For example, if you are viewing revisions ordered by `dateModified`, we can either have the next page be something like: ?afterDateModified=2398329373&afterID=292&order=modified ...or some magical token: ?afterToken=2398329373:292&order=modified I think we did this in Conpherence, but one factor there was that paging orders update with some frequency. In most cases, I think it's reasonable to pass just the ID and do a lookup to get the actual clause value (e.g., go look up object ID 292 and see what its dateModified is) and I think this is much simpler in general. Test Plan: Set page size in Differential to 3, and paged through result lists ordered by date created and date modified. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T603, T2625 Differential Revision: https://secure.phabricator.com/D6345 --- .../DifferentialRevisionListController.php | 15 +-- .../query/DifferentialRevisionQuery.php | 101 +++++++++++++++--- ...PhabricatorCursorPagedPolicyAwareQuery.php | 2 +- 3 files changed, 95 insertions(+), 23 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionListController.php b/src/applications/differential/controller/DifferentialRevisionListController.php index bb8ef64cf8..7418fb4277 100644 --- a/src/applications/differential/controller/DifferentialRevisionListController.php +++ b/src/applications/differential/controller/DifferentialRevisionListController.php @@ -97,23 +97,18 @@ final class DifferentialRevisionListController extends DifferentialController { $pager = null; if ($this->getFilterAllowsPaging($this->filter)) { - $pager = new AphrontPagerView(); - $pager->setOffset($request->getInt('page')); - $pager->setPageSize(1000); - $pager->setURI($uri, 'page'); - - $query->setOffset($pager->getOffset()); - $query->setLimit($pager->getPageSize() + 1); + $pager = new AphrontCursorPagerView(); + $pager->readFromRequest($request); } foreach ($controls as $control) { $this->applyControlToQuery($control, $query, $params); } - $revisions = $query->execute(); - if ($pager) { - $revisions = $pager->sliceResults($revisions); + $revisions = $query->executeWithCursorPager($pager); + } else { + $revisions = $query->execute(); } $views = $this->buildViews( diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index fda1e94594..77108e8b0d 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -57,6 +57,8 @@ final class DifferentialRevisionQuery private $needCommitPHIDs = false; private $needHashes = false; + private $buildingGlobalOrder; + /* -( Query Configuration )------------------------------------------------ */ @@ -439,11 +441,12 @@ final class DifferentialRevisionQuery } if (count($selects) > 1) { + $this->buildingGlobalOrder = true; $query = qsprintf( $conn_r, '%Q %Q %Q', implode(' UNION DISTINCT ', $selects), - $this->buildOrderByClause($conn_r, $is_global = true), + $this->buildOrderClause($conn_r), $this->buildLimitClause($conn_r)); } else { $query = head($selects); @@ -463,7 +466,10 @@ final class DifferentialRevisionQuery $joins = $this->buildJoinsClause($conn_r); $where = $this->buildWhereClause($conn_r); $group_by = $this->buildGroupByClause($conn_r); - $order_by = $this->buildOrderByClause($conn_r); + + $this->buildingGlobalOrder = false; + $order_by = $this->buildOrderClause($conn_r); + $limit = $this->buildLimitClause($conn_r); return qsprintf( @@ -675,7 +681,7 @@ final class DifferentialRevisionQuery "Unknown revision status filter constant '{$this->status}'!"); } - $where[] = $this->buildPagingCLause($conn_r); + $where[] = $this->buildPagingClause($conn_r); return $this->formatWhereClause($where); } @@ -699,28 +705,99 @@ final class DifferentialRevisionQuery } } + private function loadCursorObject($id) { + $results = id(new DifferentialRevisionQuery()) + ->setViewer($this->getViewer()) + ->withIDs(array($id)) + ->execute(); + return head($results); + } - /** - * @task internal - */ - private function buildOrderByClause($conn_r, $is_global = false) { + protected function buildPagingClause(AphrontDatabaseConnection $conn_r) { + $default = parent::buildPagingClause($conn_r); + + $before_id = $this->getBeforeID(); + $after_id = $this->getAfterID(); + + if (!$before_id && !$after_id) { + return $default; + } + + if ($before_id) { + $cursor = $this->loadCursorObject($before_id); + } else { + $cursor = $this->loadCursorObject($after_id); + } + + if (!$cursor) { + return null; + } + + switch ($this->order) { + case self::ORDER_CREATED: + return $default; + case self::ORDER_MODIFIED: + if ($before_id) { + return qsprintf( + $conn_r, + '(r.dateModified %Q %d OR (r.dateModified = %d AND r.id %Q %d))', + $this->getReversePaging() ? '<' : '>', + $cursor->getDateModified(), + $cursor->getDateModified(), + $this->getReversePaging() ? '<' : '>', + $cursor->getID()); + } else { + return qsprintf( + $conn_r, + '(r.dateModified %Q %d OR (r.dateModified = %d AND r.id %Q %d))', + $this->getReversePaging() ? '>' : '<', + $cursor->getDateModified(), + $cursor->getDateModified(), + $this->getReversePaging() ? '>' : '<', + $cursor->getID()); + } + case self::ORDER_PATH_MODIFIED: + if ($before_id) { + return qsprintf( + $conn_r, + '(p.epoch %Q %d OR (p.epoch = %d AND r.id %Q %d))', + $this->getReversePaging() ? '<' : '>', + $cursor->getDateCreated(), + $cursor->getDateCreated(), + $this->getReversePaging() ? '<' : '>', + $cursor->getID()); + } else { + return qsprintf( + $conn_r, + '(p.epoch %Q %d OR (p.epoch = %d AND r.id %Q %d))', + $this->getReversePaging() ? '>' : '<', + $cursor->getDateCreated(), + $cursor->getDateCreated(), + $this->getReversePaging() ? '>' : '<', + $cursor->getID()); + } + } + } + + protected function getPagingColumn() { + $is_global = $this->buildingGlobalOrder; switch ($this->order) { case self::ORDER_MODIFIED: if ($is_global) { - return 'ORDER BY dateModified DESC'; + return 'dateModified'; } - return 'ORDER BY r.dateModified DESC'; + return 'r.dateModified'; case self::ORDER_CREATED: if ($is_global) { - return 'ORDER BY dateCreated DESC'; + return 'id'; } - return 'ORDER BY r.dateCreated DESC'; + return 'r.id'; case self::ORDER_PATH_MODIFIED: if (!$this->pathIDs) { throw new Exception( "To use ORDER_PATH_MODIFIED, you must specify withPath()."); } - return 'ORDER BY p.epoch DESC'; + return 'p.epoch'; default: throw new Exception("Unknown query order constant '{$this->order}'."); } diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 3717cfa252..27ba920f83 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -56,7 +56,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } } - final protected function buildPagingClause( + protected function buildPagingClause( AphrontDatabaseConnection $conn_r) { if ($this->beforeID) {