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: standardized form_data #20010

Merged
merged 27 commits into from
Jun 5, 2022

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented May 10, 2022

SUMMARY

Superset can't smoothly switch between visualizations and can not safe switching. When you change visualizations, all your work on Explore is about to be lost. For example, there are 3 metrics on the line chart, then switching to the big number, the 3 metrics will be lost, after then switching back to the line chart, the 3 metrics was also gone.

This PR introduces a simple data structure to maintain different viz switching and save switching history. The new data structure is called Standardized Form Data. The SFD will auto-mapping form_data when triggered viz changing action. Each controlPanel in plugins provided denormalizeFormData function. The function supports a manual mapping approach when plugins have to map some control from Standardized Form Data.

Note that the Standardized Form Data is intentionally designed not to persist, but to be hidden in the client's browser memory. In the other words, if the user closes the browser or refreshes the page, the viz changing history state will lose.

This PR has supported

  • Table
  • Pivot Table v2
  • Pie
  • Big Number Total
  • Big Number with Trendline
  • Line Chart v2 (Echart)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

https://www.loom.com/share/08016e60cc594578b105ec1ab9847b4a?sharedAppSource=personal_library

After

https://www.loom.com/share/603dccc033d6496d9399811180842a16?sharedAppSource=personal_library

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@zhaoyongjie zhaoyongjie marked this pull request as ready for review May 11, 2022 17:56
@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #20010 (a05c924) into master (e9007e3) will increase coverage by 0.09%.
The diff coverage is 81.13%.

@@            Coverage Diff             @@
##           master   #20010      +/-   ##
==========================================
+ Coverage   66.44%   66.54%   +0.09%     
==========================================
  Files        1725     1727       +2     
  Lines       64678    64728      +50     
  Branches     6824     6830       +6     
==========================================
+ Hits        42978    43072      +94     
+ Misses      19968    19906      -62     
- Partials     1732     1750      +18     
Flag Coverage Δ
javascript 51.53% <81.13%> (+0.20%) ⬆️

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

Impacted Files Coverage Δ
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
.../BigNumber/BigNumberWithTrendline/controlPanel.tsx 25.00% <0.00%> (-8.34%) ⬇️
...gins/plugin-chart-echarts/src/Pie/controlPanel.tsx 33.33% <0.00%> (-6.67%) ⬇️
...rt-echarts/src/Timeseries/Regular/controlPanel.tsx 33.33% <0.00%> (-6.67%) ⬇️
...ugin-chart-pivot-table/src/plugin/controlPanel.tsx 9.09% <0.00%> (-3.41%) ⬇️
...et-frontend/src/explore/reducers/exploreReducer.js 28.23% <50.00%> (-0.52%) ⬇️
...harts/src/BigNumber/BigNumberTotal/controlPanel.ts 100.00% <100.00%> (ø)
...nd/plugins/plugin-chart-table/src/controlPanel.tsx 50.74% <100.00%> (+34.07%) ⬆️
...d/src/explore/controlUtils/standardizedFormData.ts 100.00% <100.00%> (ø)
...ontend/src/explore/controlUtils/getControlState.ts 90.74% <0.00%> (+5.55%) ⬆️
... and 4 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 e9007e3...a05c924. Read the comment docs.

@zhaoyongjie
Copy link
Member Author

/testenv up

@zhaoyongjie zhaoyongjie requested review from kgabryje, villebro, ktmud and a team May 12, 2022 13:45
@github-actions
Copy link
Contributor

@zhaoyongjie Ephemeral environment spinning up at http://52.34.220.70:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

return this.sfd.memorizedFormData.get(vizType) as QueryFormData;
}

return this.memorizedFormData.slice(-1)[0][1];
Copy link
Member

Choose a reason for hiding this comment

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

Should we add some null check in case memorizedFormData is empty?

Copy link
Member Author

@zhaoyongjie zhaoyongjie May 13, 2022

Choose a reason for hiding this comment

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

The memorizedFormData will not be empty, it is initialized in the constructor.

@kgabryje
Copy link
Member

kgabryje commented May 13, 2022

Found a bug while switching between pivot table and and other charts. When pivot table has the same value added in Columns and Rows controls, then that value is going to be duplicated when switched to a chart with columns control.

Screen.Recording.2022-05-13.at.13.38.00.mov

@zhaoyongjie
Copy link
Member Author

