2011-10-02 21:03:16 +02:00
|
|
|
<?php
|
|
|
|
|
|
|
|
/*
|
2012-01-04 06:08:12 +01:00
|
|
|
* Copyright 2012 Facebook, Inc.
|
2011-10-02 21:03:16 +02:00
|
|
|
*
|
|
|
|
* Licensed under the Apache License, Version 2.0 (the "License");
|
|
|
|
* you may not use this file except in compliance with the License.
|
|
|
|
* You may obtain a copy of the License at
|
|
|
|
*
|
|
|
|
* http://www.apache.org/licenses/LICENSE-2.0
|
|
|
|
*
|
|
|
|
* Unless required by applicable law or agreed to in writing, software
|
|
|
|
* distributed under the License is distributed on an "AS IS" BASIS,
|
|
|
|
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
|
|
|
* See the License for the specific language governing permissions and
|
|
|
|
* limitations under the License.
|
|
|
|
*/
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Flexible query API for Differential revisions. Example:
|
|
|
|
*
|
|
|
|
* // Load open revisions
|
|
|
|
* $revisions = id(new DifferentialRevisionQuery())
|
|
|
|
* ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
|
|
|
|
* ->execute();
|
|
|
|
*
|
|
|
|
* @task config Query Configuration
|
|
|
|
* @task exec Query Execution
|
|
|
|
* @task internal Internals
|
|
|
|
*/
|
|
|
|
final class DifferentialRevisionQuery {
|
|
|
|
|
|
|
|
// TODO: Replace DifferentialRevisionListData with this class.
|
|
|
|
|
|
|
|
private $pathIDs = array();
|
|
|
|
|
|
|
|
private $status = 'status-any';
|
|
|
|
const STATUS_ANY = 'status-any';
|
|
|
|
const STATUS_OPEN = 'status-open';
|
|
|
|
|
2011-12-06 23:21:36 +01:00
|
|
|
private $authors = array();
|
2011-12-02 01:57:06 +01:00
|
|
|
private $ccs = array();
|
|
|
|
private $reviewers = array();
|
|
|
|
private $revIDs = array();
|
2012-01-04 06:08:12 +01:00
|
|
|
private $commitHashes = array();
|
2011-12-02 01:57:06 +01:00
|
|
|
private $phids = array();
|
2011-12-07 17:30:49 +01:00
|
|
|
private $subscribers = array();
|
|
|
|
private $responsibles = array();
|
2011-12-02 01:57:06 +01:00
|
|
|
|
2011-10-02 21:03:16 +02:00
|
|
|
private $order = 'order-modified';
|
|
|
|
const ORDER_MODIFIED = 'order-modified';
|
|
|
|
const ORDER_CREATED = 'order-created';
|
|
|
|
/**
|
|
|
|
* This is essentially a denormalized copy of the revision modified time that
|
|
|
|
* should perform better for path queries with a LIMIT. Critically, when you
|
|
|
|
* browse "/", every revision in that repository for all time will match so
|
|
|
|
* the query benefits from being able to stop before fully materializing the
|
|
|
|
* result set.
|
|
|
|
*/
|
|
|
|
const ORDER_PATH_MODIFIED = 'order-path-modified';
|
|
|
|
|
|
|
|
private $limit = 1000;
|
|
|
|
private $offset = 0;
|
|
|
|
|
2011-12-21 02:05:52 +01:00
|
|
|
private $needRelationships = false;
|
|
|
|
private $needActiveDiffs = false;
|
|
|
|
private $needDiffIDs = false;
|
|
|
|
private $needCommitPHIDs = false;
|
2011-10-02 21:03:16 +02:00
|
|
|
|
|
|
|
|
|
|
|
/* -( Query Configuration )------------------------------------------------ */
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Filter results to revisions which affect a Diffusion path ID in a given
|
|
|
|
* repository. You can call this multiple times to select revisions for
|
|
|
|
* several paths.
|
|
|
|
*
|
|
|
|
* @param int Diffusion repository ID.
|
|
|
|
* @param int Diffusion path ID.
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function withPath($repository_id, $path_id) {
|
|
|
|
$this->pathIDs[] = array(
|
|
|
|
'repositoryID' => $repository_id,
|
|
|
|
'pathID' => $path_id,
|
|
|
|
);
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2011-12-02 01:57:06 +01:00
|
|
|
/**
|
2012-01-04 06:08:12 +01:00
|
|
|
* Filter results to revisions authored by one of the given PHIDs. Calling
|
|
|
|
* this function will clear anything set by previous calls to
|
|
|
|
* @{method:withAuthors}.
|
2011-12-06 23:21:36 +01:00
|
|
|
*
|
|
|
|
* @param array List of PHIDs of authors
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
2011-12-07 17:30:49 +01:00
|
|
|
public function withAuthors(array $author_phids) {
|
|
|
|
$this->authors = $author_phids;
|
2011-12-06 23:21:36 +01:00
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Filter results to revisions which CC one of the listed people. Calling this
|
2011-12-07 17:30:49 +01:00
|
|
|
* function will clear anything set by previous calls to @{method:withCCs}.
|
2011-12-02 01:57:06 +01:00
|
|
|
*
|
|
|
|
* @param array List of PHIDs of subscribers
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function withCCs(array $cc_phids) {
|
|
|
|
$this->ccs = $cc_phids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
/**
|
2011-12-06 23:21:36 +01:00
|
|
|
* Filter results to revisions that have one of the provided PHIDs as
|
2011-12-02 01:57:06 +01:00
|
|
|
* reviewers. Calling this function will clear anything set by previous calls
|
2011-12-07 17:30:49 +01:00
|
|
|
* to @{method:withReviewers}.
|
2011-12-02 01:57:06 +01:00
|
|
|
*
|
|
|
|
* @param array List of PHIDs of reviewers
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function withReviewers(array $reviewer_phids) {
|
|
|
|
$this->reviewers = $reviewer_phids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2012-01-04 06:08:12 +01:00
|
|
|
/**
|
|
|
|
* Filter results to revisions that have one of the provided commit hashes.
|
|
|
|
* Calling this function will clear anything set by previous calls to
|
|
|
|
* @{method:withCommitHashes}.
|
|
|
|
*
|
2012-01-10 20:39:11 +01:00
|
|
|
* @param array List of pairs <Class
|
|
|
|
* ArcanistDifferentialRevisionHash::HASH_$type constant,
|
|
|
|
* hash>
|
2012-01-04 06:08:12 +01:00
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function withCommitHashes(array $commit_hashes) {
|
|
|
|
$this->commitHashes = $commit_hashes;
|
|
|
|
return $this;
|
|
|
|
}
|
2011-10-02 21:03:16 +02:00
|
|
|
|
|
|
|
/**
|
|
|
|
* Filter results to revisions with a given status. Provide a class constant,
|
|
|
|
* such as ##DifferentialRevisionQuery::STATUS_OPEN##.
|
|
|
|
*
|
|
|
|
* @param const Class STATUS constant, like STATUS_OPEN.
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function withStatus($status_constant) {
|
|
|
|
$this->status = $status_constant;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
2011-12-02 01:57:06 +01:00
|
|
|
/**
|
|
|
|
* Filter results to only return revisions whose ids are in the given set.
|
|
|
|
*
|
|
|
|
* @param array List of revision ids
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function withIDs(array $ids) {
|
|
|
|
$this->revIDs = $ids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Filter results to only return revisions whose PHIDs are in the given set.
|
|
|
|
*
|
|
|
|
* @param array List of revision PHIDs
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function withPHIDs(array $phids) {
|
|
|
|
$this->phids = $phids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
2011-12-07 17:30:49 +01:00
|
|
|
/**
|
|
|
|
* Given a set of users, filter results to return only revisions they are
|
|
|
|
* responsible for (i.e., they are either authors or reviewers).
|
|
|
|
*
|
|
|
|
* @param array List of user PHIDs.
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function withResponsibleUsers(array $responsible_phids) {
|
|
|
|
$this->responsibles = $responsible_phids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Filter results to only return revisions with a given set of subscribers
|
|
|
|
* (i.e., they are authors, reviewers or CC'd).
|
|
|
|
*
|
|
|
|
* @param array List of user PHIDs.
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function withSubscribers(array $subscriber_phids) {
|
|
|
|
$this->subscribers = $subscriber_phids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
2011-10-02 21:03:16 +02:00
|
|
|
/**
|
|
|
|
* Set result ordering. Provide a class constant, such as
|
|
|
|
* ##DifferentialRevisionQuery::ORDER_CREATED##.
|
|
|
|
*
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function setOrder($order_constant) {
|
|
|
|
$this->order = $order_constant;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Set result limit. If unspecified, defaults to 1000.
|
|
|
|
*
|
|
|
|
* @param int Result limit.
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function setLimit($limit) {
|
|
|
|
$this->limit = $limit;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Set result offset. If unspecified, defaults to 0.
|
|
|
|
*
|
|
|
|
* @param int Result offset.
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function setOffset($offset) {
|
|
|
|
$this->offset = $offset;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Set whether or not the query will load and attach relationships.
|
|
|
|
*
|
|
|
|
* @param bool True to load and attach relationships.
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function needRelationships($need_relationships) {
|
|
|
|
$this->needRelationships = $need_relationships;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
2011-12-21 02:05:52 +01:00
|
|
|
/**
|
|
|
|
* Set whether or not the query should load the active diff for each
|
|
|
|
* revision.
|
|
|
|
*
|
|
|
|
* @param bool True to load and attach diffs.
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function needActiveDiffs($need_active_diffs) {
|
|
|
|
$this->needActiveDiffs = $need_active_diffs;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Set whether or not the query should load the associated commit PHIDs for
|
|
|
|
* each revision.
|
|
|
|
*
|
|
|
|
* @param bool True to load and attach diffs.
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function needCommitPHIDs($need_commit_phids) {
|
|
|
|
$this->needCommitPHIDs = $need_commit_phids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Set whether or not the query should load associated diff IDs for each
|
|
|
|
* revision.
|
|
|
|
*
|
|
|
|
* @param bool True to load and attach diff IDs.
|
|
|
|
* @return this
|
|
|
|
* @task config
|
|
|
|
*/
|
|
|
|
public function needDiffIDs($need_diff_ids) {
|
|
|
|
$this->needDiffIDs = $need_diff_ids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
2011-10-02 21:03:16 +02:00
|
|
|
/* -( Query Execution )---------------------------------------------------- */
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Execute the query as configured, returning matching
|
|
|
|
* @{class:DifferentialRevision} objects.
|
|
|
|
*
|
|
|
|
* @return list List of matching DifferentialRevision objects.
|
|
|
|
* @task exec
|
|
|
|
*/
|
|
|
|
public function execute() {
|
|
|
|
$table = new DifferentialRevision();
|
|
|
|
$conn_r = $table->establishConnection('r');
|
|
|
|
|
Drive Differential landing page with DifferentialRevisionQuery, simplify UI
Summary:
- Use DifferentialRevisionQuery, not DifferentialRevisionListData, to select
revisions.
- Make UI simpler (I hope?) and more flexible, similar to Maniphest. It now
shows "Active", "Revisions", "Reviews" and "Subscribed" instead of a hodge-podge
of miscellaneous stuff. All now really has all revisions, not just open
revisions.
- Allow views to be filtered and sorted more flexibly.
- Allow anonymous users to use the per-user views, just don't default them
there.
NOTE: This might have performance implications! I need some help evaluating
them.
@nh / @jungejason / @aran, can one of you run some queries agianst FB's corpus?
The "active revisions" view is built much differently now. Before, we issued two
queries:
- SELECT (open revisions you authored that need revision) UNION ALL (open
revisions you are reviewing that need review)
- SELECT (open revisions you authored that need review) UNION ALL (open
revisions you are reviewing that need revision)
These two queries generate the "Action Required" and "Waiting on Others" views,
and are available in P247.
Now, we issue only one query:
- SELECT (open revisions you authored or are reviewing)
Then we divide them into the two tables in PHP. That query is available in P246.
On the secure.phabricator.com data, this new approach seems to be much better
(like, 10x better). But the secure.phabricator.com data isn't very large. Can
someone run it against Facebook's data (using a few heavy-hitting PHIDs, like
ola or something) to make sure it won't cause a regression?
In particular:
- Run the queries and make sure the new version doesn't take too long.
- Run the queries with EXPLAIN and give me the output maybe?
Test Plan:
- Looked at different filters.
- Changed "View User" PHID.
- Changed open/all.
- Changed sort order.
- Ran EXPLAIN / select against secure.phabricator.com corpus.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: cpiro, aran, btrahan, epriestley, jungejason, nh
Maniphest Tasks: T586
Differential Revision: 1186
2011-12-08 00:35:10 +01:00
|
|
|
if ($this->shouldUseResponsibleFastPath()) {
|
|
|
|
$data = $this->loadDataUsingResponsibleFastPath();
|
|
|
|
} else {
|
|
|
|
$data = $this->loadData();
|
|
|
|
}
|
2011-10-02 21:03:16 +02:00
|
|
|
|
|
|
|
$revisions = $table->loadAllFromArray($data);
|
|
|
|
|
2011-12-21 02:05:52 +01:00
|
|
|
if ($revisions) {
|
|
|
|
if ($this->needRelationships) {
|
|
|
|
$this->loadRelationships($conn_r, $revisions);
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($this->needCommitPHIDs) {
|
|
|
|
$this->loadCommitPHIDs($conn_r, $revisions);
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($this->needActiveDiffs || $this->needDiffIDs) {
|
|
|
|
$this->loadDiffIDs($conn_r, $revisions);
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($this->needActiveDiffs) {
|
|
|
|
$this->loadActiveDiffs($conn_r, $revisions);
|
2011-10-02 21:03:16 +02:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
return $revisions;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
Drive Differential landing page with DifferentialRevisionQuery, simplify UI
Summary:
- Use DifferentialRevisionQuery, not DifferentialRevisionListData, to select
revisions.
- Make UI simpler (I hope?) and more flexible, similar to Maniphest. It now
shows "Active", "Revisions", "Reviews" and "Subscribed" instead of a hodge-podge
of miscellaneous stuff. All now really has all revisions, not just open
revisions.
- Allow views to be filtered and sorted more flexibly.
- Allow anonymous users to use the per-user views, just don't default them
there.
NOTE: This might have performance implications! I need some help evaluating
them.
@nh / @jungejason / @aran, can one of you run some queries agianst FB's corpus?
The "active revisions" view is built much differently now. Before, we issued two
queries:
- SELECT (open revisions you authored that need revision) UNION ALL (open
revisions you are reviewing that need review)
- SELECT (open revisions you authored that need review) UNION ALL (open
revisions you are reviewing that need revision)
These two queries generate the "Action Required" and "Waiting on Others" views,
and are available in P247.
Now, we issue only one query:
- SELECT (open revisions you authored or are reviewing)
Then we divide them into the two tables in PHP. That query is available in P246.
On the secure.phabricator.com data, this new approach seems to be much better
(like, 10x better). But the secure.phabricator.com data isn't very large. Can
someone run it against Facebook's data (using a few heavy-hitting PHIDs, like
ola or something) to make sure it won't cause a regression?
In particular:
- Run the queries and make sure the new version doesn't take too long.
- Run the queries with EXPLAIN and give me the output maybe?
Test Plan:
- Looked at different filters.
- Changed "View User" PHID.
- Changed open/all.
- Changed sort order.
- Ran EXPLAIN / select against secure.phabricator.com corpus.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: cpiro, aran, btrahan, epriestley, jungejason, nh
Maniphest Tasks: T586
Differential Revision: 1186
2011-12-08 00:35:10 +01:00
|
|
|
/**
|
|
|
|
* Determine if we should execute an optimized, fast-path query to fetch
|
|
|
|
* open revisions for one responsible user. This is used by the Differential
|
|
|
|
* dashboard and much faster when executed as a UNION ALL than with JOIN
|
|
|
|
* and WHERE, which is why we special case it.
|
|
|
|
*/
|
|
|
|
private function shouldUseResponsibleFastPath() {
|
|
|
|
if ((count($this->responsibles) == 1) &&
|
|
|
|
($this->status == self::STATUS_OPEN) &&
|
|
|
|
($this->order == self::ORDER_MODIFIED) &&
|
|
|
|
!$this->offset &&
|
|
|
|
!$this->limit &&
|
|
|
|
!$this->subscribers &&
|
|
|
|
!$this->reviewers &&
|
|
|
|
!$this->ccs &&
|
|
|
|
!$this->authors &&
|
|
|
|
!$this->revIDs &&
|
2012-01-04 06:08:12 +01:00
|
|
|
!$this->commitHashes &&
|
Drive Differential landing page with DifferentialRevisionQuery, simplify UI
Summary:
- Use DifferentialRevisionQuery, not DifferentialRevisionListData, to select
revisions.
- Make UI simpler (I hope?) and more flexible, similar to Maniphest. It now
shows "Active", "Revisions", "Reviews" and "Subscribed" instead of a hodge-podge
of miscellaneous stuff. All now really has all revisions, not just open
revisions.
- Allow views to be filtered and sorted more flexibly.
- Allow anonymous users to use the per-user views, just don't default them
there.
NOTE: This might have performance implications! I need some help evaluating
them.
@nh / @jungejason / @aran, can one of you run some queries agianst FB's corpus?
The "active revisions" view is built much differently now. Before, we issued two
queries:
- SELECT (open revisions you authored that need revision) UNION ALL (open
revisions you are reviewing that need review)
- SELECT (open revisions you authored that need review) UNION ALL (open
revisions you are reviewing that need revision)
These two queries generate the "Action Required" and "Waiting on Others" views,
and are available in P247.
Now, we issue only one query:
- SELECT (open revisions you authored or are reviewing)
Then we divide them into the two tables in PHP. That query is available in P246.
On the secure.phabricator.com data, this new approach seems to be much better
(like, 10x better). But the secure.phabricator.com data isn't very large. Can
someone run it against Facebook's data (using a few heavy-hitting PHIDs, like
ola or something) to make sure it won't cause a regression?
In particular:
- Run the queries and make sure the new version doesn't take too long.
- Run the queries with EXPLAIN and give me the output maybe?
Test Plan:
- Looked at different filters.
- Changed "View User" PHID.
- Changed open/all.
- Changed sort order.
- Ran EXPLAIN / select against secure.phabricator.com corpus.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: cpiro, aran, btrahan, epriestley, jungejason, nh
Maniphest Tasks: T586
Differential Revision: 1186
2011-12-08 00:35:10 +01:00
|
|
|
!$this->phids) {
|
|
|
|
return true;
|
|
|
|
}
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
private function loadDataUsingResponsibleFastPath() {
|
|
|
|
$table = new DifferentialRevision();
|
|
|
|
$conn_r = $table->establishConnection('r');
|
|
|
|
|
|
|
|
$responsible_phid = reset($this->responsibles);
|
|
|
|
$open_statuses = array(
|
2012-01-10 20:39:11 +01:00
|
|
|
ArcanistDifferentialRevisionStatus::NEEDS_REVIEW,
|
|
|
|
ArcanistDifferentialRevisionStatus::NEEDS_REVISION,
|
|
|
|
ArcanistDifferentialRevisionStatus::ACCEPTED,
|
Drive Differential landing page with DifferentialRevisionQuery, simplify UI
Summary:
- Use DifferentialRevisionQuery, not DifferentialRevisionListData, to select
revisions.
- Make UI simpler (I hope?) and more flexible, similar to Maniphest. It now
shows "Active", "Revisions", "Reviews" and "Subscribed" instead of a hodge-podge
of miscellaneous stuff. All now really has all revisions, not just open
revisions.
- Allow views to be filtered and sorted more flexibly.
- Allow anonymous users to use the per-user views, just don't default them
there.
NOTE: This might have performance implications! I need some help evaluating
them.
@nh / @jungejason / @aran, can one of you run some queries agianst FB's corpus?
The "active revisions" view is built much differently now. Before, we issued two
queries:
- SELECT (open revisions you authored that need revision) UNION ALL (open
revisions you are reviewing that need review)
- SELECT (open revisions you authored that need review) UNION ALL (open
revisions you are reviewing that need revision)
These two queries generate the "Action Required" and "Waiting on Others" views,
and are available in P247.
Now, we issue only one query:
- SELECT (open revisions you authored or are reviewing)
Then we divide them into the two tables in PHP. That query is available in P246.
On the secure.phabricator.com data, this new approach seems to be much better
(like, 10x better). But the secure.phabricator.com data isn't very large. Can
someone run it against Facebook's data (using a few heavy-hitting PHIDs, like
ola or something) to make sure it won't cause a regression?
In particular:
- Run the queries and make sure the new version doesn't take too long.
- Run the queries with EXPLAIN and give me the output maybe?
Test Plan:
- Looked at different filters.
- Changed "View User" PHID.
- Changed open/all.
- Changed sort order.
- Ran EXPLAIN / select against secure.phabricator.com corpus.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: cpiro, aran, btrahan, epriestley, jungejason, nh
Maniphest Tasks: T586
Differential Revision: 1186
2011-12-08 00:35:10 +01:00
|
|
|
);
|
|
|
|
|
|
|
|
return queryfx_all(
|
|
|
|
$conn_r,
|
|
|
|
'SELECT * FROM %T WHERE authorPHID = %s AND status IN (%Ld)
|
|
|
|
UNION ALL
|
|
|
|
SELECT r.* FROM %T r JOIN %T rel
|
|
|
|
ON rel.revisionID = r.id
|
|
|
|
AND rel.relation = %s
|
|
|
|
AND rel.objectPHID = %s
|
2011-12-16 22:34:17 +01:00
|
|
|
WHERE r.status IN (%Ld) ORDER BY dateModified DESC',
|
Drive Differential landing page with DifferentialRevisionQuery, simplify UI
Summary:
- Use DifferentialRevisionQuery, not DifferentialRevisionListData, to select
revisions.
- Make UI simpler (I hope?) and more flexible, similar to Maniphest. It now
shows "Active", "Revisions", "Reviews" and "Subscribed" instead of a hodge-podge
of miscellaneous stuff. All now really has all revisions, not just open
revisions.
- Allow views to be filtered and sorted more flexibly.
- Allow anonymous users to use the per-user views, just don't default them
there.
NOTE: This might have performance implications! I need some help evaluating
them.
@nh / @jungejason / @aran, can one of you run some queries agianst FB's corpus?
The "active revisions" view is built much differently now. Before, we issued two
queries:
- SELECT (open revisions you authored that need revision) UNION ALL (open
revisions you are reviewing that need review)
- SELECT (open revisions you authored that need review) UNION ALL (open
revisions you are reviewing that need revision)
These two queries generate the "Action Required" and "Waiting on Others" views,
and are available in P247.
Now, we issue only one query:
- SELECT (open revisions you authored or are reviewing)
Then we divide them into the two tables in PHP. That query is available in P246.
On the secure.phabricator.com data, this new approach seems to be much better
(like, 10x better). But the secure.phabricator.com data isn't very large. Can
someone run it against Facebook's data (using a few heavy-hitting PHIDs, like
ola or something) to make sure it won't cause a regression?
In particular:
- Run the queries and make sure the new version doesn't take too long.
- Run the queries with EXPLAIN and give me the output maybe?
Test Plan:
- Looked at different filters.
- Changed "View User" PHID.
- Changed open/all.
- Changed sort order.
- Ran EXPLAIN / select against secure.phabricator.com corpus.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: cpiro, aran, btrahan, epriestley, jungejason, nh
Maniphest Tasks: T586
Differential Revision: 1186
2011-12-08 00:35:10 +01:00
|
|
|
$table->getTableName(),
|
|
|
|
$responsible_phid,
|
|
|
|
$open_statuses,
|
|
|
|
|
|
|
|
$table->getTableName(),
|
|
|
|
DifferentialRevision::RELATIONSHIP_TABLE,
|
|
|
|
DifferentialRevision::RELATION_REVIEWER,
|
|
|
|
$responsible_phid,
|
|
|
|
$open_statuses);
|
|
|
|
}
|
|
|
|
|
|
|
|
private function loadData() {
|
|
|
|
$table = new DifferentialRevision();
|
|
|
|
$conn_r = $table->establishConnection('r');
|
|
|
|
|
|
|
|
$select = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'SELECT r.* FROM %T r',
|
|
|
|
$table->getTableName());
|
|
|
|
|
|
|
|
$joins = $this->buildJoinsClause($conn_r);
|
|
|
|
$where = $this->buildWhereClause($conn_r);
|
|
|
|
$group_by = $this->buildGroupByClause($conn_r);
|
|
|
|
$order_by = $this->buildOrderByClause($conn_r);
|
|
|
|
|
|
|
|
$limit = '';
|
|
|
|
if ($this->offset || $this->limit) {
|
|
|
|
$limit = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'LIMIT %d, %d',
|
|
|
|
(int)$this->offset,
|
|
|
|
$this->limit);
|
|
|
|
}
|
|
|
|
|
|
|
|
return queryfx_all(
|
|
|
|
$conn_r,
|
|
|
|
'%Q %Q %Q %Q %Q %Q',
|
|
|
|
$select,
|
|
|
|
$joins,
|
|
|
|
$where,
|
|
|
|
$group_by,
|
|
|
|
$order_by,
|
|
|
|
$limit);
|
|
|
|
}
|
|
|
|
|
|
|
|
|
2011-10-02 21:03:16 +02:00
|
|
|
/* -( Internals )---------------------------------------------------------- */
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* @task internal
|
|
|
|
*/
|
|
|
|
private function buildJoinsClause($conn_r) {
|
|
|
|
$joins = array();
|
|
|
|
if ($this->pathIDs) {
|
|
|
|
$path_table = new DifferentialAffectedPath();
|
|
|
|
$joins[] = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'JOIN %T p ON p.revisionID = r.id',
|
|
|
|
$path_table->getTableName());
|
|
|
|
}
|
|
|
|
|
2012-01-04 06:08:12 +01:00
|
|
|
if ($this->commitHashes) {
|
|
|
|
$joins[] = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'JOIN %T hash_rel ON hash_rel.revisionID = r.id',
|
2012-01-10 20:39:11 +01:00
|
|
|
ArcanistDifferentialRevisionHash::TABLE_NAME);
|
2012-01-04 06:08:12 +01:00
|
|
|
}
|
|
|
|
|
2011-12-06 23:21:36 +01:00
|
|
|
if ($this->ccs) {
|
|
|
|
$joins[] = qsprintf(
|
|
|
|
$conn_r,
|
2011-12-07 17:30:49 +01:00
|
|
|
'JOIN %T cc_rel ON cc_rel.revisionID = r.id '.
|
|
|
|
'AND cc_rel.relation = %s '.
|
|
|
|
'AND cc_rel.objectPHID in (%Ls)',
|
2011-12-06 23:21:36 +01:00
|
|
|
DifferentialRevision::RELATIONSHIP_TABLE,
|
|
|
|
DifferentialRevision::RELATION_SUBSCRIBED,
|
|
|
|
$this->ccs);
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($this->reviewers) {
|
|
|
|
$joins[] = qsprintf(
|
|
|
|
$conn_r,
|
2011-12-07 17:30:49 +01:00
|
|
|
'JOIN %T reviewer_rel ON reviewer_rel.revisionID = r.id '.
|
|
|
|
'AND reviewer_rel.relation = %s '.
|
|
|
|
'AND reviewer_rel.objectPHID in (%Ls)',
|
2011-12-06 23:21:36 +01:00
|
|
|
DifferentialRevision::RELATIONSHIP_TABLE,
|
|
|
|
DifferentialRevision::RELATION_REVIEWER,
|
|
|
|
$this->reviewers);
|
|
|
|
}
|
|
|
|
|
2011-12-07 17:30:49 +01:00
|
|
|
if ($this->subscribers) {
|
|
|
|
$joins[] = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'JOIN %T sub_rel ON sub_rel.revisionID = r.id '.
|
|
|
|
'AND sub_rel.relation IN (%Ls) '.
|
|
|
|
'AND sub_rel.objectPHID in (%Ls)',
|
|
|
|
DifferentialRevision::RELATIONSHIP_TABLE,
|
|
|
|
array(
|
|
|
|
DifferentialRevision::RELATION_SUBSCRIBED,
|
|
|
|
DifferentialRevision::RELATION_REVIEWER,
|
|
|
|
),
|
|
|
|
$this->subscribers);
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($this->responsibles) {
|
|
|
|
$joins[] = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'LEFT JOIN %T responsibles_rel ON responsibles_rel.revisionID = r.id '.
|
|
|
|
'AND responsibles_rel.relation = %s '.
|
|
|
|
'AND responsibles_rel.objectPHID in (%Ls)',
|
|
|
|
DifferentialRevision::RELATIONSHIP_TABLE,
|
|
|
|
DifferentialRevision::RELATION_REVIEWER,
|
|
|
|
$this->responsibles);
|
|
|
|
}
|
|
|
|
|
2011-10-02 21:03:16 +02:00
|
|
|
$joins = implode(' ', $joins);
|
|
|
|
|
|
|
|
return $joins;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* @task internal
|
|
|
|
*/
|
|
|
|
private function buildWhereClause($conn_r) {
|
|
|
|
$where = array();
|
|
|
|
|
|
|
|
if ($this->pathIDs) {
|
|
|
|
$path_clauses = array();
|
|
|
|
$repo_info = igroup($this->pathIDs, 'repositoryID');
|
|
|
|
foreach ($repo_info as $repository_id => $paths) {
|
|
|
|
$path_clauses[] = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'(repositoryID = %d AND pathID IN (%Ld))',
|
|
|
|
$repository_id,
|
|
|
|
ipull($paths, 'pathID'));
|
|
|
|
}
|
|
|
|
$path_clauses = '('.implode(' OR ', $path_clauses).')';
|
|
|
|
$where[] = $path_clauses;
|
|
|
|
}
|
|
|
|
|
2011-12-06 23:21:36 +01:00
|
|
|
if ($this->authors) {
|
2011-12-02 01:57:06 +01:00
|
|
|
$where[] = qsprintf(
|
|
|
|
$conn_r,
|
2011-12-06 23:21:36 +01:00
|
|
|
'authorPHID IN (%Ls)',
|
|
|
|
$this->authors);
|
2011-12-02 01:57:06 +01:00
|
|
|
}
|
|
|
|
|
|
|
|
if ($this->revIDs) {
|
|
|
|
$where[] = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'id IN (%Ld)',
|
|
|
|
$this->revIDs);
|
|
|
|
}
|
|
|
|
|
2012-01-04 06:08:12 +01:00
|
|
|
if ($this->commitHashes) {
|
|
|
|
$hash_clauses = array();
|
|
|
|
foreach ($this->commitHashes as $info) {
|
|
|
|
list($type, $hash) = $info;
|
|
|
|
$hash_clauses[] = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'(hash_rel.type = %s AND hash_rel.hash = %s)',
|
|
|
|
$type,
|
|
|
|
$hash);
|
|
|
|
}
|
|
|
|
$hash_clauses = '('.implode(' OR ', $hash_clauses).')';
|
|
|
|
$where[] = $hash_clauses;
|
|
|
|
}
|
|
|
|
|
2011-12-02 01:57:06 +01:00
|
|
|
if ($this->phids) {
|
|
|
|
$where[] = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'phid IN (%Ls)',
|
|
|
|
$this->phids);
|
|
|
|
}
|
|
|
|
|
2011-12-07 17:30:49 +01:00
|
|
|
if ($this->responsibles) {
|
|
|
|
$where[] = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'(responsibles_rel.objectPHID IS NOT NULL OR r.authorPHID IN (%Ls))',
|
|
|
|
$this->responsibles);
|
|
|
|
}
|
|
|
|
|
2011-10-02 21:03:16 +02:00
|
|
|
switch ($this->status) {
|
|
|
|
case self::STATUS_ANY:
|
|
|
|
break;
|
|
|
|
case self::STATUS_OPEN:
|
|
|
|
$where[] = qsprintf(
|
|
|
|
$conn_r,
|
|
|
|
'status IN (%Ld)',
|
|
|
|
array(
|
2012-01-10 20:39:11 +01:00
|
|
|
ArcanistDifferentialRevisionStatus::NEEDS_REVIEW,
|
|
|
|
ArcanistDifferentialRevisionStatus::NEEDS_REVISION,
|
|
|
|
ArcanistDifferentialRevisionStatus::ACCEPTED,
|
2011-10-02 21:03:16 +02:00
|
|
|
));
|
|
|
|
break;
|
|
|
|
default:
|
|
|
|
throw new Exception(
|
|
|
|
"Unknown revision status filter constant '{$this->status}'!");
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($where) {
|
|
|
|
$where = 'WHERE '.implode(' AND ', $where);
|
|
|
|
} else {
|
|
|
|
$where = '';
|
|
|
|
}
|
|
|
|
|
|
|
|
return $where;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* @task internal
|
|
|
|
*/
|
|
|
|
private function buildGroupByClause($conn_r) {
|
2011-12-07 17:30:49 +01:00
|
|
|
$join_triggers = array_merge(
|
|
|
|
$this->pathIDs,
|
|
|
|
$this->ccs,
|
|
|
|
$this->reviewers,
|
|
|
|
$this->subscribers,
|
|
|
|
$this->responsibles);
|
|
|
|
|
|
|
|
$needs_distinct = (count($join_triggers) > 1);
|
2011-10-02 21:03:16 +02:00
|
|
|
|
|
|
|
if ($needs_distinct) {
|
|
|
|
return 'GROUP BY r.id';
|
|
|
|
} else {
|
|
|
|
return '';
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* @task internal
|
|
|
|
*/
|
|
|
|
private function buildOrderByClause($conn_r) {
|
|
|
|
switch ($this->order) {
|
|
|
|
case self::ORDER_MODIFIED:
|
|
|
|
return 'ORDER BY r.dateModified DESC';
|
|
|
|
case self::ORDER_CREATED:
|
|
|
|
return 'ORDER BY r.dateCreated DESC';
|
|
|
|
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';
|
|
|
|
default:
|
|
|
|
throw new Exception("Unknown query order constant '{$this->order}'.");
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2011-12-21 02:05:52 +01:00
|
|
|
private function loadRelationships($conn_r, array $revisions) {
|
|
|
|
$relationships = queryfx_all(
|
|
|
|
$conn_r,
|
|
|
|
'SELECT * FROM %T WHERE revisionID in (%Ld) ORDER BY sequence',
|
|
|
|
DifferentialRevision::RELATIONSHIP_TABLE,
|
|
|
|
mpull($revisions, 'getID'));
|
|
|
|
$relationships = igroup($relationships, 'revisionID');
|
|
|
|
foreach ($revisions as $revision) {
|
|
|
|
$revision->attachRelationships(
|
|
|
|
idx(
|
|
|
|
$relationships,
|
|
|
|
$revision->getID(),
|
|
|
|
array()));
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
private function loadCommitPHIDs($conn_r, array $revisions) {
|
|
|
|
$commit_phids = queryfx_all(
|
|
|
|
$conn_r,
|
|
|
|
'SELECT * FROM %T WHERE revisionID IN (%Ld)',
|
|
|
|
DifferentialRevision::TABLE_COMMIT,
|
|
|
|
mpull($revisions, 'getID'));
|
|
|
|
$commit_phids = igroup($commit_phids, 'revisionID');
|
|
|
|
foreach ($revisions as $revision) {
|
|
|
|
$phids = idx($commit_phids, $revision->getID(), array());
|
|
|
|
$phids = ipull($phids, 'commitPHID');
|
|
|
|
$revision->attachCommitPHIDs($phids);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
private function loadDiffIDs($conn_r, array $revisions) {
|
|
|
|
$diff_table = new DifferentialDiff();
|
|
|
|
|
|
|
|
$diff_ids = queryfx_all(
|
|
|
|
$conn_r,
|
|
|
|
'SELECT revisionID, id FROM %T WHERE revisionID IN (%Ld)
|
|
|
|
ORDER BY id DESC',
|
|
|
|
$diff_table->getTableName(),
|
|
|
|
mpull($revisions, 'getID'));
|
|
|
|
$diff_ids = igroup($diff_ids, 'revisionID');
|
|
|
|
|
|
|
|
foreach ($revisions as $revision) {
|
|
|
|
$ids = idx($diff_ids, $revision->getID(), array());
|
|
|
|
$ids = ipull($ids, 'id');
|
|
|
|
$revision->attachDiffIDs($ids);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
private function loadActiveDiffs($conn_r, array $revisions) {
|
|
|
|
$diff_table = new DifferentialDiff();
|
|
|
|
|
|
|
|
$load_ids = array();
|
|
|
|
foreach ($revisions as $revision) {
|
|
|
|
$diffs = $revision->getDiffIDs();
|
|
|
|
if ($diffs) {
|
|
|
|
$load_ids[] = max($diffs);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
$active_diffs = array();
|
|
|
|
if ($load_ids) {
|
|
|
|
$active_diffs = $diff_table->loadAllWhere(
|
|
|
|
'id IN (%Ld)',
|
|
|
|
$load_ids);
|
|
|
|
}
|
|
|
|
|
|
|
|
$active_diffs = mpull($active_diffs, null, 'getRevisionID');
|
|
|
|
foreach ($revisions as $revision) {
|
|
|
|
$revision->attachActiveDiff(idx($active_diffs, $revision->getID()));
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2011-10-02 21:03:16 +02:00
|
|
|
|
|
|
|
}
|