fix: Add implicit orderby to certain queries #12674
Conversation
bryanck
changed the title
bryanck
force-pushed
the
implicit-orderby
branch
from
f897088 to
ae10fdd
Compare
Codecov Report
@@ Coverage Diff @@
## master #12674 +/- ##
==========================================
- Coverage 66.85% 66.64% -0.21%
==========================================
Files 1018 1018
Lines 49776 49800 +24
Branches 4869 4877 +8
==========================================
- Hits 33278 33191 -87
- Misses 16375 16482 +107
- Partials 123 127 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
junlincc
added
viz:charts:line
| for query in self._query_context.queries: | ||
| if query.row_limit and query.metrics and not query.orderby: | ||
| query.orderby = [(query.metrics[0], False)] |
There was a problem hiding this comment.
While I understand the use case and agree with it from an end user perspective, always adding orderbys to queries can have a significant performance impact on queries referencing large tables. Going forward we want to make less implicit assumptions about ordering and let the viz' explicitly request ordering if it's needed. Therefore I feel this logic should be moved to the individual plugins rather than apply it on all queries.
There was a problem hiding this comment.
That sounds reasonable, however the behavior of returning unordered results in these cases seems to be a regression, as this wasn't happening in v0.37. Perhaps the viz components changed?
There was a problem hiding this comment.
If no one else has a problem with this, you can go ahead and close it and we'll patch it on our side for now, until the components are updated (I'll look into that).
There was a problem hiding this comment.
IIRC, word cloud was using the legacy API pre 0.37. Maybe that's why?
I added a “sort by metric” option for Sankey not long ago (disabled by default), maybe you can take a similar approach?
There was a problem hiding this comment.
@bryanck originally this regression was caused by #11153 which removed the implicit orderby from chart data requests. I agree with the fix on a chart-by-chart basis, and definitely think it makes sense on word cloud. The problem with implicitly assuming an orderby clause is that it will cause unnecessary query overhead in cases where it is potentially not required, and sometimes cause unwanted side-effects. One side-effect that was noticed by this implicit ordering was on timeseries charts, where ordering by metric caused "scanning" from top to bottom if the limit was reached (=the observations with the lowest metric values didn't get through). In the case of a timeseries chart it feels more natural to order the results by series, not metrics, so as to make sure the request contains as many full series as possible.
Again I'd urge adding this fix to each chart separately, and also adding a sort control to charts where one might be needed.
5 participants
SUMMARY
This PR adds implicit ordering for the line chart and the word cloud chart. Currently, if a line chart has a group by, the series is limited in number, and there is no sort specified, the series selected will be arbitrary rather than the top series based on the metric. Similarly, the word cloud chart, if limited, will select an arbitrary set of results, rather than the top results based on the metric.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
Create a line chart with group by, limit the series number, and the top N series should be shown. Similarly with the word cloud chart, apply a limit, and the top results should be displayed rather than an arbitrary set.
ADDITIONAL INFORMATION