-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
fix: echarts timeseries groupby #11103
Conversation
@@ -718,6 +718,7 @@ class ChartDataQueryObjectSchema(Schema): | |||
) | |||
groupby = fields.List( | |||
fields.String(description="Columns by which to group the query.",), | |||
allow_none=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QueryObject
automatically does self.groupby = groupby or []
in the constructor, so no need to require a fields.List
.
superset/charts/schemas.py
Outdated
@@ -782,7 +783,7 @@ class ChartDataQueryObjectSchema(Schema): | |||
description="Reverse order. Default: `false`", required=False | |||
) | |||
extras = fields.Nested(ChartDataExtrasSchema, required=False) | |||
columns = fields.List(fields.String(), description="",) | |||
columns = fields.List(fields.String(), description="", allow_none=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for columns
as groupby
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit could be nice to add a description to columns
and extras
if len(aggregates) == 1 and len(column) > 1: | ||
# drop aggregate for single aggregate pivots with multiple groupings | ||
# from column name (aggregates always come first in column name) | ||
column = column[1:] | ||
return ", ".join(column) | ||
return ", ".join([str(col) for col in column]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a shame we have to format everything as strings for now, but hoping to remedy this once we move on to some other serialization format than JSON.
Codecov Report
@@ Coverage Diff @@
## master #11103 +/- ##
==========================================
- Coverage 61.36% 60.52% -0.84%
==========================================
Files 383 383
Lines 24188 24186 -2
==========================================
- Hits 14842 14638 -204
- Misses 9346 9548 +202
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* fix: echarts timeseries groupby * address review comment
SUMMARY
Bump ECharts viz plugin to
0.15.4
to fix a bug affecting the groupby control (apache-superset/superset-ui#800). Also add support for pivoting non-string column values.TEST PLAN
Local testing + CI + new tests
ADDITIONAL INFORMATION