-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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: take form_data reference for metrics for pivot_v2 table reports #21029
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21029 +/- ##
==========================================
+ Coverage 66.35% 66.45% +0.09%
==========================================
Files 1767 1767
Lines 67356 67553 +197
Branches 7147 7147
==========================================
+ Hits 44694 44892 +198
+ Misses 20834 20833 -1
Partials 1828 1828
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 you add a test to https://github.com/apache/superset/blob/master/tests/unit_tests/charts/test_post_processing.py? We have some cool unit tests for the post processing. :)
Co-authored-by: Beto Dealmeida <[email protected]>
…into fix-pivot-reports
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.
Awesome!
assert ( | ||
"COUNT(*)" | ||
in apply_post_process(result, form_data, datasource=sqla_table)["queries"][0][ | ||
"data" | ||
].keys() | ||
) |
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: I would make this assertion more generic, since it might be useful to capture future regressions in other parts of the code. Something like:
assert apply_post_process(result, form_data, datasource=sqla_table) = {
"queries": [
{
"data": { "COUNT(*)": ..., ... },
},
]
)
SUMMARY
When executing pivot v2 table report if the
verbose_map
doesn't match up with the df columns we are throwing this error for metric/values. To fix this for pivot v2 table we won't be using the verbose map to change the values/metrics names to have better alignment.Click here to view exception in pandas
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION