feat: standardized form_data #20010

zhaoyongjie · 2022-05-10T13:33:02Z

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

codecov · 2022-05-12T02:36:06Z

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 · 2022-05-12T13:44:41Z

/testenv up

github-actions · 2022-05-12T13:46:45Z

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

superset-frontend/src/explore/controlUtils/standardizedFormData.ts

kgabryje · 2022-05-13T11:26:33Z

superset-frontend/src/explore/controlUtils/standardizedFormData.ts

+      return this.sfd.memorizedFormData.get(vizType) as QueryFormData;
+    }
+
+    return this.memorizedFormData.slice(-1)[0][1];


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

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

superset-frontend/packages/superset-ui-chart-controls/src/types.ts

kgabryje · 2022-05-13T11:40:14Z

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 · 2022-05-13T12:41:38Z

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 · 2022-05-13T12:56:49Z

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 · 2022-05-16T07:21:17Z

/testenv up

github-actions · 2022-05-16T07:26:22Z

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

villebro

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

superset-frontend/packages/superset-ui-chart-controls/src/types.ts

superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/controlPanel.ts

villebro · 2022-05-16T09:04:20Z

superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx

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


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

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!

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.

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

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 😆 )

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.

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.

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.

superset-frontend/src/explore/controlUtils/standardizedFormData.test.tsx

superset-frontend/packages/superset-ui-chart-controls/src/types.ts

ktmud

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:

superset/superset-frontend/src/explore/store.js

Lines 34 to 62 in 24f805e

    
           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; 
        
           }

superset-frontend/packages/superset-ui-chart-controls/src/types.ts

superset-frontend/src/explore/controlUtils/standardizedFormData.ts

zhaoyongjie · 2022-05-17T01:59:56Z

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.

re-mapping the value of controls
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 · 2022-05-17T02:02:13Z

/testenv up

github-actions · 2022-05-17T02:06:02Z

@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 · 2022-05-18T01:49:32Z

@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 · 2022-05-18T03:14:55Z

@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.

rusackas

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.

github-actions · 2022-06-05T13:05:26Z

Ephemeral environment shutdown and build artifacts deleted.

pull-request-size bot added the size/L label May 10, 2022

zhaoyongjie force-pushed the standardize_controls branch from 88eaf86 to 923538d Compare May 10, 2022 14:39

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

zhaoyongjie force-pushed the standardize_controls branch from 8868739 to b8bd5ce Compare May 12, 2022 01:57

zhaoyongjie requested review from kgabryje, villebro, ktmud and a team May 12, 2022 13:45

pull-request-size bot added size/XL and removed size/L labels May 13, 2022

kgabryje reviewed May 13, 2022

View reviewed changes

superset-frontend/src/explore/controlUtils/standardizedFormData.ts Outdated Show resolved Hide resolved

kgabryje reviewed May 13, 2022

View reviewed changes

superset-frontend/packages/superset-ui-chart-controls/src/types.ts Outdated Show resolved Hide resolved

zhaoyongjie force-pushed the standardize_controls branch from 632e635 to 0b35ea5 Compare May 16, 2022 03:11

villebro reviewed May 16, 2022

View reviewed changes

ktmud reviewed May 16, 2022

View reviewed changes

superset-frontend/packages/superset-ui-chart-controls/src/types.ts Outdated Show resolved Hide resolved

superset-frontend/src/explore/controlUtils/standardizedFormData.ts Outdated Show resolved Hide resolved

wip

a0c1fbf

zhaoyongjie added 20 commits May 26, 2022 21:49

fix type

93f3003

add public controls

b2f4230

add axis

8681a9b

add comments

6e1fed7

ut

86a9f8a

pie chart and bignumber

596366c

fix pie

54d3083

table and ut

3b04ff4

remove scaffold

7b8dc2f

add pivottable

3e10d09

add comments

6fdb5b1

minor fix

4dd96e0

more ut

8e3bc74

remove unused var

34bcaa6

address comments

5867b5f

minor typos

00d5cd5

rename standardized_form_data=>standardizedFormData

6d26cb4

sharedFormData => standardizedState

7606652

