iop
/
superset
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

This commit is contained in:
Geido 2024-11-08 19:57:13 +02:00 committed by GitHub
parent 683ed0d943
commit f4c36a6d05
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 86 additions and 146 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
superset-frontend/src/dashboard/components/Dashboard.jsx
View File

@ -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),
);
}

22
superset-frontend/src/dashboard/components/Dashboard.test.jsx
View File

@ -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: [] },

24
superset-frontend/src/dashboard/util/getRelatedCharts.test.ts
View File

@ -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]);
});

138
superset-frontend/src/dashboard/util/getRelatedCharts.ts
View File

@ -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);
}
return false;
})
.map(slice => slice.slice_id);
const chartsInScopeSet = new Set(chartsInScope);
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;
}

24
superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.test.tsx
View File

@ -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: {

15
superset-frontend/src/dashboard/util/useFilterFocusHighlightStyles.ts
View File

@ -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 (