feat(cross-filters): add cross filters by simcha90 · Pull Request #12662 · apache/superset

simcha90 · 2021-01-21T14:38:57Z

SUMMARY

Add cross filter functionality that gives option to charts to filter other charts

Description according @ktmud questions

1 .This feature is small POC that shows how to support cross-filter functionality between charts.
It partially based on Native Filters functionality with small differences:

it uses setExtraFormData hook passed to chart that save data in redux nativeFilter domain (same like native filters) (not sure if it will be changed in future or not)
when we get data for charts filtering it takes data from native filters domain redux by registered native filters and also by other charts that were registered with property isNativeFilters (I think we use this property only for POC, next we will change it to different property for cross-filtering (cc @villebro))

"Select" Chart it's a native filter that take options like other charts from explore settings
We looked in implementation of filter_box filtering logic and it's too coupled to filter_box structure and it's decoupling can take a lot of time and can cause regression, so we decided reimplement this logic from scratch

Next steps to implement:

Add support for scoping cross-filters

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-01-21.at.16.27.36.mov

TEST PLAN

ADDITIONAL INFORMATION

codecov-io · 2021-01-21T17:52:57Z

Codecov Report

Merging #12662 (cfc7254) into master (2ce7982) will decrease coverage by 0.05%.
The diff coverage is 40.90%.

@@            Coverage Diff             @@
##           master   #12662      +/-   ##
==========================================
- Coverage   53.06%   53.00%   -0.06%     
==========================================
  Files         489      489              
  Lines       17314    17327      +13     
  Branches     4482     4490       +8     
==========================================
- Hits         9187     9185       -2     
- Misses       8127     8142      +15

Flag	Coverage Δ
cypress	`53.00% <40.90%> (-0.06%)`	⬇️

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

Impacted Files	Coverage Δ
superset-frontend/src/chart/ChartContainer.jsx	`100.00% <ø> (ø)`
superset-frontend/src/chart/ChartRenderer.jsx	`75.67% <0.00%> (-1.04%)`	⬇️
...rontend/src/components/ListView/CardSortSelect.tsx	`78.94% <ø> (ø)`
...ontend/src/components/ListViewCard/ImageLoader.tsx	`75.00% <0.00%> (ø)`
...perset-frontend/src/dashboard/containers/Chart.jsx	`100.00% <ø> (ø)`
...shboard/util/charts/getFormDataWithExtraFilters.ts	`92.00% <ø> (ø)`
...perset-frontend/src/views/CRUD/chart/ChartList.tsx	`79.81% <ø> (+5.50%)`	⬆️
...rontend/src/views/CRUD/dashboard/DashboardCard.tsx	`76.00% <ø> (ø)`
...end/src/views/CRUD/data/database/DatabaseModal.tsx	`68.96% <ø> (ø)`
superset-frontend/src/views/CRUD/utils.tsx	`32.05% <ø> (ø)`
... and 20 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 3e0681b...cfc7254. Read the comment docs.

ktmud

Could you add more details on the architecture behind this feature?

Will it only be available to native filters?
Where are the options in the "Select" chart come from?
Were there any rejected alternatives during the implementation?

simcha90 · 2021-01-24T07:41:52Z

Fixed / Added description @ktmud @villebro

agatapst · 2021-01-25T14:28:34Z

