Page MenuHomePhabricator
Differential D15560

Improve task dependency queries (Fixes T8126)
AbandonedPublic
Actions

Authored by epriestley on Apr 1 2016, 8:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 17, 4:36 AM
Unknown Object (File)
Sun, Dec 8, 6:49 PM
Unknown Object (File)
Sun, Dec 8, 2:10 AM
Unknown Object (File)
Thu, Dec 5, 3:28 PM
Unknown Object (File)
Sun, Dec 1, 7:40 AM
Unknown Object (File)
Sat, Nov 30, 1:24 PM
Unknown Object (File)
Wed, Nov 27, 10:48 AM
Unknown Object (File)
Sat, Nov 23, 3:17 PM
View All Files
Subscribers
20after4
chad
epriestley

Details

Reviewers
isfs
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T8126: Issue with using the filter "Blocked: Show Tasks Not Blocked By Other Tasks" in Queries
Required Signatures
L28 Phacility Individual Contributor License Agreement
Summary

To fix T8126 this filters blocking/blocked tasks to include only open tasks at join time rather than after joining, then counts the number of such tasks after grouping.

This should be about as performant as the current implementation (with the benefit of also working), however, I doubt the MySQL optimiser is smart enough to realise we're only counting tasks and comparing with zero and it therefore can stop after finding the first open blocking/blocked task. Consequently, refactoring this to use EXISTS subqueries with a LIMIT clause may give a performance boost. Let me know if you'd like me to attempt that.

And of course, let me know any other direction you'd like me to go with this.

Test Plan
  • Created four tasks, T1 to T4.
  • Added T2 and T3 as blocking tasks for T1.
  • Created Manifest queries Blocking, Non-Blocking, Blocked and Unblocked.
  • Closed T1. Checked queries. Reopened T1.
  • Closed T2. Checked queries.
  • Closed T3. Checked queries.
  • Queries always returned expected results. Tasks listed iff they had zero/greater-than-zero open blocking/blocked tasks.

Diff Detail

Repository
rP Phabricator
Branch
stable
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 11378
Build 16866: Run Core Tests
Build 14168: arc lint + arc unit

Event Timeline

isfs updated this revision to Diff 37513.Apr 1 2016, 8:13 AM
isfs retitled this revision from to Improve task dependency queries (Fixes T8126).
isfs updated this object.
isfs edited the test plan for this revision. (Show Details)
isfs added a task: T8126: Issue with using the filter "Blocked: Show Tasks Not Blocked By Other Tasks" in Queries.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptApr 1 2016, 8:13 AM
Herald added a required legal document: L28 Phacility Individual Contributor License Agreement. · View Herald Transcript
Herald added a subscriber: epriestley. · View Herald Transcript
isfs added a comment.Apr 6 2016, 9:56 PM

@epriestley, have I done the right thing submitting a review here for this? Could you spare a few minutes to accept/land it, or throw it back to me for further work? If I should be following a different procedure, please let me know. No pressure; just a gentle nudge and plea for guidance. Really appreciate all your work and respect your right to set your own priorities.

isfs added inline comments.Apr 6 2016, 10:39 PM
src/applications/maniphest/query/ManiphestTaskQuery.php
686

This was the original logic, but I'd like to query it.

E.g. if I choose to

  • Show blocked tasks
  • Hide blocking tasks

I would expect to see 'top level' tasks that have subtasks, but are not subtasks themselves. However, with this 'or' logic, I would actually see all tasks with subtasks as well as all tasks which aren't holding anything up.

I also believe conjunctive logic is more what the user expects from a query form. E.g. if I choose high priority tasks and list myself as assignee, I expect to see my high priority tasks, not all high priority tasks along with all tasks assigned to me.

I think this should be changed to 'AND', but perhaps for traceability it should be done in a separate change or have a separate task created.

isfs added a reviewer: epriestley.Jun 3 2016, 3:03 AM
isfs added a subscriber: chad.

@chad indicated that @epriestley reviews code from contributors, so adding him as a reviewer.

20after4 added a subscriber: 20after4.Jul 20 2016, 8:00 PM

This looks good to me, and I agree that it makes a lot more sense to use AND rather than OR when building the HAVING clause.

In case you're feeling discouraged by the silence here, I should reassure you that I'm fairly confident it's not due to anything you've done wrong here.

There has been some recent refactoring of the way tasks and other object relationships are handled, e.g. D16166: Split "Edit Blocking Tasks" into "Edit Parent Tasks" and "Edit Subtasks". That and related work might affect this code and I suspect that is why this hasn't been reviewed/merged yet.

epriestley mentioned this in T8126: Issue with using the filter "Blocked: Show Tasks Not Blocked By Other Tasks" in Queries.Jul 28 2016, 3:56 PM
epriestley commandeered this revision.Jul 28 2016, 6:05 PM
epriestley edited reviewers, added: isfs; removed: epriestley.

This should be obsoleted by D16340, which also updates the language.

I also made the AND change, and got away with slightly fewer HAVING clauses. Let me know if you're still seeing issues.

epriestley abandoned this revision.Jul 28 2016, 6:05 PM
isfs edited edge metadata.Jul 29 2016, 7:35 AM

Yeah, D16340 looks good to me and should perform similarly. I'll raise any issues when I upgrade, but I don't anticipate any problems. Thanks for sorting this out!

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
src/
applications/
maniphest/
query/
81 lines40%
Loading...

Diff 37513

View Options

src/applications/maniphest/query/ManiphestTaskQuery.php

Loading...
Phabricator · Built by Phacility