fix typo and add UT for memorizedFormData

b0fd499

UT

a05c924

zhaoyongjie force-pushed the standardize_controls branch from 5a63fe2 to a05c924 Compare May 26, 2022 13:52

kgabryje mentioned this pull request Jun 2, 2022

feat(explore): Implement viz switcher redesign #20248

Merged

9 tasks

rusackas approved these changes Jun 3, 2022

View reviewed changes

zhaoyongjie merged commit dd4b581 into apache:master Jun 5, 2022

kgabryje mentioned this pull request Jun 8, 2022

feat(explore): Denormalize form data in echarts, world map and nvd3 bar and line charts #20313

Merged

9 tasks

zhaoyongjie mentioned this pull request Jun 15, 2022

feat: setting limit value when Pie chart switches #20392

Merged

9 tasks

zhaoyongjie mentioned this pull request Jun 24, 2022

feat(standardized form data): keep all columns and metrics #20377

Merged

9 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: standardized form_data #20010

feat: standardized form_data #20010

zhaoyongjie commented May 10, 2022 •

edited

codecov bot commented May 12, 2022 •

edited

zhaoyongjie commented May 12, 2022

github-actions bot commented May 12, 2022

kgabryje May 13, 2022

zhaoyongjie May 13, 2022 •

edited

kgabryje commented May 13, 2022 •

edited

zhaoyongjie commented May 13, 2022

kgabryje commented May 13, 2022

stephenLYZ commented May 16, 2022

github-actions bot commented May 16, 2022

villebro left a comment

villebro May 16, 2022 •

edited

kgabryje May 16, 2022

villebro May 16, 2022

kgabryje May 16, 2022

villebro May 16, 2022

zhaoyongjie May 16, 2022 •

edited

villebro May 16, 2022

zhaoyongjie May 16, 2022 •

edited

ktmud left a comment •

edited

zhaoyongjie commented May 17, 2022

zhaoyongjie commented May 17, 2022

github-actions bot commented May 17, 2022

ktmud commented May 18, 2022

zhaoyongjie commented May 18, 2022

rusackas left a comment

github-actions bot commented Jun 5, 2022

	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;
	}

feat: standardized form_data #20010

feat: standardized form_data #20010

Conversation

zhaoyongjie commented May 10, 2022 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented May 12, 2022 • edited

Codecov Report

zhaoyongjie commented May 12, 2022

github-actions bot commented May 12, 2022

kgabryje May 13, 2022

Choose a reason for hiding this comment

zhaoyongjie May 13, 2022 • edited

Choose a reason for hiding this comment

kgabryje commented May 13, 2022 • edited

zhaoyongjie commented May 13, 2022

kgabryje commented May 13, 2022

stephenLYZ commented May 16, 2022

github-actions bot commented May 16, 2022

villebro left a comment

Choose a reason for hiding this comment

villebro May 16, 2022 • edited

Choose a reason for hiding this comment

kgabryje May 16, 2022

Choose a reason for hiding this comment

villebro May 16, 2022

Choose a reason for hiding this comment

kgabryje May 16, 2022

Choose a reason for hiding this comment

villebro May 16, 2022

Choose a reason for hiding this comment

zhaoyongjie May 16, 2022 • edited

Choose a reason for hiding this comment

villebro May 16, 2022

Choose a reason for hiding this comment

zhaoyongjie May 16, 2022 • edited

Choose a reason for hiding this comment

ktmud left a comment • edited

Choose a reason for hiding this comment

zhaoyongjie commented May 17, 2022

zhaoyongjie commented May 17, 2022

github-actions bot commented May 17, 2022

ktmud commented May 18, 2022

zhaoyongjie commented May 18, 2022

rusackas left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 5, 2022

zhaoyongjie commented May 10, 2022 •

edited

codecov bot commented May 12, 2022 •

edited

zhaoyongjie May 13, 2022 •

edited

kgabryje commented May 13, 2022 •

edited

villebro May 16, 2022 •

edited

zhaoyongjie May 16, 2022 •

edited

zhaoyongjie May 16, 2022 •

edited

ktmud left a comment •

edited