villebro commented on a change in pull request #12674:
URL: https://github.com/apache/superset/pull/12674#discussion_r563465840
##########
File path: superset/charts/commands/data.py
##########
@@ -82,6 +82,10 @@ def set_query_context(self, form_data: Dict[str, Any]) ->
QueryContext:
except ValidationError as error:
raise error
+ for query in self._query_context.queries:
+ if query.row_limit and query.metrics and not query.orderby:
+ query.orderby = [(query.metrics[0], False)]
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
