-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
[bugfix] Fix Tableviz metrics column disorder #5566
[bugfix] Fix Tableviz metrics column disorder #5566
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5566 +/- ##
=======================================
Coverage 63.82% 63.82%
=======================================
Files 364 364
Lines 23059 23059
Branches 2568 2568
=======================================
Hits 14717 14717
Misses 8327 8327
Partials 15 15
Continue to review full report at Codecov.
|
Thanks for the fix! |
superset/viz.py
Outdated
@@ -95,19 +95,21 @@ def __init__(self, datasource, form_data, force=False): | |||
self.process_metrics() | |||
|
|||
def process_metrics(self): | |||
self.metric_dict = {} |
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.
Can't we just use an OrderedDict
here? https://docs.python.org/2/library/collections.html#collections.OrderedDict
5801ead
to
422b87a
Compare
422b87a
to
d5e283b
Compare
d5e283b
to
d365e01
Compare
@mistercrunch Any idea on a timeline for this PR? This is blocking our users and I hate having to build containers for things if I can just get 'em for free :) |
Merging this because of the unit test, but looks like the fix already made it into master. |
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.
Merging this because of the unit test, but looks like the fix already made it into master.
thanks! the fix already merged into master by mistercrunch in another PR
superset/viz.py
Outdated
self.metric_dict[self.get_metric_label(o)] = o | ||
|
||
# Cast to list needed to return serializable object in py3 | ||
self.all_metrics = list(self.metric_dict.values()) |
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.
@mistercrunch this is the cause of metrics column disorder in TableViz.
It tries to use self.metric_dict
and then converts to list here. I assume that it's going to remove duplicate metrics, since some metrics appear more than once in total which locate in different METRIC_KEYS
.
superset/viz.py
Outdated
# avoid generating duplicate metrics getting from METRIC_KEYS | ||
if self.get_metric_label(o) not in self.metric_labels: | ||
self.metric_labels.append(self.get_metric_label(o)) | ||
self.all_metrics.append(o) |
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.
@mistercrunch so the best way to avoid duplicate metrics and keep its original order is using list
at the beginning.
tests/viz_tests.py
Outdated
u'optionName': u'metric_hww53ilkph_jo8b2fyt8rs'}, | ||
u'SUM(SP_URB_TOTL)', | ||
u'count'] | ||
self.assertEqual(test_viz.all_metrics, expect_all_metrics) |
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.
now the labels and metrics order is correct.
superset/viz.py
Outdated
self.metric_dict = {} | ||
# metrics in TableViz is order sensitive, so metric_dict should be | ||
# OrderedDict | ||
self.metric_dict = OrderedDict() |
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.
@mistercrunch I have updated this self.metric_dict
to OrderedDict()
now. Please help to merge this PR.
* fix metrics disorder * add tableviz metric order tests * lint code * use OrderedDict to avoid metrics disorder * fix unit test
pivot_table can not order |
Fixes #5373
The cause of metrics disorder in Table View is located in function
BaseViz. process_metrics
.This function was introduced from https://github.com/apache/incubator-superset/pull/4914/files
This fix also works for percentage metrics.