This is an automated email from the ASF dual-hosted git repository. michaelsmolina pushed a commit to branch airbnb in repository https://gitbox.apache.org/repos/asf/superset.git
commit 264eebb3f73ae7fc1f996886fe7be14471fbaf0a Author: Geido <[email protected]> AuthorDate: Fri Nov 8 19:57:13 2024 +0200 fix(Dashboard): Native & Cross-Filters Scoping Performance (#30881) (cherry picked from commit f4c36a6d055205a3e9ccd9131c317fa79dc30b83) --- .../src/dashboard/components/Dashboard.jsx | 9 +- .../src/dashboard/components/Dashboard.test.jsx | 22 +--- .../src/dashboard/util/getRelatedCharts.test.ts | 24 ++-- .../src/dashboard/util/getRelatedCharts.ts | 138 ++++++++------------- .../util/useFilterFocusHighlightStyles.test.tsx | 24 +--- .../util/useFilterFocusHighlightStyles.ts | 15 +-- 6 files changed, 86 insertions(+), 146 deletions(-) diff --git a/superset-frontend/src/dashboard/components/Dashboard.jsx b/superset-frontend/src/dashboard/components/Dashboard.jsx index 1a852edda8..ddbc2441a5 100644 --- a/superset-frontend/src/dashboard/components/Dashboard.jsx +++ b/superset-frontend/src/dashboard/components/Dashboard.jsx @@ -224,6 +224,7 @@ class Dashboard extends PureComponent { ownDataCharts, this.appliedOwnDataCharts, ); + [...allKeys].forEach(filterKey => { if ( !currFilterKeys.includes(filterKey) && @@ -231,12 +232,12 @@ class Dashboard extends PureComponent { ) { // 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 index cd76787a21..e3421ee040 100644 --- 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 index 874b912d90..a1ba74a20c 100644 --- 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 index 6c63e0d4b1..b3d55ead2a 100644 --- a/superset-frontend/src/dashboard/util/getRelatedCharts.ts +++ b/superset-frontend/src/dashboard/util/getRelatedCharts.ts @@ -32,25 +32,25 @@ 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[] { @@ -58,86 +58,56 @@ function getRelatedChartsForCrossFilter( 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 index ca19c63d51..80e6038c07 100644 --- 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: { diff --git a/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts b/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts index b28d0a3771..aa636cb1ee 100644 --- a/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts +++ b/superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts @@ -54,18 +54,19 @@ const useFilterFocusHighlightStyles = (chartId: number) => { const slices = useSelector((state: RootState) => state.sliceEntities.slices) || {}; - const relatedCharts = getRelatedCharts( - nativeFilters.filters as Record<string, Filter>, - null, - slices, - ); - const highlightedFilterId = nativeFilters?.focusedFilterId || nativeFilters?.hoveredFilterId; + if (!(focusedFilterScope || highlightedFilterId)) { return {}; } + const relatedCharts = getRelatedCharts( + highlightedFilterId as string, + nativeFilters.filters[highlightedFilterId as string] as Filter, + slices, + ); + // we use local styles here instead of a conditionally-applied class, // because adding any conditional class to this container // causes performance issues in Chrome. @@ -80,7 +81,7 @@ const useFilterFocusHighlightStyles = (chartId: number) => { }; if (highlightedFilterId) { - if (relatedCharts[highlightedFilterId]?.includes(chartId)) { + if (relatedCharts.includes(chartId)) { return focusedChartStyles; } } else if (
