Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DataGrid] Filtering performance: V7 API #9254

Merged
merged 46 commits into from
Jun 28, 2023
Merged

[DataGrid] Filtering performance: V7 API #9254

merged 46 commits into from
Jun 28, 2023

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Jun 7, 2023

Includes points 1 & 9 from #9120.

@romgrk romgrk mentioned this pull request Jun 7, 2023
Closed
@mui-bot
Copy link

mui-bot commented Jun 7, 2023 •
edited
Loading

Netlify deploy preview

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

Updated pages

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 322.6 623.5 425.5 455.8 106.494
Sort 100k rows ms 537.5 1,154.3 537.5 878.26 210.621
Select 100k rows ms 205.5 388.4 232.8 268.54 66.52
Deselect 100k rows ms 164.6 485 236.9 258.84 117.62

Generated by 🚫 dangerJS against ffa0250

@romgrk romgrk added performance component: data grid This is the name of the generic UI component, not the React module! labels Jun 7, 2023
@romgrk
Copy link
Contributor Author

romgrk commented Jun 7, 2023 •
edited
Loading

I have a few small bugs to hunt down but the overall architectural changes match what I understood from our discussions in the experiment PR, so this is ready-ish for review.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged labels Jun 8, 2023
MBilalShafi
MBilalShafi reviewed Jun 9, 2023
View reviewed changes
Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting the larger PR.
The plan is to merge the PR now and get rid of the v6 code with v7, right?

packages/grid/x-data-grid-premium/src/hooks/features/aggregation/wrapColumnWithAggregation.tsx Outdated Show resolved Hide resolved
packages/grid/x-data-grid-premium/src/hooks/features/rowGrouping/createGroupingColDef.tsx Show resolved Hide resolved
packages/grid/x-data-grid-premium/src/hooks/features/rowGrouping/gridRowGroupingUtils.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 9, 2023
@mui mui deleted a comment from github-actions bot Jun 9, 2023
@mui mui deleted a comment from github-actions bot Jun 9, 2023
@romgrk
Copy link
Contributor Author

romgrk commented Jun 9, 2023

Thanks for splitting the larger PR. The plan is to merge the PR now and get rid of the v6 code with v7, right?

Yes, we get rid of code at v7.

@romgrk romgrk requested a review from MBilalShafi June 9, 2023 13:23
@romgrk romgrk requested a review from cherniavskii June 21, 2023 02:14
cherniavskii
cherniavskii approved these changes Jun 21, 2023
View reviewed changes
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

packages/grid/x-data-grid/src/hooks/features/filter/gridFilterUtils.ts Show resolved Hide resolved
packages/grid/x-data-grid/src/hooks/features/filter/useGridFilter.tsx Outdated Show resolved Hide resolved
packages/grid/x-data-grid/src/models/api/gridParamsApi.ts Outdated Show resolved Hide resolved
packages/grid/x-data-grid/src/models/api/gridParamsApi.ts Outdated Show resolved Hide resolved
packages/grid/x-data-grid/src/models/gridFilterOperator.ts Outdated Show resolved Hide resolved
@romgrk
Copy link
Contributor Author

romgrk commented Jun 28, 2023 •
edited
Loading

@cherniavskii I've finished linting and adding tests for filters & quick filters, if you wanna give this another look go ahead.

cherniavskii
cherniavskii approved these changes Jun 28, 2023
View reviewed changes
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

packages/grid/x-data-grid/src/hooks/features/rows/gridRowsInterfaces.ts Outdated Show resolved Hide resolved
@cherniavskii
Copy link
Member

I've opened #9511 to skip flaky tests in JSDOM.
Feel free to ignore these failed flaky tests and merge this PR

@cherniavskii cherniavskii added the feature: Filtering Related to the data grid Filtering feature label Jun 28, 2023
@romgrk romgrk merged commit 4f7e1db into mui:master Jun 28, 2023
@romgrk romgrk deleted the perf-filtering-v7-filters branch June 28, 2023 20:25
@mAllenBond mAllenBond mentioned this pull request Jul 5, 2023
Closed
2 tasks
@romgrk romgrk mentioned this pull request Jul 14, 2023
Closed
2 tasks
@cherniavskii cherniavskii mentioned this pull request Aug 1, 2023
Closed
2 tasks
@romgrk romgrk mentioned this pull request Aug 3, 2023
Open
2 tasks
@romgrk romgrk mentioned this pull request Aug 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cherniavskii cherniavskii cherniavskii approved these changes

@MBilalShafi MBilalShafi Awaiting requested review from MBilalShafi

Assignees
No one assigned
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Filtering Related to the data grid Filtering feature performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@romgrk @mui-bot @cherniavskii @MBilalShafi