Page MenuHomePhabricator

Convert complex query subclasses to use internal cursors
ClosedPublic

Authored by epriestley on Mar 18 2019, 9:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 20, 4:40 PM
Unknown Object (File)
Wed, Apr 3, 6:43 PM
Unknown Object (File)
Wed, Apr 3, 6:43 PM
Unknown Object (File)
Wed, Apr 3, 6:42 PM
Unknown Object (File)
Wed, Apr 3, 6:42 PM
Unknown Object (File)
Sun, Mar 31, 6:59 PM
Unknown Object (File)
Fri, Mar 29, 12:29 AM
Unknown Object (File)
Feb 21 2024, 4:38 PM
Subscribers
None

Details

Summary

Depends on D20292. Ref T13259. This converts the rest of the getPagingValueMap() callsites to operate on internal cursors instead.

These are pretty one-off for the most part, so I'll annotate them inline.

Test Plan
  • Grouped tasks by project, sorted by title, paged through them, saw consistent outcomes.
  • Queried edges with "edge.search", paged through them using the "after" cursor.
  • Poked around the other stuff without catching any brokenness.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

src/applications/almanac/query/AlmanacInterfaceQuery.php
199–201

In this case, we're paging based on a table we JOIN, so we can just include the column we care about in the SELECT and then read the raw row value.

Pretty sure nothing in the UI actually uses this today: I don't think you can execute an interface query by name with a limit and page through it? Maybe in a typeahead? I'll go double check that.

src/applications/feed/query/PhabricatorFeedQuery.php
150–165

This one isn't too bad:

  • Feed stories have no "ID", use "chronologicalKey" (basically a 64-bit ID) instead.
  • They return a raw row instead of an object since they're super old. This is fine, just slightly odd.

Tested this by paging through feed locally, seemed to work properly.

src/applications/maniphest/query/ManiphestTaskQuery.php
30

Maniphest is the biggest mess by far.

You can "Group By: Project" which allows each result to appear more than once in the result set. If task T123 is in projects "A" and "B", you'll get back two copies of T123 in the results.

We should maybe just remove this (it predates workboards by several years, although they aren't really the same thing), but I was able to keep it working without more than ~50 lines of gross disgusting mess.

src/applications/repository/query/PhabricatorRepositoryQuery.php
494

(This is redundant, we automatically load all the columns from the primary table. Query ends up looking like SELECT r.*, r.*, ...).

src/infrastructure/edges/query/PhabricatorEdgeObjectQuery.php
141–151

This is only used by edge.search.

Edges have no IDs, so we're faking it. This isn't too awful. Some day, maybe they should have IDs.

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
332

This could have been removed earlier, I got rid of the only callsite near the beginning of this change sequence.

src/applications/almanac/query/AlmanacInterfaceQuery.php
199–201

I changed the modular datasource limit from 100 to 3, then browsed interfaces in the "Bind Interface" UI from a Service. Worked fine.

I got the gist of this, but the Maniphest stuff is pretty black magic. I'll trust in your test plan.

This revision is now accepted and ready to land.Mar 18 2019, 10:28 PM
This revision was automatically updated to reflect the committed changes.