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

Allowing to set the sort order during query with "group by" #3439

Merged
merged 1 commit into from
Sep 12, 2017

Conversation

fabianmenges
Copy link
Contributor

If you run a "group by" query with a "series limit" you can specify the "sort by" metric but not the sort order. In the following example you can see that you get the top 5 names. There is currently no way to look at the bottom 5 names. (sort order is hard coded to desc).
group_by

This change includes the backend changes to add this functionality. I'm happy to help on the front end as well.

@coveralls
Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage increased (+0.004%) to 69.121% when pulling baaa4a1e330ec628db05786beea50bc838b4edb9 on tc-dc:fmenges/order_direction into 3dfdde1 on apache:master.

@mistercrunch
Copy link
Member

LGTM but please rebase (fixed the build on master)

@coveralls
Copy link

coveralls commented Sep 12, 2017

Coverage Status

Coverage increased (+0.004%) to 69.121% when pulling 640c953 on tc-dc:fmenges/order_direction into 147c12d on apache:master.

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing control?

@@ -137,6 +137,9 @@ def query_obj(self):
row_limit = int(
form_data.get("row_limit") or config.get("ROW_LIMIT"))

# default order direction
order_desc = form_data.get("order_desc", True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there's a control (in controls.jsx) missing in this PR (?)

@mistercrunch mistercrunch merged commit 3c0e85e into apache:master Sep 12, 2017
@mistercrunch
Copy link
Member

I'm actually working on something related, merged to build around this.

@fabianmenges
Copy link
Contributor Author

K, sounds good.

@fabianmenges fabianmenges deleted the fmenges/order_direction branch September 19, 2017 18:33
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.20.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.20.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants