refactor(native-filters): update dataMask and ExtraFormData schema #13983

simcha90 · 2021-04-07T05:20:13Z

SUMMARY

This PR implements native filter and cross filter API simplification efforts introduced in apache-superset/superset-ui#1040 and apache-superset/superset-ui#1053 . The following changes are done:

setDataMask hook now has new structure:

{
   filterState: { value: any, ...otherProps: any }
   ownState: { ...otherProps: any }
   extraFormData: ExtraFormData
}

Nested append_form_data and override_form_data fields in ExtraFormData type are moved up one level into the main filter object
currentValue removed from formData now it passed in props.filterState.value
In SuperChart injected ownState and filterState
Fixed bug when remove cross filter from dashboard it's wasn't from dashboard metadata
Fixed bug that filter_select was attached to filterBoxes
Fixed bug that when user just added cross filter without setting, it's scoping filter not appeared in filter badge
migrate old filter set metadata to new format
upgrade Python code for legacy charts that depend on viz.py

Affected areas:

Native filters
Cross filters
Own state of charts
Filter sets

TEST PLAN

All frontend + backend unit tests have been updated, and a test for the migration has been added to ensure that both up and down migrations produce expected results.

ADDITIONAL INFORMATION

Has associated issue:
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: only affects dashboards with filter sets. Will only affect environments running the DASHBOARD_NATIVE_FILTERS_SET feature flag
Introduces new feature or API
Removes existing feature or API

amitmiran137 · 2021-04-07T06:22:32Z

LGTM, Let's see what CI reveals

� Conflicts: � superset-frontend/src/explore/components/DataTablesPane/index.tsx

� Conflicts: � superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterSets/utils/index.ts � superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx

� Conflicts: � superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx � superset-frontend/src/filters/components/Select/transformProps.ts � superset-frontend/src/filters/components/Select/types.ts

villebro · 2021-04-12T06:57:53Z

FYI: marking this as WIP until all superset-ui dependencies have been resolved

villebro

A few comments. During testing I also noticed that filter sets don't seem to work:

superset-frontend/src/dashboard/actions/dashboardState.js

superset-frontend/src/dataMask/types.ts

codecov · 2021-04-12T11:17:24Z

Codecov Report

Merging #13983 (28aa87a) into master (68e11cd) will decrease coverage by 0.07%.
The diff coverage is 60.86%.

❗ Current head 28aa87a differs from pull request most recent head d493bb8. Consider uploading reports for the commit d493bb8 to get more accurate results

@@            Coverage Diff             @@
##           master   #13983      +/-   ##
==========================================
- Coverage   79.93%   79.85%   -0.08%     
==========================================
  Files         943      943              
  Lines       47772    47745      -27     
  Branches     6019     6050      +31     
==========================================
- Hits        38185    38126      -59     
- Misses       9468     9497      +29     
- Partials      119      122       +3

Flag	Coverage Δ
cypress	`56.29% <57.14%> (-0.14%)`	⬇️
hive	`80.48% <90.47%> (-0.01%)`	⬇️
javascript	`70.26% <41.42%> (+0.06%)`	⬆️
mysql	`80.75% <90.47%> (-0.01%)`	⬇️
postgres	`80.78% <90.47%> (-0.01%)`	⬇️
presto	`?`
python	`81.19% <90.47%> (-0.16%)`	⬇️
sqlite	`80.39% <90.47%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
superset-frontend/src/chart/Chart.jsx	`64.81% <ø> (ø)`
.../src/dashboard/components/gridComponents/Chart.jsx	`86.59% <ø> (ø)`
...ilters/FilterBar/FilterControls/FilterControls.tsx	`94.73% <ø> (-0.27%)`	⬇️
...nativeFilters/FilterBar/FilterSets/EditSection.tsx	`80.00% <ø> (ø)`
...perset-frontend/src/dashboard/containers/Chart.jsx	`100.00% <ø> (ø)`
superset-frontend/src/dashboard/reducers/types.ts	`0.00% <ø> (ø)`
superset-frontend/src/dataMask/types.ts	`100.00% <ø> (ø)`
...tend/src/explore/components/ExploreChartHeader.jsx	`84.21% <ø> (ø)`
...ntend/src/explore/components/ExploreChartPanel.jsx	`82.60% <ø> (ø)`
...src/explore/components/controls/VizTypeControl.jsx	`92.06% <0.00%> (ø)`
... and 56 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 68e11cd...d493bb8. Read the comment docs.

� Conflicts: � superset-frontend/spec/javascripts/dashboard/fixtures/mockNativeFilters.ts � superset-frontend/src/dashboard/actions/hydrate.js � superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterSets/FiltersHeader.tsx � superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx � superset-frontend/src/dashboard/util/activeAllDashboardFilters.ts

…et into own_state

github-actions · 2021-04-13T18:41:43Z

⚠️ @simcha90 Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

villebro · 2021-04-15T08:07:21Z

superset/utils/core.py

-    applied_time_extras = form_data.get("applied_time_extras", {})
-    form_data["applied_time_extras"] = applied_time_extras


applied_time_extras are no longer needed for filter indicators

dpgaspar

Looks good, left a couple of comments

dpgaspar · 2021-04-15T08:56:17Z

superset/constants.py

+    "time_range": "time_range",
+}
+
+EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS = [


nit: we could use a set here

dpgaspar · 2021-04-15T08:59:02Z

superset/migrations/versions/fc3a3a8ff221_migrate_filter_sets_to_new_format.py

+
+
+def upgrade():
+    bind = op.get_bind()


There's some complexity here, would be nice to place the entire logic here: https://github.com/apache/superset/tree/master/superset/migrations/shared
Then add some tests to assert that the migrations works has expected

#13351

kgabryje · 2021-04-15T10:27:35Z

LGTM! It seems that there's still the old structure used in findExistingFilterSet.test.ts

zhaoyongjie

LGTM! Added some suggestions.

zhaoyongjie · 2021-04-15T14:01:29Z

superset/migrations/versions/fc3a3a8ff221_migrate_filter_sets_to_new_format.py

+    dashboards = (
+        session.query(Dashboard)
+        .filter(Dashboard.json_metadata.like('%"filter_sets_configuration"%'))
+        .all()
+    )
+    changed_filter_sets, changed_filters = 0, 0
+    for dashboard in dashboards:


Suggested change

dashboards = (

session.query(Dashboard)

.filter(Dashboard.json_metadata.like('%"filter_sets_configuration"%'))

.all()

)

changed_filter_sets, changed_filters = 0, 0

for dashboard in dashboards:

dashboards = (

session.query(Dashboard)

.filter(Dashboard.json_metadata.like('%"filter_sets_configuration"%'))

)

changed_filter_sets, changed_filters = 0, 0

for dashboard in dashboards.yield_per(100):

Thanks - this is a great pattern! As this is behind a FF, I don't expect we'll have lots of these dashboards. Also in the interest of CI time and unblocking other PRs that are dependent on this I think I'll just merge now, but I can open up a follow up PR to implement this.

zhaoyongjie · 2021-04-15T14:02:36Z

superset/migrations/versions/fc3a3a8ff221_migrate_filter_sets_to_new_format.py

+    dashboards = (
+        session.query(Dashboard)
+        .filter(Dashboard.json_metadata.like('%"filter_sets_configuration"%'))
+        .all()
+    )
+    changed_filter_sets, changed_filters = 0, 0
+    for dashboard in dashboards:


Suggested change

dashboards = (

session.query(Dashboard)

.filter(Dashboard.json_metadata.like('%"filter_sets_configuration"%'))

.all()

)

changed_filter_sets, changed_filters = 0, 0

for dashboard in dashboards:

dashboards = (

session.query(Dashboard)

.filter(Dashboard.json_metadata.like('%"filter_sets_configuration"%'))

)

changed_filter_sets, changed_filters = 0, 0

for dashboard in dashboards.yield_per(100):

…pache#13983) * refactor: updates usage of `ownFilters` to `ownState` * refactor: update dataMask (final) * lint: fix lint * refactor: revert feat * fix: fix missed chart configuration * add filter set migration * apply new changes * fix migration revision * update migration * fix jest mock * js lint * fix test types * update tests and types * remove append_form_data from tests * fix findExistingFilterSet tests * add migration test Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>

simcha90 added 2 commits April 6, 2021 17:05

refactor: updates usage of ownFilters to ownState

9b27475

Merge branch 'master' of github.com:apache/superset into own_state

a9aaf29

pull-request-size bot added the size/L label Apr 7, 2021

amitmiran137 requested a review from villebro April 7, 2021 06:22

refactor: update dataMask (final)

274f559

pull-request-size bot added size/XL and removed size/L labels Apr 8, 2021

simcha90 changed the title ~~refactor(native-filters): Updates usage of ownFilters to ownState~~ refactor(native-filters): Updates usage of dataMask structure Apr 8, 2021

simcha90 added 2 commits April 8, 2021 11:41

Merge branch 'master' of github.com:apache/superset into own_state

fc47dad

� Conflicts: � superset-frontend/src/explore/components/DataTablesPane/index.tsx

lint: fix lint

6a9d5d3

amitmiran137 added the v1.2 label Apr 11, 2021

simcha90 added 2 commits April 12, 2021 08:35

Merge branch 'master' of github.com:apache/superset into own_state

1501558

� Conflicts: � superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterSets/utils/index.ts � superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx

villebro changed the title ~~refactor(native-filters): Updates usage of dataMask structure~~ [WIP] refactor(native-filters): Updates usage of dataMask structure Apr 12, 2021

villebro reviewed Apr 12, 2021

View reviewed changes

superset-frontend/src/dashboard/actions/dashboardState.js Outdated Show resolved Hide resolved

superset-frontend/src/dataMask/types.ts Show resolved Hide resolved

simcha90 added 2 commits April 12, 2021 10:24

refactor: revert feat

b64ae02

fix: fix missed chart configuration

63799f5

simcha90 requested a review from a team as a code owner April 12, 2021 10:34

add filter set migration

1f12208

villebro force-pushed the own_state branch from 6ff3831 to 1f12208 Compare April 12, 2021 10:43

simcha90 added 2 commits April 13, 2021 09:49

Merge branch 'own_state' of github.com:simchaNielsen/incubator-supers…

b84d266

…et into own_state

villebro added 4 commits April 14, 2021 16:16

apply new changes

8f5a2f2

Merge branch 'master' into own_state

11d21af

fix migration revision

aa6baba

Merge branch 'master' into own_state

68d4b1f

update migration

8751f3f

pull-request-size bot added size/XXL and removed size/XL labels Apr 15, 2021

fix jest mock

2ee3dfc

villebro changed the title ~~[WIP] refactor(native-filters): Updates usage of dataMask structure~~ refactor(native-filters): update dataMask and ExtraFormData schema Apr 15, 2021

villebro added 3 commits April 15, 2021 07:56

js lint

0653a92

fix test types

336adbf

update tests and types

1ebfa7d

villebro reviewed Apr 15, 2021

View reviewed changes

villebro added the risk:db-migration PRs that require a DB migration label Apr 15, 2021

remove append_form_data from tests

29a8b59

villebro force-pushed the own_state branch from 29049f8 to 29a8b59 Compare April 15, 2021 08:13

dpgaspar reviewed Apr 15, 2021

View reviewed changes

fix findExistingFilterSet tests

22863aa

add migration test

d493bb8

villebro force-pushed the own_state branch from b5112aa to d493bb8 Compare April 15, 2021 13:00

zhaoyongjie approved these changes Apr 15, 2021

View reviewed changes

zhaoyongjie mentioned this pull request Apr 15, 2021

feat(plugin-chart-echarts): Radar chart POC apache-superset/superset-ui#1029

Merged

villebro merged commit 8ef572a into apache:master Apr 15, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(native-filters): update dataMask and ExtraFormData schema #13983

refactor(native-filters): update dataMask and ExtraFormData schema #13983

simcha90 commented Apr 7, 2021 •

edited by villebro

Loading

amitmiran137 commented Apr 7, 2021

villebro commented Apr 12, 2021

villebro left a comment

codecov bot commented Apr 12, 2021 •

edited

Loading

github-actions bot commented Apr 13, 2021

villebro Apr 15, 2021 •

edited

Loading

dpgaspar left a comment

dpgaspar Apr 15, 2021

dpgaspar Apr 15, 2021

kgabryje commented Apr 15, 2021

zhaoyongjie left a comment

zhaoyongjie Apr 15, 2021

villebro Apr 15, 2021

zhaoyongjie Apr 15, 2021

		applied_time_extras = form_data.get("applied_time_extras", {})
		form_data["applied_time_extras"] = applied_time_extras

refactor(native-filters): update dataMask and ExtraFormData schema #13983

refactor(native-filters): update dataMask and ExtraFormData schema #13983

Conversation

simcha90 commented Apr 7, 2021 • edited by villebro Loading

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

amitmiran137 commented Apr 7, 2021

villebro commented Apr 12, 2021

villebro left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2021 • edited Loading

Codecov Report

github-actions bot commented Apr 13, 2021

villebro Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Apr 15, 2021

Choose a reason for hiding this comment

dpgaspar Apr 15, 2021

Choose a reason for hiding this comment

kgabryje commented Apr 15, 2021

zhaoyongjie left a comment

Choose a reason for hiding this comment

zhaoyongjie Apr 15, 2021

Choose a reason for hiding this comment

villebro Apr 15, 2021

Choose a reason for hiding this comment

zhaoyongjie Apr 15, 2021

Choose a reason for hiding this comment

simcha90 commented Apr 7, 2021 •

edited by villebro

Loading

codecov bot commented Apr 12, 2021 •

edited

Loading

villebro Apr 15, 2021 •

edited

Loading