Skip to content
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

feat: reset metrics on dataset change #12782

Merged
merged 10 commits into from
Feb 5, 2021

Conversation

pkdotson
Copy link
Member

@pkdotson pkdotson commented Jan 27, 2021

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

@pkdotson pkdotson marked this pull request as draft January 27, 2021 00:15
@codecov-io
Copy link

codecov-io commented Jan 27, 2021

Codecov Report

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

Impacted file tree graph

@@            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 junlincc added the explore:control Related to the controls panel of Explore label Jan 27, 2021
@junlincc
Copy link
Member

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.

@pkdotson pkdotson marked this pull request as ready for review January 28, 2021 23:04
@pkdotson pkdotson changed the title feat: [wip] reset metrics on dataset change feat: reset metrics on dataset change Jan 28, 2021
Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

111

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

ktmud commented Jan 29, 2021

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
Copy link
Member

junlincc commented Jan 29, 2021

@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
Copy link
Member

ktmud commented Jan 29, 2021

@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
Copy link
Member

ktmud commented Jan 29, 2021

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
Copy link
Member

junlincc commented Jan 29, 2021

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
Copy link
Member

ktmud commented Jan 30, 2021

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
Copy link
Member

junlincc commented Jan 30, 2021

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

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor nits, LGTM!

Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zhaoyongjie zhaoyongjie merged commit 3bb14ab into apache:master Feb 5, 2021
@villebro villebro deleted the clear-metrics-2 branch February 19, 2021 08:24
ktmud added a commit to ktmud/superset that referenced this pull request Feb 26, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels explore:control Related to the controls panel of Explore preset-io size/M 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[explore] auto clear metrics when switching dataset
8 participants