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

zhaoyongjie · 2022-06-14T10:26:36Z

SUMMARY

This PR introduced a new mechanism base on the standardize form data. the metrics and columns will always keep its values until user remove it.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

viz.switch.mov

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-06-14T14:02:24Z

Codecov Report

Merging #20377 (e0f9b8a) into master (a833674) will increase coverage by 0.00%.
The diff coverage is 81.08%.

@@           Coverage Diff           @@
##           master   #20377   +/-   ##
=======================================
  Coverage   66.74%   66.74%           
=======================================
  Files        1739     1740    +1     
  Lines       65130    65165   +35     
  Branches     6898     6905    +7     
=======================================
+ Hits        43472    43496   +24     
- Misses      19908    19920   +12     
+ Partials     1750     1749    -1

Flag	Coverage Δ
javascript	`51.81% <81.08%> (+0.01%)`	⬆️

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	`20.00% <0.00%> (-5.00%)`	⬇️
...s/plugin-chart-echarts/src/Funnel/controlPanel.tsx	`57.14% <0.00%> (-9.53%)`	⬇️
...ns/plugin-chart-echarts/src/Gauge/controlPanel.tsx	`50.00% <0.00%> (-16.67%)`	⬇️
...ns/plugin-chart-echarts/src/Graph/controlPanel.tsx	`20.00% <0.00%> (-1.43%)`	⬇️
...gins/plugin-chart-echarts/src/Pie/controlPanel.tsx	`25.00% <0.00%> (-3.58%)`	⬇️
...ins/plugin-chart-echarts/src/Tree/controlPanel.tsx	`37.50% <0.00%> (-5.36%)`	⬇️
.../plugin-chart-echarts/src/Treemap/controlPanel.tsx	`50.00% <0.00%> (-16.67%)`	⬇️
...harts/src/BigNumber/BigNumberTotal/controlPanel.ts	`100.00% <100.00%> (ø)`
...d/src/explore/controlUtils/standardizedFormData.ts	`100.00% <100.00%> (ø)`
... and 11 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 a833674...e0f9b8a. Read the comment docs.

kgabryje · 2022-06-14T14:03:11Z

Thanks for the improvement, love how it works with the new viz switcher!
1 problem that I found while testing - it crashes when I change viz type from big number to pie (only 1 control crashes) and bar chart (the whole page crashes)

Screen.Recording.2022-06-14.at.16.00.02.mov

zhaoyongjie · 2022-06-15T07:22:32Z

/testenv up

zhaoyongjie · 2022-06-15T07:23:23Z

@kgabryje Thanks review! I have rebased the latest master and fix the bug. please review again.

github-actions · 2022-06-15T07:24:35Z

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

kgabryje · 2022-06-15T11:42:05Z

@zhaoyongjie Tested again. It looks like Dimensions are not kept when I do Open line chart -> switch to big number -> switch to bar chart. However, it works correctly if I do Open line chart -> switch to bar (dimensions are kept) -> switch to big number -> switch to bar (dimensions are still kept).

Another example of flow working as expected: Open line chart -> switch to bar (dimensions are kept) -> switch to big number -> switch to bar (dimensions are still kept) -> switch to area chart (dimensions are still kept).
Another example of flow NOT working as expected: Open line chart -> switch to bar (dimensions are kept) -> switch to big number -> switch to area chart (dimensions are NOT kept)

So it looks like dimensions are kept in target chart only if previous chart also had dimensions control.

Screen.Recording.2022-06-15.at.13.43.40.mov

zhaoyongjie · 2022-06-15T11:58:57Z

@zhaoyongjie Tested again. It looks like Dimensions are not kept when I do Open line chart -> switch to big number -> switch to bar chart. However, it works correctly if I do Open line chart -> switch to bar (dimensions are kept) -> switch to big number -> switch to bar (dimensions are still kept).

Another example of flow working as expected: Open line chart -> switch to bar (dimensions are kept) -> switch to big number -> switch to bar (dimensions are still kept) -> switch to area chart (dimensions are still kept). Another example of flow NOT working as expected: Open line chart -> switch to bar (dimensions are kept) -> switch to big number -> switch to area chart (dimensions are NOT kept)

So it looks like dimensions are kept in target chart only if previous chart also had dimensions control.
Screen.Recording.2022-06-15.at.13.43.40.mov

Yes, this is the current workflow. I explain it as follows:

line chart -> switch to big number (do transform from line to big number)
big number -> switch to bar chart (do transform from big number to bar chart), the big number has not dimensions so the bar chart also has not dimensions.
if you do not do any operation on the big number, from big number switch to the line chart, standardized form data will do restore, so the line chart form_data can be restored.

the transform means to transform from the previous chart(Whether you query the data or not.)
the restore means to restore to the original form_data.

