[DataGrid] Do not apply filters on `rowExpansionChange` #8671

cherniavskii · 2023-04-19T10:45:38Z

Before: https://codesandbox.io/s/friendly-gwen-w9ftyn?file=/demo.tsx
After: https://codesandbox.io/s/hardcore-mahavira-n88l5j?file=/demo.tsx

See #8579 (comment) for the issue explanation.
To avoid filtering rows on rowExpansionChange, I extracted visibleRowsLookup out of the filter state:

-state.filter.visibleRowsLookup
+state.visibleRows.lookup

Pros:

Filters are not applied on rowExpansionChange

Cons:

Before this change, visibleRowsLookup was updated along with state.filter.filteredRowsLookup in the same iteration over state.rows.tree. After this change, we iterate over state.rows.tree again to compute state.visibleRowsLookup.

TODO:

Add unit test

mui-bot · 2023-04-19T10:50:40Z

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8671--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	614.8	1,569	614.8	988.38	344.409
Sort 100k rows	ms	532.9	1,223.4	1,223.4	972.04	233.484
Select 100k rows	ms	299	401.5	363.9	353.08	39.943
Deselect 100k rows	ms	193.7	356.3	250.2	255.92	57.278

Generated by 🚫 dangerJS against f0629a3

cherniavskii · 2023-04-20T18:38:12Z

packages/grid/x-data-grid/src/hooks/features/filter/useGridFilter.tsx

      filteredDescendantCountLookup: {},
    },
  };
 };

