Page MenuHomePhabricator
Create Task
Maniphest T13379

Query panels executing bucketed queries with limits have unclear behavior
Closed, WontfixPublic
Actions

Description

See PHI1383.

When a query panel on a dashboard has a configured limit and executes a bucketed query, like the "Active Revisions" query in Differential, the result can be counterintuitive.

PHI1383 was reported against an old version of Phabricator and changes in D20333 and connected to T13272 have generally improved behavior here, but applying a limit to these queries does not really result in intuitive behavior, since the limit means a sort of arbitrary set of revisions are shown. For example, a limit of 10 does not mean "show only the first 10 revisions", and because the bucketing/ordering is dependent on application-level behavior this theoretical limit can not be applied in the database anyway. It's somewhat unlikely that this limit is ever relevant/desirable anyway.

Here's what the panel looks like with a limit of 2:

Screen Shot 2019-08-14 at 2.26.16 PM.png (289×959 px, 38 KB)

I think the cleanest fix here is likely to just ignore the limit on bucketing queries for now (which is what we do inside applications, so it makes behavior consistent if nothing else), and then maybe revisit this somewhere down the line.

Related Objects

Event Timeline

epriestley triaged this task as Low priority.Aug 15 2019, 7:21 PM
epriestley created this task.
Herald added a subscriber: amckinley. · View Herald TranscriptAug 15 2019, 7:21 PM
epriestley added a comment.Aug 15 2019, 7:43 PM

The bucket limit is currently implemented like this:

public function getPageSize(PhabricatorSavedQuery $saved) {
  $bucket = $this->getResultBucket($saved);

  $limit = (int)$saved->getParameter('limit');

  if ($limit > 0) {
    if ($bucket) {
      $bucket->setPageSize($limit);
    }
    return $limit;
  }

  if ($bucket) {
    return $bucket->getPageSize();
  }

  return 100;
}

I could just make the bucket part stronger than the configured part.

This will produce the right behavior in Diffusion and Differential, and also produce the right behavior in Query dashboard panels, resolving the root issue.

But this will also make running bucketed queries via the API override the page size. The behavior of bucketing-via-API is currently pretty bizarre anyway (the "bucket" constraint is not supported, but you can use a prebuilt query and get results back without any bucketing information). This is probably wrong, but I'm hesitant to change the behavior purely as collateral damage.

So I'm maybe going to make a narrower change, and have query panels ignore the "Limit" field if they are executing a bucketed query. This is probably not the best possible long-term approach for handling bucketing, but narrowly resolves the root issue here with no collateral damage.

epriestley closed this task as Wontfix.Aug 15 2019, 7:47 PM

Actually, since the modern behavior is reasonable by default (the "Limit" field is not required, and defaults to empty, and the panel works properly if "Limit" is left blank), users have to explicitly set a limit to get any unintuitive behavior here. If they do, they still get a "View All Results" hint.

The actual limiting behavior is perhaps unintuitive but I think I'm going to deal with the broader issue by doing nothing and assuming you know what you're doing if you explicitly configure a "Limit".

Phabricator · Built by Phacility