Found a bug while switching between pivot table and and other charts. When pivot table has the same value added in Columns and Rows controls, then that value is going to be duplicated when switched to a chart with columns control.
Screen.Recording.2022-05-13.at.13.38.00.mov

Thanks, Kamil. I also noticed this but I want to discuss it with you for this. Since the Pivot Table allows Row and Columns to have the same columns, I didn't deduplicate the name of columns when collecting sharedControl. The current solution also provides the same view between Table and PivotTable. So, I think that if we need to add the restriction on the SharedControl, we also need to add this in Pivot Table.

@kgabryje
Copy link
Member

Thanks, Kamil. I also noticed this but I want to discuss it with you for this. Since the Pivot Table allows Row and Columns to have the same columns, I didn't deduplicate the name of columns when collecting sharedControl. The current solution also provides the same view between Table and PivotTable. So, I think that if we need to add the restriction on the SharedControl, we also need to add this in Pivot Table.

I think that we should deduplicate control values by default, as we never allow drag and dropping duplicated values. In the case of Pivot Table it's allowed because Rows and Columns are separate controls. So I think that when we transition from Pivot Table to any other chart we should join values from Rows and Columns, remove duplicates and populate Dimensions of the new viz with the result

@stephenLYZ
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@stephenLYZ Ephemeral environment spinning up at http://34.217.96.104:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

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.

Some minor comments, but in general this is a great move in the direction of making the Control panel more intuitive.

