fix(Dashboard): Native & Cross-Filters Scoping Performance (#30881)

(cherry picked from commit f4c36a6)
apache · Nov 19, 2024 · 264eebb · 264eebb
1 parent dd80526
commit 264eebb
Show file tree

Hide file tree

Showing 6 changed files with 86 additions and 146 deletions.
diff --git a/superset-frontend/src/dashboard/components/Dashboard.jsx b/superset-frontend/src/dashboard/components/Dashboard.jsx
@@ -224,19 +224,20 @@ class Dashboard extends PureComponent {
       ownDataCharts,
       this.appliedOwnDataCharts,
     );
+
     [...allKeys].forEach(filterKey => {
       if (
         !currFilterKeys.includes(filterKey) &&
         appliedFilterKeys.includes(filterKey)
       ) {
         // filterKey is removed?
         affectedChartIds.push(
-          ...getRelatedCharts(appliedFilters, activeFilters, slices)[filterKey],
+          ...getRelatedCharts(filterKey, appliedFilters[filterKey], slices),
         );
       } else if (!appliedFilterKeys.includes(filterKey)) {
         // filterKey is newly added?
         affectedChartIds.push(
-          ...getRelatedCharts(activeFilters, appliedFilters, slices)[filterKey],
+          ...getRelatedCharts(filterKey, activeFilters[filterKey], slices),
         );
       } else {
         // if filterKey changes value,
@@ -251,9 +252,7 @@ class Dashboard extends PureComponent {
           )
         ) {
           affectedChartIds.push(
-            ...getRelatedCharts(activeFilters, appliedFilters, slices)[
-              filterKey
-            ],
+            ...getRelatedCharts(filterKey, activeFilters[filterKey], slices),
           );
         }
 

diff --git a/superset-frontend/src/dashboard/components/Dashboard.test.jsx b/superset-frontend/src/dashboard/components/Dashboard.test.jsx
@@ -157,9 +157,7 @@ describe('Dashboard', () => {
     });
 
     it('should call refresh when native filters changed', () => {
-      getRelatedCharts.mockReturnValue({
-        [NATIVE_FILTER_ID]: [230],
-      });
+      getRelatedCharts.mockReturnValue([230]);
       wrapper.setProps({
         activeFilters: {
           ...OVERRIDE_FILTERS,
@@ -191,13 +189,7 @@ describe('Dashboard', () => {
     });
 
     it('should call refresh if a filter is added', () => {
-      getRelatedCharts.mockReturnValue({
-        '1_region': [1],
-        '2_country_name': [1, 2],
-        '3_region': [1],
-        '3_country_name': [],
-        gender: [1],
-      });
+      getRelatedCharts.mockReturnValue([1]);
       const newFilter = {
         gender: { values: ['boy', 'girl'], scope: [1] },
       };
@@ -209,12 +201,7 @@ describe('Dashboard', () => {
     });
 
     it('should call refresh if a filter is removed', () => {
-      getRelatedCharts.mockReturnValue({
-        '1_region': [1],
-        '2_country_name': [1, 2],
-        '3_region': [1],
-        '3_country_name': [],
-      });
+      getRelatedCharts.mockReturnValue([]);
       wrapper.setProps({
         activeFilters: {},
       });
@@ -223,6 +210,7 @@ describe('Dashboard', () => {
     });
 
     it('should call refresh if a filter is changed', () => {
+      getRelatedCharts.mockReturnValue([1]);
       const newFilters = {
         ...OVERRIDE_FILTERS,
         '1_region': { values: ['Canada'], scope: [1] },
@@ -236,6 +224,7 @@ describe('Dashboard', () => {
     });
 
     it('should call refresh with multiple chart ids', () => {
+      getRelatedCharts.mockReturnValue([1, 2]);
       const newFilters = {
         ...OVERRIDE_FILTERS,
         '2_country_name': { values: ['New Country'], scope: [1, 2] },
@@ -262,6 +251,7 @@ describe('Dashboard', () => {
     });
 
     it('should call refresh with empty [] if a filter is changed but scope is not applicable', () => {
+      getRelatedCharts.mockReturnValue([]);
       const newFilters = {
         ...OVERRIDE_FILTERS,
         '3_country_name': { values: ['CHINA'], scope: [] },

diff --git a/superset-frontend/src/dashboard/util/getRelatedCharts.test.ts b/superset-frontend/src/dashboard/util/getRelatedCharts.test.ts
@@ -49,10 +49,8 @@ test('Return all chart ids in global scope with native filters', () => {
     } as unknown as Filter,
   };
 
-  const result = getRelatedCharts(filters, null, slices);
-  expect(result).toEqual({
-    filterKey1: [1, 2, 3],
-  });
+  const result = getRelatedCharts('filterKey1', filters.filterKey1, slices);
+  expect(result).toEqual([1, 2, 3]);
 });
 
 test('Return only chart ids in specific scope with native filters', () => {
@@ -74,10 +72,8 @@ test('Return only chart ids in specific scope with native filters', () => {
     } as unknown as Filter,
   };
 
-  const result = getRelatedCharts(filters, null, slices);
-  expect(result).toEqual({
-    filterKey1: [1, 3],
-  });
+  const result = getRelatedCharts('filterKey1', filters.filterKey1, slices);
+  expect(result).toEqual([1, 3]);
 });
 
 test('Return all chart ids with cross filter in global scope', () => {
@@ -90,10 +86,8 @@ test('Return all chart ids with cross filter in global scope', () => {
     } as AppliedCrossFilterType,
   };
 
-  const result = getRelatedCharts(filters, null, slices);
-  expect(result).toEqual({
-    '3': [1, 2],
-  });
+  const result = getRelatedCharts('3', filters['3'], slices);
+  expect(result).toEqual([1, 2]);
 });
 
 test('Return only chart ids in specific scope with cross filter', () => {
@@ -108,8 +102,6 @@ test('Return only chart ids in specific scope with cross filter', () => {
     } as AppliedCrossFilterType,
   };
 
-  const result = getRelatedCharts(filters, null, slices);
-  expect(result).toEqual({
-    '1': [2],
-  });
+  const result = getRelatedCharts('1', filters['1'], slices);
+  expect(result).toEqual([2]);
 });
diff --git a/superset-frontend/src/dashboard/util/getRelatedCharts.ts b/superset-frontend/src/dashboard/util/getRelatedCharts.ts
@@ -32,112 +32,82 @@ function isGlobalScope(scope: number[], slices: Record<string, Slice>) {
 }
 
 function getRelatedChartsForSelectFilter(
-  nativeFilter: AppliedNativeFilterType | Filter,
   slices: Record<string, Slice>,
   chartsInScope: number[],
-) {
-  return Object.values(slices)
-    .filter(slice => {
-      const { slice_id } = slice;
-      // all have been selected, always apply
-      if (isGlobalScope(chartsInScope, slices)) return true;
-      // hand-picked in scope, always apply
-      if (chartsInScope.includes(slice_id)) return true;
+): number[] {
+  // all have been selected, always apply
+  if (isGlobalScope(chartsInScope, slices)) {
+    return Object.keys(slices).map(Number);
+  }
+
+  const chartsInScopeSet = new Set(chartsInScope);
 
-      return false;
-    })
-    .map(slice => slice.slice_id);
+  return Object.values(slices).reduce((result: number[], slice) => {
+    if (chartsInScopeSet.has(slice.slice_id)) {
+      result.push(slice.slice_id);
+    }
+    return result;
+  }, []);
 }
 function getRelatedChartsForCrossFilter(
   filterKey: string,
-  crossFilter: AppliedCrossFilterType,
   slices: Record<string, Slice>,
   scope: number[],
 ): number[] {
   const sourceSlice = slices[filterKey];
 
   if (!sourceSlice) return [];
 
-  return Object.values(slices)
-    .filter(slice => {
-      // cross-filter emitter
-      if (slice.slice_id === Number(filterKey)) return false;
-      // hand-picked in the scope, always apply
-      const fullScope = [
-        ...scope.filter(s => String(s) !== filterKey),
-        Number(filterKey),
-      ];
-      if (isGlobalScope(fullScope, slices)) return true;
-      // hand-picked in the scope, always apply
-      if (scope.includes(slice.slice_id)) return true;
+  const fullScope = [
+    ...scope.filter(s => String(s) !== filterKey),
+    Number(filterKey),
+  ];
+  const scopeSet = new Set(scope);
 
-      return false;
-    })
-    .map(slice => slice.slice_id);
+  return Object.values(slices).reduce((result: number[], slice) => {
+    if (slice.slice_id === Number(filterKey)) {
+      return result;
+    }
+    // Check if it's in the global scope
+    if (isGlobalScope(fullScope, slices)) {
+      result.push(slice.slice_id);
+      return result;
+    }
+    // Check if it's hand-picked in scope
+    if (scopeSet.has(slice.slice_id)) {
+      result.push(slice.slice_id);
+    }
+    return result;
+  }, []);
 }
 
 export function getRelatedCharts(
-  filters: Record<
-    string,
-    AppliedNativeFilterType | AppliedCrossFilterType | Filter
-  >,
-  checkFilters: Record<
-    string,
-    AppliedNativeFilterType | AppliedCrossFilterType | Filter
-  > | null,
+  filterKey: string,
+  filter: AppliedNativeFilterType | AppliedCrossFilterType | Filter,
   slices: Record<string, Slice>,
 ) {
-  const related = Object.entries(filters).reduce((acc, [filterKey, filter]) => {
-    const isCrossFilter =
-      Object.keys(slices).includes(filterKey) &&
-      isAppliedCrossFilterType(filter);
+  let related: number[] = [];
+  const isCrossFilter =
+    Object.keys(slices).includes(filterKey) && isAppliedCrossFilterType(filter);
 
-    const chartsInScope = Array.isArray(filter.scope)
-      ? filter.scope
-      : (filter as Filter).chartsInScope ?? [];
+  const chartsInScope = Array.isArray(filter.scope)
+    ? filter.scope
+    : ((filter as Filter).chartsInScope ?? []);
 
-    if (isCrossFilter) {
-      const checkFilter = checkFilters?.[filterKey] as AppliedCrossFilterType;
-      const crossFilter = filter as AppliedCrossFilterType;
-      const wasRemoved = !!(
-        ((filter.values && filter.values.filters === undefined) ||
-          filter.values?.filters?.length === 0) &&
-        checkFilter?.values?.filters?.length
-      );
-      const actualCrossFilter = wasRemoved ? checkFilter : crossFilter;
+  if (isCrossFilter) {
+    related = getRelatedChartsForCrossFilter(filterKey, slices, chartsInScope);
+  }
 
-      return {
-        ...acc,
-        [filterKey]: getRelatedChartsForCrossFilter(
-          filterKey,
-          actualCrossFilter,
-          slices,
-          chartsInScope,
-        ),
-      };
-    }
-
-    const nativeFilter = filter as AppliedNativeFilterType | Filter;
-    // on highlight, a standard native filter is passed
-    // on apply, an applied native filter is passed
-    if (
-      isAppliedNativeFilterType(nativeFilter) ||
-      isNativeFilter(nativeFilter)
-    ) {
-      return {
-        ...acc,
-        [filterKey]: getRelatedChartsForSelectFilter(
-          nativeFilter,
-          slices,
-          chartsInScope,
-        ),
-      };
-    }
-    return {
-      ...acc,
-      [filterKey]: chartsInScope,
-    };
-  }, {});
+  const nativeFilter = filter as AppliedNativeFilterType | Filter;
+  // on highlight, a standard native filter is passed
+  // on apply, an applied native filter is passed
+  if (
+    !isCrossFilter ||
+    isAppliedNativeFilterType(nativeFilter) ||
+    isNativeFilter(nativeFilter)
+  ) {
+    related = getRelatedChartsForSelectFilter(slices, chartsInScope);
+  }
 
   return related;
 }
diff --git a/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.test.tsx b/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.test.tsx
@@ -61,9 +61,7 @@ describe('useFilterFocusHighlightStyles', () => {
   });
 
   it('should return unfocused styles if chart is not in scope of focused native filter', async () => {
-    mockGetRelatedCharts.mockReturnValue({
-      'test-filter': [],
-    });
+    mockGetRelatedCharts.mockReturnValue([]);
     const store = createMockStore({
       nativeFilters: {
         focusedFilterId: 'test-filter',
@@ -83,9 +81,7 @@ describe('useFilterFocusHighlightStyles', () => {
   });
 
   it('should return unfocused styles if chart is not in scope of hovered native filter', async () => {
-    mockGetRelatedCharts.mockReturnValue({
-      'test-filter': [],
-    });
+    mockGetRelatedCharts.mockReturnValue([]);
     const store = createMockStore({
       nativeFilters: {
         hoveredFilterId: 'test-filter',
@@ -106,9 +102,7 @@ describe('useFilterFocusHighlightStyles', () => {
 
   it('should return focused styles if chart is in scope of focused native filter', async () => {
     const chartId = 18;
-    mockGetRelatedCharts.mockReturnValue({
-      testFilter: [chartId],
-    });
+    mockGetRelatedCharts.mockReturnValue([chartId]);
     const store = createMockStore({
       nativeFilters: {
         focusedFilterId: 'testFilter',
@@ -129,9 +123,7 @@ describe('useFilterFocusHighlightStyles', () => {
 
   it('should return focused styles if chart is in scope of hovered native filter', async () => {
     const chartId = 18;
-    mockGetRelatedCharts.mockReturnValue({
-      testFilter: [chartId],
-    });
+    mockGetRelatedCharts.mockReturnValue([chartId]);
     const store = createMockStore({
       nativeFilters: {
         hoveredFilterId: 'testFilter',
@@ -152,9 +144,7 @@ describe('useFilterFocusHighlightStyles', () => {
 
   it('should return unfocused styles if focusedFilterField is targeting a different chart', async () => {
     const chartId = 18;
-    mockGetRelatedCharts.mockReturnValue({
-      testFilter: [],
-    });
+    mockGetRelatedCharts.mockReturnValue([]);
     const store = createMockStore({
       dashboardState: {
         focusedFilterField: {
@@ -178,9 +168,7 @@ describe('useFilterFocusHighlightStyles', () => {
 
   it('should return focused styles if focusedFilterField chart equals our own', async () => {
     const chartId = 18;
-    mockGetRelatedCharts.mockReturnValue({
-      testFilter: [chartId],
-    });
+    mockGetRelatedCharts.mockReturnValue([chartId]);
     const store = createMockStore({
       dashboardState: {
         focusedFilterField: {