-    .filter(type => !registry.get(type).isNativeFilter)
+    .filter(
+      type =>
+        isFeatureEnabled(FeatureFlag.DASHBOARD_CROSS_FILTERS) ||


@simchanielsen could you explain why we need this feature flag enabled here (and below)?

@agatapst DASHBOARD_NATIVE_FILTERS feature flag filter out native filter charts from the view that responsible for creation of charts, because if they will be created as charts they will not can filter other charts, but with new feature of cross-filter functionality they will can to do it, so no need any more filter out them from this view. Because of that FeatureFlag.DASHBOARD_CROSS_FILTERS just cancel DASHBOARD_NATIVE_FILTERS flag

Thanks, makes sense 🙂

ktmud · 2021-01-25T19:03:18Z

I wonder in terms of actual UX, is a separate filter_select viz type actually what we want?

I remember when we talked about cross-filtering before, the expectation was that clicking on elements in a chart itself will update the filters (be it a table chart cell, or the legend in a line chart). Each chart will have a cross_filter config value and will emit filter events accordingly.

Is this still the ultimate goal?

villebro · 2021-01-26T12:43:52Z

I wonder in terms of actual UX, is a separate filter_select viz type actually what we want?

I remember when we talked about cross-filtering before, the expectation was that clicking on elements in a chart itself will update the filters (be it a table chart cell, or the legend in a line chart). Each chart will have a cross_filter config value and will emit filter events accordingly.

Is this still the ultimate goal?

I feel the ultimate goal is to support cross filtering in all charts where it makes sense. But opt-in of course; clicking an item on a chart should only cause cross filtering if explicitly enabled. The following obvious charts come to mind:

table chart: filter only selected rows
pivot table: filter only selected rows/columns/cells
pie chart: filter only selected slices
bar chart: filter only selected bars
timeseries chart: filter only selected time range (x-axis) or selected series.

etc. Even in the presence of more advanced charts with cross-filtering functionality, the simple select filter used in native filters may very well be something that end users want to place on the dashboard like a chart, especially when there will be lots of selects on a dashboard. To reduce confusion, it would be a good idea to keep these native filter charts separate from regular charts in the chart type selector, e.g. in a separate tab ("charts" vs "native filters").

� Conflicts: � superset-frontend/spec/javascripts/dashboard/util/getFormDataWithExtraFilters_spec.ts � superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts

� Conflicts: � superset-frontend/src/dashboard/components/nativeFilters/utils.ts

� Conflicts: � superset-frontend/src/dashboard/components/nativeFilters/utils.ts � superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts

� Conflicts: � superset-frontend/src/explore/components/controls/VizTypeControl.jsx

villebro · 2021-02-12T07:57:49Z

+  if (isFeatureEnabled(FeatureFlag.DASHBOARD_CROSS_FILTERS)) {
+    Object.entries(charts).forEach(([key, chart]) => {
+      if (isCrossFilter(chart?.formData?.viz_type)) {
+        const filterState = nativeFilters.filtersState[key] || {};
+        const { extraFormData: newExtra = {} } = filterState;
+        extraFormData = mergeExtraFormData(extraFormData, newExtra);
+      }
+    });
+  }


General comment: it doesn't really matter now, but especially once we start using overrides as opposed to appends, the order in which these get applied will matter. If possible, we should probably think about some way of visually understanding which chart/filter has taken precedent/been overridden.

villebro

LGTM

* feat: add cross filters * refactor: fix CR notes * lint: fix lint * lint: fix lint * chore: pre-commit * refactor: under chage * refactor: move to behaviors * lint: fix lint Co-authored-by: amitmiran137 <amit.miran@nielsen.com>

* master: (30 commits) refactor(native-filters): decouple params from filter config modal (first phase) (apache#13021) fix(native-filters): set currentValue null when empty (apache#13000) Custom superset_config.py + secret envs (apache#13096) Update http error code from 400 to 403 (apache#13061) feat(native-filters): add storybook entry for select filter (apache#13005) feat(native-filters): Time native filter (apache#12992) Force pod restart on config changes (apache#13056) feat(cross-filters): add cross filters (apache#12662) fix(explore): Enable selecting an option not included in suggestions (apache#13029) Improves RTL configuration (apache#13079) Added a note about the ! prefix for breaking changes to CONTRIBUTING.md (apache#13083) chore: lock down npm to v6 (apache#13069) fix: API tests, make them possible to run independently again (apache#13076) fix: add config to disable dataset ownership on the old api (apache#13051) add required * indicator to message content/notif method (apache#12931) fix: Retroactively add granularity param to charts (apache#12960) fix(ci): multiline regex in change detection (apache#13075) feat(style): hide dashboard header by url parameter (apache#12918) fix(explore): pie chart label bugs (apache#13052) fix: Disabled state button transition time (apache#13008) ...

* feat: add cross filters * refactor: fix CR notes * lint: fix lint * lint: fix lint * chore: pre-commit * refactor: under chage * refactor: move to behaviors * lint: fix lint Co-authored-by: amitmiran137 <amit.miran@nielsen.com>

feat: add cross filters

65e5dfd

pull-request-size Bot added the size/M label Jan 21, 2021

ktmud reviewed Jan 21, 2021

View reviewed changes

Comment thread superset-frontend/src/dashboard/components/nativeFilters/utils.ts Outdated

Comment thread superset-frontend/src/dashboard/components/nativeFilters/utils.ts Outdated

junlincc added dashboard:native-filters Related to the native filters of the Dashboard hold:testing! On hold for testing labels Jan 22, 2021

refactor: fix CR notes

5d5d9e1

simcha90 added 2 commits January 24, 2021 09:42

lint: fix lint

e4ec409

lint: fix lint

cadcb2d

agatapst reviewed Jan 25, 2021

View reviewed changes

simcha90 and others added 9 commits January 26, 2021 14:56

Merge branch 'master' of github.com:apache/superset into cross-filters

6db310b

� Conflicts: � superset-frontend/spec/javascripts/dashboard/util/getFormDataWithExtraFilters_spec.ts � superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts

Merge branch 'master' of github.com:apache/superset into cross-filters

d5747b6

� Conflicts: � superset-frontend/src/dashboard/components/nativeFilters/utils.ts

Merge branch 'master' of github.com:apache/superset into cross-filters

4a42e58

� Conflicts: � superset-frontend/src/dashboard/components/nativeFilters/utils.ts � superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts

chore: pre-commit

a477026

refactor: under chage

a5d9def

Merge branch 'master' of github.com:apache/superset into cross-filters

cfc7254

Merge branch 'master' of github.com:apache/superset into cross-filters

2bb95ce

� Conflicts: � superset-frontend/src/explore/components/controls/VizTypeControl.jsx

refactor: move to behaviors

64768ca

lint: fix lint

d931e06

junlincc added the requires:design-review Requires input/approval from a Designer label Feb 11, 2021

villebro reviewed Feb 12, 2021

View reviewed changes

villebro approved these changes Feb 12, 2021

View reviewed changes

villebro merged commit 956f276 into apache:master Feb 12, 2021

junlincc removed the hold:testing! On hold for testing label Feb 17, 2021

villebro mentioned this pull request Feb 25, 2021

Table filter not working #8273

Closed

suddjian mentioned this pull request Mar 16, 2021

feat(plugin-chart-echarts): Emit cross filters for pie and boxplot apache-superset/superset-ui#1010

Merged

junlincc added dashboard:cross-filters Related to the Dashboard cross filters and removed dashboard:native-filters Related to the native filters of the Dashboard labels Apr 27, 2021

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cross-filters): add cross filters#12662

feat(cross-filters): add cross filters#12662
villebro merged 13 commits into
apache:masterfrom
simcha90:cross-filters

simcha90 commented Jan 21, 2021 •

edited

Loading

Uh oh!

codecov-io commented Jan 21, 2021 •

edited

Loading

Uh oh!

ktmud left a comment

Uh oh!

Uh oh!

Uh oh!

simcha90 commented Jan 24, 2021

Uh oh!

agatapst Jan 25, 2021 •

edited

Loading

Uh oh!

simcha90 Jan 25, 2021 •

edited

Loading

Uh oh!

agatapst Jan 25, 2021

Uh oh!

ktmud commented Jan 25, 2021

Uh oh!

villebro commented Jan 26, 2021 •

edited

Loading

Uh oh!

villebro Feb 12, 2021

Uh oh!

villebro left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

simcha90 commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Uh oh!

codecov-io commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ktmud left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

simcha90 commented Jan 24, 2021

Uh oh!

agatapst Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simcha90 Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agatapst Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

ktmud commented Jan 25, 2021

Uh oh!

villebro commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

villebro Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

simcha90 commented Jan 21, 2021 •

edited

Loading

codecov-io commented Jan 21, 2021 •

edited

Loading

agatapst Jan 25, 2021 •

edited

Loading

simcha90 Jan 25, 2021 •

edited

Loading

villebro commented Jan 26, 2021 •

edited

Loading