Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Included LIMIT, SLIMIT and ORDER BY time for influxdb #6065 #7735

Merged
merged 4 commits into from Mar 9, 2017

Conversation

thuck
Copy link
Contributor

@thuck thuck commented Mar 5, 2017

Fixes #6065

@thuck thuck changed the title Included LIMIT, SLIMIT and ORDER BY time for infludb #6065 Included LIMIT, SLIMIT and ORDER BY time for influxdb #6065 Mar 5, 2017
@thuck
Copy link
Contributor Author

thuck commented Mar 6, 2017

This should also close #7232

@daniellee daniellee self-assigned this Mar 8, 2017
@daniellee daniellee added this to the 4.3.0 milestone Mar 8, 2017
@daniellee
Copy link
Contributor

Feedback:

  • For a table panel, the default sorting on the Time column is descending so the order by clause has no effect unless you disable the table sorting. It does work well together with the LIMIT if you want to get the top x values. What's the use case for order by? Is the default table panel sorting a problem for the average Influxdb user?
  • LIMIT and SLIMIT seem to work fine
  • The query editor changes look fine except for one thing. Can you change the ORDER BY time to look the same as the group by. The label would be ORDER BY and time would be a non-editable query part. Like the image below but change group by to order by.

image

@thuck
Copy link
Contributor Author

thuck commented Mar 8, 2017

The order by time is applied in the query before the LIMIT, so the effect is basically X first points or X last points in time.So if you don't have this implemented LIMIT will only apply for the X first points since the default for influx is ASC, so if you want the most recent data to be used with LIMIT you must use order by time desc.
Sure; I'll check the ORDER BY part on the web interface.

@daniellee
Copy link
Contributor

If you fix the ux for the ORDER BY then I think this is good to merge. Nice work!

@thuck
Copy link
Contributor Author

thuck commented Mar 8, 2017

It should be fine now, let me know if there is any other changed necessary.

@daniellee daniellee merged commit 2f5814d into grafana:master Mar 9, 2017
@daniellee
Copy link
Contributor

Thanks again @thuck Fourth PR this week! Great to get TOPN functionality for InfluxDB in Grafana.

@torkelo
Copy link
Member

torkelo commented May 16, 2017

inclined to revert this change. Just messes up the editor so much with options that you don't use 99.99% of the time. Maybe make the raw query editor mode better would make more sense?

@thuck
Copy link
Contributor Author

thuck commented May 16, 2017

@torkelo Maybe you can keep it as it is, considering that influxdb will change their query language again not sure if put more effort on the current one is a good investment of time.
Maybe this version of the language should be put in "maintenance" and no more changes that are not bug fixes should be made?
Also I don't think it messes a lot of with the editor, but I get you point of reducing the visual "clunk" of the visual editor.

@ying-jeanne ying-jeanne added the pr/external This PR is from external contributor label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/datasource datasource/InfluxDB pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

influxdb query builder support for ORDER BY and LIMIT
5 participants