feat: reset metrics on dataset change

[pkdotson]

SUMMARY

This pr will reset metrics controls when there is a change in dataset.

closes apache-superset/superset-roadmap#112

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-01-28.at.3.01.34.PM.mov

TEST PLAN

change dataset and in explore view and check controls of various charts to ensure all controls are defaulted to empty state.

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[codecov-io]

Codecov Report

Merging #12782 (29a42e0) into master (fa8c492) will increase coverage by 2.53%.
The diff coverage is 59.25%.

@@            Coverage Diff             @@
##           master   #12782      +/-   ##
==========================================
+ Coverage   66.42%   68.95%   +2.53%     
==========================================
  Files        1022     1026       +4     
  Lines       50106    48856    -1250     
  Branches     5193     5257      +64     
==========================================
+ Hits        33283    33689     +406     
+ Misses      16680    15033    -1647     
+ Partials      143      134       -9     

Flag	Coverage Δ
cypress	50.83% <59.25%> (+0.35%)	⬆️
javascript	61.78% <48.14%> (+0.05%)	⬆️
python	67.32% <ø> (+3.25%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../explore/components/controls/CollectionControl.jsx	41.66% <0.00%> (+20.45%)	⬆️
...ntrols/MetricControl/AdhocMetricPopoverTrigger.tsx	94.73% <ø> (ø)
...s/controls/MetricControl/MetricDefinitionValue.jsx	94.11% <ø> (ø)
...mponents/controls/MetricControl/MetricsControl.jsx	90.11% <ø> (+4.15%)	⬆️
.../src/explore/components/ControlPanelsContainer.jsx	86.73% <37.50%> (-4.38%)	⬇️
.../controls/MetricControl/AdhocMetricEditPopover.jsx	79.85% <50.00%> (-0.77%)	⬇️
...nd/src/explore/components/controls/TextControl.tsx	68.75% <66.66%> (-0.30%)	⬇️
...nents/controls/MetricControl/AdhocMetricOption.jsx	100.00% <100.00%> (+25.00%)	⬆️
.../src/explore/components/controls/SelectControl.jsx	93.69% <100.00%> (+0.23%)	⬆️
superset/db_engines/hive.py	0.00% <0.00%> (-85.72%)	⬇️
... and 150 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa8c492...29a42e0. Read the comment docs.

[junlincc]

end state of this project

1. reset/clear/uncheck all query and customize control when dataset is changed
2. In the change dataset warning msg, add a line to notify users all the current control settings will be cleared by changing dataset.

[junlincc]

Tested nvd3, deck.gl, echarts on dropdown select, input, checkbox, time range(default last week), advanced analytics and annotation. all LGTM! ✅
ready for code review @rusackas @villebro @zhaoyongjie

[villebro]

For SelectControl, MetricControl etc this mostly makes sense, but I think this will have unwanted consequences for controls that default to non-empty/false. For example, Pie Chart defaults to true on the "Show labels" and "Put labels outside" controls, which would be false with this change. I think it would be optimal to rather reset to the default control values as defined by the selected viz.

[ktmud]

I think it would be nice to retain metrics/columns with matching column names at least since a lot of times users would want to fix a broken chart or iterate on an existing one by replacing the datasource with a newer (and slightly updated) dataset.

I know this requirement exists because as a former Data Scientist, I've done similar operation in Tableau a lot.

A better experience is probably to still keep the selected options, but highlight the invalid ones (like Tableau does with invalid calculated columns when you switch a datasource).

@junlincc could you elaborate on the motivation behind the original roadmap ticket? If it is about not showing invalid options, would highlighting them solve the concerns you had?

[junlincc]

@ktmud @hushaoqing
This is a request I received from a user forum hosted by organization that has hundreds and thousands Superset daily users. If i remember correctly, you attended that forum as well. This has been an open roadmap item for over couple months so we left more than enough time to collect feedback before implementing the changes.
Given the number of active users in this organization, we assumed that this request is backed by large amount of user research and has reached consensus by most of the users involved. I personally also encounter this pain point in my daily usage of Superset. so the request makes lot of sense to me. and I think @pkdotson did a fabulous job implementing the changes.

Any changes in Superset nowadays can be contentious, that's why we wanna respect and prioritize the requests that is based on some testing results and research large enterprises raised over personal preference or opinion.

We will go ahead with this change and take your suggestion into next iteration.

[ktmud]

@junlincc Sorry for missing the context. I asked for more information because it was not stated in the roadmap ticket. It would be nice if we can ensure all future tickets have that info.

I know the ticket has been open for a long time, but if I didn't notice this roadmap item, then I doubt others users would have, either.

Again, apologies to both you and @pkdotson for jumping in late, but I think it's worth considering alternatives given how important the control areas are. If you don't think my proposed alternative make sense, then go ahead and implement original proposal. I also don't want @pkdotson 's hard work go wasted.

[ktmud]

And FYI, one of the motivation behind SIP-43 was to have the ability to retain control values when switching viz types:

This not only greatly simplifies the code, but also makes it easier to switch between visualization types---all control values for Query, Transform, and even Columns can be easily retained.

I think it would make sense to extend that ability to datasource switch, too.

[junlincc]

And FYI, one of the motivation behind SIP-43 was to have the ability to retain control values when switching viz types:

This not only greatly simplifies the code, but also makes it easier to switch between visualization types---all control values for Query, Transform, and even Columns can be easily retained.

I think it would make sense to extend that ability to datasource switch, too.

Right, that's the goal. In terms of timeline, it probably will happen naturally after Echarts migration and dynamic control that @villebro proposed. Well actually it's already happening in Time-series Echarts. Really appreciate your suggestions though!

since chart creation flow is still fairly broken, and the lack of consistency in control fields across different chart types, it makes more sense reseting to default when switching dataset for now.

@pkdotson please address @villebro's comment, then we are good to go. thanks you both!

[ktmud]

I guess I got a little concerned when you said this is the end of this project:

end state of this project

- reset/clear/uncheck all query and customize control when dataset is changed
- In the change dataset warning msg, add a line to notify users all the current control settings will be cleared by changing dataset.

Rather than resetting everything to default, I think it makes more sense to just reset SelectControl, MetricsControl and AdhocFilterControl to their default state. Because if the end goal is to retain control values for other controls, we already don't have to do anything special about them. The select controls are trickier because you need to check column validity.

[junlincc]

I guess I got a little concerned when you said this is the end of this project:

The end state of this project for the time being. We really want to make incremental changes and iterate rapidly instead of holding off the progress because of factors that are out of control.

Back to the solution itself... I am going to respect the original feature request received from the forum for now. let's discuss more next week to explore different options along with other contentious topics. im sure we can reach consensus somehow :) @mihir174 @ktmud @villebro

[villebro]

Two minor nits, LGTM!

[junlincc]

Approve as product sign-off ✅

reset control to default -> default metric count, row limit 10000

@pkdotson thanks for the PR! please address @zhaoyongjie 's comment, if that's not much of work.

[zhaoyongjie]

LGTM