Make the Differentials filter by "Responsible Users = Project" more efficient.
Closes T16055
Details
Details
- Reviewers
jmeador aklapper - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T16055: DifferentialRevisionQuery: avoid nonsense query comparing r.authorPHID with group PHIDs
- visit Differentials
- search by Responsible Users = Group, and anything else
Inspect the database queries with the DarkConsole (that can be enabled from your personal Settings > Developer Settings).
Resulting query does not contain anymore a query against differential_revision with this nonsense condition:
r.authorPHID IN ('PHID-PROJ-plufiz5pclartkuv5ggz')The filter still works returning the same results.
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Branch
- arcpatch-D25676_3
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 1930 Build 1930: arc lint + arc unit
Event Timeline
Comment Actions
@jmeador Hi, what would be the Test Plan to get a "resulting query"? Going to /differential/query/advanced/, selecting the magnifier button for the Responsible Users field, and checking that after applying the patch only user accounts are listed, and no more projects or packages, I assume?
Comment Actions
More specifically, examining DarkConsole service logs, you would notice a change in the query to differential_revisions:
Prior to the patch, the query would contain authorPHID in (PHID-USER-xxx, PHID-PROJ-xxxx) in the first part of the query, assuming the responsible user is a member of at least one Project. After the patch, the values list would only contain PHID-USER-xxx, eliminating project/owner packages from the query.
Comment Actions
Note that the responsible users' projects and packages should absolutely still be included in the reviewers query join (line 487 right-side). This is why $this->responsibles remains unmodified.
Comment Actions
Ah, thanks, makes sense now. I'd probably rephrase "but harms the authors constraint" a little bit to explicitly cover what you explained well in D25676#18549 - an author PHID in $this->responsibles might not always be a member of a project or owner of a package, so the query results were incomplete beforehand.
Also, I think defining $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; outside of the foreach loop would be slightly more performant?
Comment Actions
Uh there's also phid_group_by_type($type) for extra readability and less code. Also maybe more modular for future things.
| src/applications/differential/query/DifferentialRevisionQuery.php | ||
|---|---|---|
| 470 | This MAY need the idx trick: $authors_by_type = phid_group_by_type($this->responsibles); $responsible_authors = idx($authors_by_type, PhabricatorPeopleUserPHIDType::TYPECONST); Or something similar (sorry I'm from mobile) To do not cause an error if that array key is not available for unkown reasons | |
Comment Actions
Ah...I may take another look (jmeador: Feel of course also very free to commandeer this back to you)
Comment Actions
Thanks for this patch that unfortunately it's revealing even more weird legacy ghosts.
If I've understand correctly the proposed test plan should be this:
- visit Differentials
- search by Responsible Users = Group
This is the resulting query in origin/master:
(SELECT `r`.* FROM `differential_revision` r WHERE (r.authorPHID IN ('PHID-PROJ-plufiz5pclartkuv5ggz')) ORDER BY `r`.`id` DESC LIMIT 101) UNION DISTINCT (SELECT `r`.* FROM `differential_revision` r LEFT JOIN `differential_reviewer` reviewer ON reviewer.revisionPHID = r.phid AND reviewer.reviewerStatus != 'resigned' AND reviewer.reviewerPHID in ('PHID-PROJ-plufiz5pclartkuv5ggz') WHERE ((reviewer.reviewerPHID IS NOT NULL)) ORDER BY `r`.`id` DESC LIMIT 101) ORDER BY `id` DESC LIMIT 101
After the change, this would be executed instead (with a small fix with idx as suggested). Look at the new query ( 🤯), because it's even more semantically wrong:
(SELECT `r`.* FROM `differential_revision` r ORDER BY `r`.`id` DESC LIMIT 101) UNION DISTINCT (SELECT `r`.* FROM `differential_revision` r LEFT JOIN `differential_reviewer` reviewer ON reviewer.revisionPHID = r.phid AND reviewer.reviewerStatus != 'resigned' AND reviewer.reviewerPHID in ('PHID-PROJ-plufiz5pclartkuv5ggz') WHERE ((reviewer.reviewerPHID IS NOT NULL)) ORDER BY `r`.`id` DESC LIMIT 101) ORDER BY `id` DESC LIMIT 101
Let's make the query more evident. Before, it was this nonsense pseudo-query:
( 🐱 (SELECT revisions WHERE **nonsense** limit 101) + 🧸 (SELECT revisions WHERE reviewer is a member of the project limit 101) ) limit 101 → results: first 101 🧸
→ so in origin/master the results are just from query 2, the members of the project
After the change, we have this instead:
( 🐱 (SELECT all revisions limit 101) + 🧸 (SELECT revisions WHERE reviewer is a member of the project limit 101) ) limit 101 → results: first 101 🐱
→ 🔴 So, after the proposed change, the search filter does nothing anymore, just returning all revisions.
I will dig a bit more.