Right now, the Standardized Form Data doesn't provide a mechanism to "skip" a chart when do transformation.

kgabryje · 2022-06-15T13:16:03Z

Yes, this is the current workflow. I explain it as follows:

line chart -> switch to big number (do transform from line to big number)

big number -> switch to bar chart (do transform from big number to bar chart), the big number has not dimensions so the bar chart also has not dimensions.

if you do not do any operation on the big number, from big number switch to the line chart, standardized form data will do restore, so the line chart form_data can be restored.

the transform means to transform from the previous chart(Whether you query the data or not.)

the restore means to restore to the original form_data.

Right now, the Standardized Form Data doesn't provide a mechanism to "skip" a chart when do transformation.

I see your point! I'm still a bit afraid that this flow might be confusing for the end user who doesn't know how the feature works under the hood. I think more intuitive flow would be to keep controls in standardized form data and reset it only when user runs query. Like for example, if we saved dimensions in standardized form data while on line chart, they should still be there when user switches to big number, so they can carry over to next viz types. Only after user runs big number query the dimensions would get reset.
That's how I think I'd expect this feature to behave if I was the end user. Maybe we should get some input from product people? @lauderbaugh @rusackas what do you guys think?

zhaoyongjie · 2022-06-15T13:48:05Z

Thanks for the review. I have some different views.

I think more intuitive flow would be to keep controls in standardized form data and reset it only when user runs query. Like for example, if we saved dimensions in standardized form data while on line chart, they should still be there when user switches to big number, so they can carry over to next viz types. Only after user runs big number query the dimensions would get reset.

If we discard the unqueried charts, this will affect the consistency of the analysis. for example

line chart -> switch to the big number
add some filters on the big number, but do not query.
switch to the pie chart,

if we discard the big number form_data, the filters on the big number will be lost when the user switches to the Pie chart.

kgabryje · 2022-06-15T13:55:41Z

if we discard the big number form_data, the filters on the big number will be lost when the user switches to the Pie chart.

Ah no, I don't want to discard big number form data, sorry for not writing that clearer. I only don't want to reset the controls that are not present in big number.
So for example, when we set some controls in line chart, switch to big number and do some changes and then switch to pie - I don't think we should discard changes from big number and restore line chart form data. I think we should use form data of big number + use dimensions stored in standardizedFormData.columns, which was not reset by changing viz type to big number

lauderbaugh · 2022-06-15T15:46:32Z

I think an ideal flow might look something like:
If we have form data for big number and switch to pie, it will keep the form data. If we then switch to line it will keep the form data all the way from big number. If we make adjustments to line and then switch to bar, the adjustments from line will be reflected in bar, (though anything not adjusted would continue to bring the form data all the way from big number if possible). Does that make sense?

kgabryje · 2022-06-15T16:57:54Z

I think an ideal flow might look something like: If we have form data for big number and switch to pie, it will keep the form data. If we then switch to line it will keep the form data all the way from big number. If we make adjustments to line and then switch to bar, the adjustments from line will be reflected in bar, (though anything not adjusted would continue to bring the form data all the way from big number if possible). Does that make sense?

The matter up for discussion is "should switching to Big Number (for example) reset the Dimensions in standardized form data". Big number doesn't have Dimensions control, while Line, Bar, Pie etc. do. So we need to decide what happens when we set open line chart, set a Dimension, then switch to Big Number, then switch to Bar. Should Bar chart have Dimension control populated with value set in Line chart, or should it be reset since the previous viz didn't have that control.

rusackas · 2022-06-15T17:07:01Z

I think an ideal flow might look something like: If we have form data for big number and switch to pie, it will keep the form data. If we then switch to line it will keep the form data all the way from big number. If we make adjustments to line and then switch to bar, the adjustments from line will be reflected in bar, (though anything not adjusted would continue to bring the form data all the way from big number if possible). Does that make sense?

The matter up for discussion is "should switching to Big Number (for example) reset the Dimensions in standardized form data". Big number doesn't have Dimensions control, while Line, Bar, Pie etc. do. So we need to decide what happens when we set open line chart, set a Dimension, then switch to Big Number, then switch to Bar. Should Bar chart have Dimension control populated with value set in Line chart, or should it be reset since the previous viz didn't have that control.

Thanks for distilling this discussion...