+function useGridVisibleRowsLookup(apiRef: React.MutableRefObject<GridPrivateApiCommunity>) {


I'm going to move it outside of the useGridFilter hook, it was just faster to do it like this to validate the approach

joserodolfofreitas

Wow, the performance step is impressive! Nice work!

github-actions · 2023-05-18T13:23:25Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

packages/grid/x-data-grid/src/hooks/features/rows/gridVisibleRowsSelector.ts

packages/grid/x-data-grid/src/hooks/features/rows/useGridVisibleRowsState.ts

MBilalShafi · 2023-05-24T08:05:44Z

Great work @cherniavskii, it does seem to solve the problem, though I think we would require to apply filtering and aggregation in some use cases, for example when we lazy-load row children, we will need to reapply filtering and aggregation to re-validate the stale data. (But we can do that on demand too, let's say when the promise for fetching new rows is resolved, not sure)

After digging a bit into the problem, I found the problem went all the way down to the applyAggregation method, which recomputes state.aggregation.lookup on each call (even when not needed):

mui-x/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/useGridAggregation.ts

Lines 72 to 89 in e9cb67d

    
           const applyAggregation = React.useCallback(() => { 
        
             const aggregationLookup = createAggregationLookup({ 
        
               apiRef, 
        
               getAggregationPosition: props.getAggregationPosition, 
        
               aggregationFunctions: props.aggregationFunctions, 
        
               aggregationRowsScope: props.aggregationRowsScope, 
        
             }); 
        
             apiRef.current.setState((state) => ({ 
        
               ...state, 
        
               aggregation: { ...state.aggregation, lookup: aggregationLookup }, 
        
             })); 
        
           }, [ 
        
             apiRef, 
        
             props.getAggregationPosition, 
        
             props.aggregationFunctions, 
        
             props.aggregationRowsScope, 
        
           ]);

One way to solve this could be to only recompute the state.aggregation.lookup on demand, something like:

diff --git a/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/useGridAggregation.ts b/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/useGridAggregation.ts
index 2a9e12601..d1b2f75b1 100644
--- a/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/useGridAggregation.ts
+++ b/packages/grid/x-data-grid-premium/src/hooks/features/aggregation/useGridAggregation.ts
@@ -70,21 +70,38 @@ export const useGridAggregation = (
   );
 
   const applyAggregation = React.useCallback(() => {
-    const aggregationLookup = createAggregationLookup({
-      apiRef,
-      getAggregationPosition: props.getAggregationPosition,
-      aggregationFunctions: props.aggregationFunctions,
-      aggregationRowsScope: props.aggregationRowsScope,
-    });
-
-    apiRef.current.setState((state) => ({
-      ...state,
-      aggregation: { ...state.aggregation, lookup: aggregationLookup },
-    }));
+    const { rulesOnLastRowHydration } = apiRef.current.caches.aggregation;
+
+    const aggregationRules = props.disableAggregation
+      ? {}
+      : getAggregationRules({
+          columnsLookup: gridColumnLookupSelector(apiRef),
+          aggregationModel: gridAggregationModelSelector(apiRef),
+          aggregationFunctions: props.aggregationFunctions,
+        });
+
+    if (
+      !areAggregationRulesEqual(rulesOnLastRowHydration, aggregationRules) ||
+      !apiRef.current.state.aggregation.lookup
+    ) {
+      const aggregationLookup = createAggregationLookup({
+        apiRef,
+        getAggregationPosition: props.getAggregationPosition,
+        aggregationFunctions: props.aggregationFunctions,
+        aggregationRowsScope: props.aggregationRowsScope,
+      });
+
+      apiRef.current.setState((state) => ({
+        ...state,
+        aggregation: { ...state.aggregation, lookup: aggregationLookup },
+      }));
+    }
   }, [
     apiRef,
-    props.getAggregationPosition,
+    props.disableAggregation,
     props.aggregationFunctions,
+    props.getAggregationPosition,
     props.aggregationRowsScope,
   ]);

This is definitely not an ultimate fix as it partially solves the problem of not re-computing the aggregation lookup on rows expansion change, but it causes another bug, i.e. now the grid is not responding properly to the aggregation function change.

Another Dimension:

On the codesandbox where the package for this PR is installed, I played around and I observed that the page was not anymore stuck on row expansion change, but when I tried to change an aggregation function, it blocked the UI thread for a similar amount of time:

I think the reason is that our diffing algorithm is not smart enough to check, "ok, so out of 100 columns, the aggregation functions for column 1 has changed, let's go ahead and only re-compute the aggregation lookup for this column", rather it recomputes all the data once any column updates it's aggregation method.

So maybe this direction could be a better one:

Optimize the diffing algorithm to compute the specific sub-part of aggregation we need to compute (to solve problem 2)
Apply this diffing also when we call applyAggregation on filteredRowsSet. (to solve the original problem)

What do you think?

packages/grid/x-data-grid-premium/src/DataGridPremium/useDataGridPremiumComponent.tsx

packages/grid/x-data-grid-premium/src/hooks/features/rowGrouping/gridRowGroupingUtils.ts

cherniavskii · 2023-05-24T19:38:55Z

...s/grid/x-data-grid-premium/src/hooks/features/rowGrouping/useGridRowGroupingPreProcessors.ts

@@ -17,6 +17,7 @@ import {
  createRowTree,


@MBilalShafi

On the codesandbox where the package for this PR is installed, I played around and I observed that the page was not anymore stuck on row expansion change, but when I tried to change an aggregation function, it blocked the UI thread for a similar amount of time:

I think the reason is that our diffing algorithm is not smart enough to check, "ok, so out of 100 columns, the aggregation functions for column 1 has changed, let's go ahead and only re-compute the aggregation lookup for this column", rather it recomputes all the data once any column updates it's aggregation method.

That's a very good point, I'm sure we can optimize aggregation (especially when there's a lot of aggregated columns) 👍

m4theushw · 2023-05-25T18:03:59Z

packages/grid/x-data-grid/src/hooks/features/rows/useGridVisibleRowsState.ts

+        visibleRows: visibleRowsState,
+      };
+    });
+    apiRef.current.forceUpdate();


Wouldn't be easier to understand if we updated the visibleRows state also inside

mui-x/packages/grid/x-data-grid/src/hooks/features/filter/useGridFilter.tsx

Line 78 in 8c25860

const updateFilteredRows = React.useCallback(() => {

and remove the filteredRowsSet event? We can make an exception and have a single hook controlling two states. This apiRef.current.forceUpdate call here but not in the filtering hook seems inconsistent. We don't forceUpdate there but expect that one listener of filteredRowsSet will call it. I had some problems in #9112 where, in one of the solutions I tried, I removed a forceUpdate from a method that was doing nothing and one test started to fail. I think it would be better if we tried to rely less on event listeners and make the code less "connected". At the end we need:

update both states when applyFilters is called

update only visibleRows if a row is expanded

Good point, it will be easier to follow 👍

For the filteredRowsSet event, it will still be used by the aggregation hook.

romgrk · 2023-05-29T16:45:05Z

...s/grid/x-data-grid-premium/src/hooks/features/rowGrouping/useGridRowGroupingPreProcessors.ts

+const getVisibleRows: GridStrategyProcessor<'visibleRows'> = (params) => {
+  return {
+    lookup: getVisibleRowsLookup(params),
+  };
+};


Do we need to wrap in a return object?

Not really. I added a wrapper when I extracted the state management to a separate hook, but now that it's coupled with the filter state - I have removed the wrapper and renamed visibleRows to visibleRowsLookup 1aca18c (#8671)

MBilalShafi

A very nice optimization 🚀
Great work!

packages/grid/x-data-grid/src/hooks/features/filter/useGridFilter.tsx

MBilalShafi · 2023-05-30T10:21:32Z

packages/grid/x-data-grid/src/hooks/features/rows/gridVisibleRowsInterfaces.ts

@@ -0,0 +1,12 @@
+import type { GridRowId } from '../../../models/gridRows';


Nit: Should this interface and gridVisibleRowsSelectors be moved in close proximity to the rest of the code hooks/features/filter/gridFilterState or models/gridVisibleRows for better organization of files as it's being used in useGridFilter?

Yeah, I'll move everything back to the filtering files (it's where it was originally anyway, before the state split)

packages/grid/x-data-grid-pro/src/hooks/features/treeData/useGridTreeDataPreProcessors.tsx

romgrk · 2023-05-31T23:24:02Z

packages/grid/x-data-grid-pro/src/utils/tree/utils.ts

+
+    visibleRowsLookup[node.id] = isPassingFiltering && areAncestorsExpanded;
+
+    // TODO rows v6: Should we keep storing the visibility status of footer independently or rely on the group visibility in the selector ?


Do you have any opinions on this comment? Are footers sometimes hidden?

Yes, you can have aggregation footer for each row group - see https://mui.com/x/react-data-grid/aggregation/#AggregationGetAggregationPosition.tsx

When the group is filtered out, the aggregation footer should be hidden as well.

IIUC, the visibility of the group is always equal to the visibility of the footer, right?

Not exactly, you can have a visible row group, but if it's not expanded - the footer row shouldn't be visible:

mui-x/packages/grid/x-data-grid-pro/src/utils/tree/utils.ts

Lines 268 to 269 in 9aa0d40

visibleRowsLookup[node.footerId] =

isPassingFiltering && areAncestorsExpanded && !!node.childrenExpanded;

row-group-footer.mov

romgrk · 2023-05-31T23:31:04Z

packages/grid/x-data-grid/src/hooks/features/filter/gridFilterState.ts

+/**
+ * Visibility status for each row.
+ * A row is visible if it is passing the filters AND if its parents are expanded.
+ * If a row is not registered in this lookup, it is visible.
+ */
+export interface GridVisibleRowsLookupState extends Record<GridRowId, boolean> {}


Just fyi, you may have seen in my filtering PR, but I was hoping to convert both the filteredRows & visibleRows from objects into Sets as it improves performance for large datasets.

Minor note on the type, I would find type GridVisibleRowsLookupState = Record<GridRowId, boolean> more semantically accurate. We're not building a new interface, we're defining what the shape of that state is, and it is a record.

Just fyi, you may have seen in my filtering PR, but I was hoping to convert both the filteredRows & visibleRows from objects into Sets as it improves performance for large datasets.

Yes, but that would be a breaking change, right?
Unless we do something like this:

/** * For backward compatibility only, in case if anyone is using `gridVisibleRowsLookupSetSelector` * @deprecated Use `gridVisibleRowsLookupSetSelector` instead */ export const gridVisibleRowsLookupSelector = (state: GridStateCommunity) => setToObject(state.visibleRowsLookup); // TODO v7: rename to `gridVisibleRowsLookupSelector` export const gridVisibleRowsLookupSetSelector = (state: GridStateCommunity) => state.visibleRowsLookup;

Minor note on the type, I would find type GridVisibleRowsLookupState = Record<GridRowId, boolean> more semantically accurate.

Updated 👍

Yes for the change, that's what I'm planning to do for the selectors.

do not apply filters on rowExpansionChange

0cee1cd

cherniavskii added performance feature: Row grouping Related to the data grid Row grouping feature feature: Aggregation Related to the data grid Aggregation feature component: data grid This is the name of the generic UI component, not the React module! labels Apr 19, 2023

cherniavskii added 2 commits April 20, 2023 20:09

Merge branch 'master' into do-not-apply-filters-on-rowExpansionChange

0c9f390

add unit tests

b34e757

cherniavskii commented Apr 20, 2023

View reviewed changes

cherniavskii marked this pull request as ready for review April 20, 2023 18:40

cherniavskii requested review from DanailH, MBilalShafi and m4theushw April 20, 2023 18:40

cherniavskii added 4 commits April 25, 2023 10:53

Merge branch 'master' into do-not-apply-filters-on-rowExpansionChange

8e9ced1

move useGridVisibleRowsLookup outside of useGridFilter

1e1e912

change shape of visibleRowsLookup state to visibleRows.lookup

952c8a4

rename useGridVisibleRowsLookup to useGridVisibleRowsState

db01474

joserodolfofreitas reviewed Apr 25, 2023

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 18, 2023

Merge branch 'master' into do-not-apply-filters-on-rowExpansionChange

b9cbd3d

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 22, 2023

romgrk reviewed May 22, 2023

View reviewed changes

packages/grid/x-data-grid/src/hooks/features/rows/gridVisibleRowsSelector.ts Outdated Show resolved Hide resolved

romgrk reviewed May 22, 2023

View reviewed changes

packages/grid/x-data-grid/src/hooks/features/rows/useGridVisibleRowsState.ts Outdated Show resolved Hide resolved

avoid useCallback for pure functions

d055c1d

cherniavskii commented May 24, 2023

View reviewed changes

packages/grid/x-data-grid-premium/src/DataGridPremium/useDataGridPremiumComponent.tsx Outdated Show resolved Hide resolved

cherniavskii commented May 24, 2023

View reviewed changes

packages/grid/x-data-grid-premium/src/hooks/features/rowGrouping/gridRowGroupingUtils.ts Show resolved Hide resolved

cherniavskii commented May 24, 2023

View reviewed changes

m4theushw mentioned this pull request May 25, 2023

[DataGrid] Skip filtering if model didn't change #9112

Open

m4theushw reviewed May 25, 2023

View reviewed changes

cherniavskii added 2 commits May 29, 2023 16:44

Merge branch 'master' into do-not-apply-filters-on-rowExpansionChange

0b1f969

move visible state management to filtering hook

7422d23

cherniavskii requested a review from m4theushw May 29, 2023 16:38

romgrk reviewed May 29, 2023

View reviewed changes

MBilalShafi approved these changes May 30, 2023

View reviewed changes

cherniavskii added 7 commits May 30, 2023 17:37

initialize visibleRows state along with filter state

c58488b

move visibleRows interfaces back to filtering

baab0df

move visible rows selectors to filter selectors

5b5cd60

avoid additional package exports

daa6bd8

flatten visibleRows lookup state

1aca18c

rename visibleRows strategy processor

a10b77e

fix tests

6bd5c21

cherniavskii added the feature: Tree data Related to the data grid Tree data feature label May 31, 2023

romgrk reviewed May 31, 2023

View reviewed changes

cherniavskii added 2 commits June 1, 2023 10:24

Merge branch 'master' into do-not-apply-filters-on-rowExpansionChange

9e99470

use type instead of interface

f0629a3

cherniavskii merged commit 4ecaf68 into mui:master Jun 1, 2023
4 checks passed

cherniavskii deleted the do-not-apply-filters-on-rowExpansionChange branch June 1, 2023 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Do not apply filters on `rowExpansionChange` #8671

[DataGrid] Do not apply filters on `rowExpansionChange` #8671

cherniavskii commented Apr 19, 2023 •

edited

Loading

mui-bot commented Apr 19, 2023 •

edited

Loading

cherniavskii Apr 20, 2023

cherniavskii Apr 25, 2023

joserodolfofreitas left a comment

github-actions bot commented May 18, 2023

MBilalShafi commented May 24, 2023 •

edited

Loading

cherniavskii May 24, 2023

m4theushw May 25, 2023

cherniavskii May 29, 2023

romgrk May 29, 2023

cherniavskii May 31, 2023

MBilalShafi left a comment

MBilalShafi May 30, 2023

cherniavskii May 30, 2023

romgrk May 31, 2023

cherniavskii Jun 1, 2023

romgrk Jun 1, 2023

cherniavskii Jun 1, 2023

romgrk May 31, 2023

cherniavskii Jun 1, 2023

cherniavskii Jun 1, 2023

romgrk Jun 1, 2023

		@@ -0,0 +1,12 @@
		import type { GridRowId } from '../../../models/gridRows';


		visibleRowsLookup[node.id] = isPassingFiltering && areAncestorsExpanded;

		// TODO rows v6: Should we keep storing the visibility status of footer independently or rely on the group visibility in the selector ?

	visibleRowsLookup[node.footerId] =
	isPassingFiltering && areAncestorsExpanded && !!node.childrenExpanded;

[DataGrid] Do not apply filters on rowExpansionChange #8671

[DataGrid] Do not apply filters on rowExpansionChange #8671

Conversation

cherniavskii commented Apr 19, 2023 • edited Loading

Pros:

Cons:

TODO:

mui-bot commented Apr 19, 2023 • edited Loading

Netlify deploy preview

Updated pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joserodolfofreitas left a comment

Choose a reason for hiding this comment

github-actions bot commented May 18, 2023

MBilalShafi commented May 24, 2023 • edited Loading

Another Dimension:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBilalShafi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[DataGrid] Do not apply filters on `rowExpansionChange` #8671

[DataGrid] Do not apply filters on `rowExpansionChange` #8671

cherniavskii commented Apr 19, 2023 •

edited

Loading

mui-bot commented Apr 19, 2023 •

edited

Loading

MBilalShafi commented May 24, 2023 •

edited

Loading