feat: Add sort by metric for charts with multiple metrics #13057
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13057 +/- ##
==========================================
+ Coverage 53.06% 61.85% +8.79%
==========================================
Files 489 981 +492
Lines 17314 46368 +29054
Branches 4482 4505 +23
==========================================
+ Hits 9187 28680 +19493
- Misses 8127 17688 +9561
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -865,6 +865,12 @@ def query_obj(self) -> QueryObjectDict: | |||
| raise QueryObjectValidationError(_("Please choose at least one metric")) | |||
| if set(groupby) & set(columns): | |||
| raise QueryObjectValidationError(_("Group By' and 'Columns' can't overlap")) | |||
| sort_by = self.form_data.get("timeseries_limit_metric") | |||
There was a problem hiding this comment.
I am curious, why use timeseries_limit_metric control in a not time-series chart.
There was a problem hiding this comment.
I think this is just from legacy form_data field names. We'd need a db migration if we want to rename it.
Superset has a timeseries-first UI. Each pivot group can be considered a series since the base query has the ability to aggregate by time + pivot column.
There was a problem hiding this comment.
Wait a minute, since this is a new field for pivot table and apache-superset/superset-ui#952 hasn't been merged, maybe we CAN rename it to something else?
There was a problem hiding this comment.
Or we can keep it as timeseries_limit_metric to be able to reuse the existing code and do a wholesale migration later.
There was a problem hiding this comment.
I was super confused by the name yesterday, and agree we need to get rid of it. I'm ok both ways, but I think it might be simpler to do a bulk rename later to avoid partially migrated names.
superset/viz.py
Outdated
| sort_by_label = utils.get_metric_name(sort_by) | ||
| if sort_by_label not in d["metrics"]: | ||
| d["metrics"].append(sort_by) | ||
| d["orderby"] = [(sort_by, not self.form_data.get("order_desc", True))] |
There was a problem hiding this comment.
it is may be more readable
| d["orderby"] = [(sort_by, not self.form_data.get("order_desc", True))] | |
| d["orderby"] = [(sort_by, bool(self.form_data.get("order_desc"))] |
There was a problem hiding this comment.
This is incorrect. The second parameter in the orderby tuple is "is_ascending", which is the opposite meaning of order_desc. However, we also want the default to be order in descending in case order_desc is not set because it's the most sensible default when a sort by metric is specified.
There was a problem hiding this comment.
thanks for the explanation, it's my fault
maloun96
changed the title
…heckbox is not selected
|
in this commit 61349b9 |
|
@villebro those changes are relatively low risk, instead of pulling each PR, I will take a look once they are on master all at once. 🟢 |
mistercrunch
added
🏷️ bot
SUMMARY
Add sort by metric for pivot-table
Associated with: apache-superset/superset-ui#952
pivot-table
parallel-coordinates
chart-rose
partition
treemap
nvd3-area
horizon
TEST PLAN
Select pivot table
ADDITIONAL INFORMATION