There might be a good reason not to do this (some danger I'm not clear on) but my gut feeling is this:

• All data that's modified or appended to the standardized controls should be persisted. You may switch to another viz that doesn't use it, but it should remain in memory/state. Then when you switch at any time to a viz looking for that control, it can be recalled. A field should not be disposed of until the chart is saved, and the ephemeral state is disposed of.

• I think the current logic that already applies to other extraneous (non-standardized) controls (i.e. each chart type keeps its unique properties like a Pie/Donut's inner radius setting) is that that they are kept in that ephemeral state, and restored when returning to the Pie chart after any number of viz switches. Treating standardized controls the same way, so that they may always be restored, makes sense to me.

In other words, is there any harm in keeping that extra/unused data (Dimensions, in our case) in the ephemeral state, in case some other chart type finds it useful?

zhaoyongjie · 2022-06-17T14:17:12Z

/testenv up

github-actions · 2022-06-17T14:21:12Z

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

kgabryje · 2022-06-17T15:00:28Z

I noticed 1 bug, not sure if it was there before. When we switch from Big number to Table, we get a validation error, even though metrics control is filled. Deleting and re-adding the metric fixes the problem.

Screen.Recording.2022-06-17.at.16.57.55.mov

zhaoyongjie · 2022-06-17T15:16:25Z

I noticed 1 bug, not sure if it was there before. When we switch from Big number to Table, we get a validation error, even though metrics control is filled. Deleting and re-adding the metric fixes the problem.
Screen.Recording.2022-06-17.at.16.57.55.mov

big number sent a query, then form_data should follow big_numer's form_data. this means you want show a data layout is big_number, so dimension is lost.(the bignumber does not have dimension.)

The big_number is one metric and 0 dimension viz, you can't get a dimension if you change to table or other more dimension viz.

kgabryje · 2022-06-18T05:58:31Z

big number sent a query, then form_data should follow big_numer's form_data. this means you want show a data layout is big_number, so dimension is lost.(the bignumber does not have dimension.)

The big_number is one metric and 0 dimension viz, you can't get a dimension if you change to table or other more dimension viz.

I mean, table with metric added is correct form data, but the controls show errors (see video)

zhaoyongjie · 2022-06-18T14:39:45Z

big number sent a query, then form_data should follow big_numer's form_data. this means you want show a data layout is big_number, so dimension is lost.(the bignumber does not have dimension.)
The big_number is one metric and 0 dimension viz, you can't get a dimension if you change to table or other more dimension viz.

I mean, table with metric added is correct form data, but the controls show errors (see video)

@kgabryje Oh! I saw! Thanks for the heads up. this issue is also on the master branch. I will fix it in a separate PR.

zhaoyongjie · 2022-06-22T12:23:42Z

/testenv up

github-actions · 2022-06-22T12:27:40Z

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

kgabryje · 2022-06-22T12:34:22Z

Hmm the behaviour of reverting metrics is a bit weird... I think it shouldn't be reverted to viz type's memorized data? But take metrics/dimensions from standardized data?

Screen.Recording.2022-06-22.at.14.31.43.mov

Screen.Recording.2022-06-22.at.14.34.43.mov

zhaoyongjie · 2022-06-22T12:56:52Z

@kgabryje I think we've discussed this before. You don't have a query, so do a restore operation. not a transform operation. it's also you proposal for this feature.

zhaoyongjie · 2022-06-22T13:28:47Z

@kgabryje
I wrote a script for your first footage.

Line chart: ['test metric']
do: transform -> bar
Bar chart: ['test metric']
do: edit, Bar chart, ['test metric', 'count(*)']
do: restore(non query) -> Line, because the bar chart in the memorizedFormData(stack)
Line chart: ['test metric']
do: transform -> bar
Bar chart: ['test metric'], transform from Line chart
do: edit, Bar chart, ['test metric', 'count(*)']
do: transform -> area, because the area chart not in the stack
Are chart: metrics: ['test metric', 'count(*)']
do: restore(non query) -> Line, because the Line chart in the stack
Line chart: metrics: ['test metric']
....
....

the restore propose: restore the interface
the transform propose: continue exploring

zhaoyongjie · 2022-06-24T13:04:14Z

/testenv up

github-actions · 2022-06-24T13:15:25Z

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

kgabryje

LGTM! Thank you for your patience and this awesome feature 😄

github-actions · 2022-06-24T13:46:49Z

Ephemeral environment shutdown and build artifacts deleted.

(cherry picked from commit bbbe102)

pull-request-size bot added the size/M label Jun 14, 2022

zhaoyongjie marked this pull request as ready for review June 14, 2022 13:00

zhaoyongjie requested a review from kgabryje June 14, 2022 13:33

zhaoyongjie force-pushed the feat/revert_chart branch from df92c9b to 09adc06 Compare June 15, 2022 02:39

michael-s-molina added the v2.0 label Jun 16, 2022

pull-request-size bot added size/L and removed size/M labels Jun 17, 2022

zhaoyongjie force-pushed the feat/revert_chart branch from 675e043 to 60bea97 Compare June 22, 2022 07:07

zhaoyongjie changed the title ~~feat(standardize form data): restore previous form_data~~ feat(standardize form data): restore previous form_data and inhert form_data by query history Jun 22, 2022

zhaoyongjie changed the title ~~feat(standardize form data): restore previous form_data and inhert form_data by query history~~ feat(standardize form data): restore previous form_data and inhert form_data from query history Jun 22, 2022

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

4589a1d

zhaoyongjie force-pushed the feat/revert_chart branch from 99cddae to 4589a1d Compare June 24, 2022 10:58

zhaoyongjie changed the title ~~feat(standardize form data): restore previous form_data and inhert form_data from query history~~ feat(standardized form data): keep all columns and metrics Jun 24, 2022

fix lost dimension

e0f9b8a

kgabryje approved these changes Jun 24, 2022

View reviewed changes

zhaoyongjie merged commit bbbe102 into apache:master Jun 24, 2022

michael-s-molina mentioned this pull request Jun 24, 2022

docs: WIP release notes for 2.0 #20490

Merged

michael-s-molina pushed a commit that referenced this pull request Jun 28, 2022

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

4716e0e

(cherry picked from commit bbbe102)

michael-s-molina pushed a commit that referenced this pull request Jun 28, 2022

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

b8d9208

(cherry picked from commit bbbe102)

mistercrunch added 🍒 2.0.0 🍒 2.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zhaoyongjie commented Jun 14, 2022 •

edited

Loading

codecov bot commented Jun 14, 2022 •

edited

Loading

kgabryje commented Jun 14, 2022

zhaoyongjie commented Jun 15, 2022

zhaoyongjie commented Jun 15, 2022

github-actions bot commented Jun 15, 2022

kgabryje commented Jun 15, 2022 •

edited

Loading

zhaoyongjie commented Jun 15, 2022 •

edited

Loading

kgabryje commented Jun 15, 2022

zhaoyongjie commented Jun 15, 2022

kgabryje commented Jun 15, 2022 •

edited

Loading

lauderbaugh commented Jun 15, 2022

kgabryje commented Jun 15, 2022

rusackas commented Jun 15, 2022

zhaoyongjie commented Jun 17, 2022

github-actions bot commented Jun 17, 2022

kgabryje commented Jun 17, 2022

zhaoyongjie commented Jun 17, 2022 •

edited

Loading

kgabryje commented Jun 18, 2022

zhaoyongjie commented Jun 18, 2022

zhaoyongjie commented Jun 22, 2022

github-actions bot commented Jun 22, 2022

kgabryje commented Jun 22, 2022 •

edited

Loading

zhaoyongjie commented Jun 22, 2022

zhaoyongjie commented Jun 22, 2022 •

edited

Loading

zhaoyongjie commented Jun 24, 2022

github-actions bot commented Jun 24, 2022

kgabryje left a comment

github-actions bot commented Jun 24, 2022

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

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

Conversation

zhaoyongjie commented Jun 14, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jun 14, 2022 • edited Loading

Codecov Report

kgabryje commented Jun 14, 2022

zhaoyongjie commented Jun 15, 2022

zhaoyongjie commented Jun 15, 2022

github-actions bot commented Jun 15, 2022

kgabryje commented Jun 15, 2022 • edited Loading

zhaoyongjie commented Jun 15, 2022 • edited Loading

kgabryje commented Jun 15, 2022

zhaoyongjie commented Jun 15, 2022

kgabryje commented Jun 15, 2022 • edited Loading

lauderbaugh commented Jun 15, 2022

kgabryje commented Jun 15, 2022

rusackas commented Jun 15, 2022

zhaoyongjie commented Jun 17, 2022

github-actions bot commented Jun 17, 2022

kgabryje commented Jun 17, 2022

zhaoyongjie commented Jun 17, 2022 • edited Loading

kgabryje commented Jun 18, 2022

zhaoyongjie commented Jun 18, 2022

zhaoyongjie commented Jun 22, 2022

github-actions bot commented Jun 22, 2022

kgabryje commented Jun 22, 2022 • edited Loading

zhaoyongjie commented Jun 22, 2022

zhaoyongjie commented Jun 22, 2022 • edited Loading

zhaoyongjie commented Jun 24, 2022

github-actions bot commented Jun 24, 2022

kgabryje left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 24, 2022

zhaoyongjie commented Jun 14, 2022 •

edited

Loading

codecov bot commented Jun 14, 2022 •

edited

Loading

kgabryje commented Jun 15, 2022 •

edited

Loading

zhaoyongjie commented Jun 15, 2022 •

edited

Loading

kgabryje commented Jun 15, 2022 •

edited

Loading

zhaoyongjie commented Jun 17, 2022 •

edited

Loading

kgabryje commented Jun 22, 2022 •

edited

Loading

zhaoyongjie commented Jun 22, 2022 •

edited

Loading