@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
denormalizeFormData: formData => ({
...formData,
metrics: formData.standardized_form_data.sharedFormData.metrics,
Copy link
Member

@villebro villebro May 16, 2022

Choose a reason for hiding this comment

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

I'm wondering if we should consider adding percent_metrics to the metrics array in sharedFormData? Let's say we have a table chart with just a single metric in percentage_metrics and change to Pie - currently the metric will be lost.

As a solution - Maybe we could add a property to the normalized control values that states some additional context about the origin of the value. In this case it could specify that it originated from the percent_metrics control rather than a "regular" metric control. So the additional value could, for instance, be called sourceControl.

This would also address the problem identified in the Pivot table - if we had the additional context about what type of a control the value originated from, we would more easily be able to map them back to their original controls (groupbyRows or groupbyColums). Ping @kgabryje

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I follow - it looks to me like rows and columns in pivot table do return to their original states?

Screen.Recording.2022-05-16.at.11.35.36.mov

Or do you mean that, for example, if both columns and rows in pivot table contain a value name, then it gets deduplicated when switching to table but sourceControl contains information that name originates from both columns and rows in pivot table? If that's the point, I like this idea!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the response - I must have had something wrong with my devenv, as after I reloaded it worked just like you described 👍 To add to this comment:

So I think that when we transition from Pivot Table to any other chart we should join values from Rows and Columns, remove duplicates and populate Dimensions of the new viz with the result

I think we should keep the duplicates in the normalized form data, but we could enrich the interface so that we know where the values came from:

export interface SharedFormData {
  metrics: {
    value: QueryFormMetric;
    sourceViz: string;
    sourceControl: string;
  }[];
  columns: {
    value: QueryFormColumn;
    sourceViz: string;
    sourceControl: string;
  }[];
}

This way the pie chart might not care if the metric was from metrics or percent_metrics, but some other chart might decide to map the values over differently, depending on where the value came from. This is especially true for the x_axis control, which in a sense is a column/groupby, but should be handled differently from the series control.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep the duplicates in the normalized form data

I'm still not really sold on this, because we can enter a state which is not allowed via user interface

Copy link
Member

Choose a reason for hiding this comment

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

The good thing here is this is all ephemeral state, so we don't have to be super cautious about breaking changes. So I vote iterating in small increments and not over-abstracting right now (which is precisely what I was doing 😆 )

Copy link
Member Author

@zhaoyongjie zhaoyongjie May 16, 2022

Choose a reason for hiding this comment

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

I'm wondering if we should consider adding percent_metrics to the metrics array in sharedFormData? Let's say we have a table chart with just a single metric in percentage_metrics and change to Pie - currently the metric will be lost.

This is an interesting question. It is also a legacy of Superset's history. The Percentage Metrics is a post-processing metric rather than a SQL base metric. basically, it can not simply be mapping from a source viz to a target viz. The same control is Contribution Mode in the Line chart V2.

I have said before, to solve this issue, I think we eventually need to move all post-processing(AA) into the metric and column so that we can do the real mapping between different charts.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, percent_metrics always makes me cringe for that exact reason (=it's really a post transform op). I'm fine leaving it out for now, as we can easily iteratively improve this functionality as we don't have to worry about backward compatibility.

Copy link
Member Author

@zhaoyongjie zhaoyongjie May 16, 2022

Choose a reason for hiding this comment

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

This way the pie chart might not care if the metric was from metrics or percent_metrics, but some other chart might decide to map the values over differently, depending on where the value came from. This is especially true for the x_axis control, which in a sense is a column/groupby, but should be handled differently from the series control.

The xaxis didn't put in the sharedControls. it's a publicControls. In the other words, the x-axis just "inhert" latest chart x-axis value, it can not mapping from sharedControls.

I suggest that the modeling(like metric/dimension/filter/etc...) should decouple from visualization. The sourceControl: string; means that preserving the context of the control, will allow modeling and visualization to be mixed.

Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

I'm not sure we need to store the standardized form data in a state object. The re-mapping calculation should be ephemeral and wouldn't be useful once users have done switching viz types.

How about we just add a toStandardizedState and fromStandardizedState config to each plugin and use them in the explorer reducer when vizType changes?

There is a function that initiates all control states, which should also be able to receive what is the previous viz type:

export function getControlsState(state, inputFormData) {
/*
* Gets a new controls object to put in the state. The controls object
* is similar to the configuration control with only the controls
* related to the current viz_type, materializes mapStateToProps functions,
* adds value keys coming from inputFormData passed here. This can't be an action creator
* just yet because it's used in both the explore and dashboard views.
* */
// Getting a list of active control names for the current viz
const formData = { ...inputFormData };
const vizType =
formData.viz_type || state.common.conf.DEFAULT_VIZ_TYPE || 'table';
handleDeprecatedControls(formData);
const controlsState = getAllControlsState(
vizType,
state.datasource.type,
state,
formData,
);
const controlPanelConfig = getChartControlPanelRegistry().get(vizType) || {};
if (controlPanelConfig.onInit) {
return controlPanelConfig.onInit(controlsState);
}
return controlsState;
}

@zhaoyongjie
Copy link
Member Author

I'm not sure we need to store the standardized form data in a state object. The re-mapping calculation should be ephemeral and wouldn't be useful once users have done switching viz types.

The Standardized Form Data has 2 uses.

  1. re-mapping the value of controls
  2. save switching history and retrieve the latest form_data by the viz type.

The first use case can easily achieve by defining from/to function in each plugin, but the second use case has to save in form_data.

@zhaoyongjie
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@zhaoyongjie Ephemeral environment spinning up at http://34.220.15.121:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@ktmud
Copy link
Member

ktmud commented May 18, 2022

@zhaoyongjie Is 2 about allowing users to restore field values when they switch between chart types?

Would it suffice if we just keep the history in a reducer state as well? It could be a simple vizType to formData dict. A plugin's fromStandardizedState function will accept both the standardized state from current formData and the previous formData stored and decide how to merge them.

@zhaoyongjie
Copy link
Member Author

@zhaoyongjie Is 2 about allowing users to restore field values when they switch between chart types?

Yes. The memorizedFormData(field in StandardizeFormData) saved the previous chart type as a key, and form_data as a value. In order to find the latest chart, the array was used to store it.

Would it suffice if we just keep the history in a reducer state as well? It could be a simple vizType to formData dict. A plugin's fromStandardizedState function will accept both the standardized state from current formData and the previous formData stored and decide how to merge them.

I've tried this, and as you can see, it's hard to read the logic of the current reducer.

In abstract terms. We just need to collect/calculate the current form_data and controlsState for "arrived viz", and I've written the steps on how to do that in the comments. IMO, we should "encapsulate" the transformation logic rather than “leak” information.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

I think being an ephemeral piece of state that doesn't have any utility elsewhere in the application, there's not a strong need to complicate this with redux updates. If there is value in that, it seems like it would be fine as a follow-up PR. I think this provides a huge UX improvement as is, with little danger of side effects compared to the current implementation. I'd love to keep pressing on with doing this for more plugins, and making touchups/improvements in parallel as needed.

@zhaoyongjie zhaoyongjie merged commit dd4b581 into apache:master Jun 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2022

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 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 size/XL 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants