perf(ui): virtualized schema table rows #6287

stanbaker · 2022-10-25T23:22:26Z

Resolves performance issues when rendering schema definitions for wide tables with many columns (100+)

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

…ing wide table definitions with many columns.

github-actions · 2022-10-26T17:54:44Z

Unit Test Results (build & test)

613 tests ±0 609 ✔️ ±0 11m 44s ⏱️ ±0s
151 suites ±0     4 💤 ±0
151 files ±0     0 ❌ ±0

Results for commit 88318c9. ± Comparison against base commit 3e907ab.

♻️ This comment has been updated with latest results.

chriscollins3456

this is a great idea! virtualizing this list and improving rendering performance of big tables is definitely something we want to do, thank you for looking into this!

I have some comments about a degradation in missing functionality as well as some unexpected / wonky UI behavior.

Let me know if you want any help as you're working on this because this would be a really great thing to get in!

chriscollins3456 · 2022-10-26T21:37:17Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/SchemaTable.tsx

-                    components={{
-                        body: {
-                            row: SchemaRow,
-                        },
-                    }}


unfortunately removing this will also remove our functionality around showing foreign key entities on clicking them. So when you have a foreign key like this schema:

and you click on "Foreign Key" you get this:

Is there a way to use virtuallist while preserving this schema functionality to use SchemaRow? After playing around with it a bit myself it doesn't seem very easy. So we'd have to change our implementation to still get the same functionality that SchemaRow provides with foreign keys.

Another option is to look at other virtual list libraries that might still allow us to do this. Have you looked at react-window? not sure how easy it would be to use with ant designs table, though.

Hey @chriscollins3456, first of all -- thank you for the thorough review! I'm currently in progress working on getting the foreign key information back to working with the chip toggle.

It looks like you had originally started some work to have rows expand based on search (filterSchemaRows.ts), so I was going to build off of where that is used in the expandable prop for the table itself with selectedFkFieldPath info.

Originally I had tried to implement react-window with autosizer, but was running in to a wall due to how ant-design (really rc-table) expects to render the components overrides vs how react-window prefers to wrap virtualized rows.

I'm making good progress on restoring the FK functionality though and expect to have it pushed tomorrow afternoon after resolving some dependency issues.

amazing! I appreciate the explanation - all that makes total sense to me

chriscollins3456 · 2022-10-26T21:39:48Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/SchemaTable.tsx

-                            row: SchemaRow,
-                        },
-                    }}
+                    scroll={{ y: '100vh', x: '100%' }}


unfortunately adding this scroll causes a double scroll in the table, which is not desired. this is what I'm seeing (a scrollbar inside of a scrollbar)

In order to use the scroll prop here we'll have to rejigger some css elsewhere to make things look nice

i'm also seeing some buggy behavior when you try to grab the scrollbar and drag it and then try to scroll afterward (I'm assuming a bug with ant-design's scroll prop here?)
here's a screen recording of what I'm talking about:

Screen.Recording.2022-10-26.at.5.45.31.PM.mov

finally, on smaller tables, I'm seeing a scrollbar added unnecessarily and cutting off the final row from reaching the bottom:

Ah! Good catch(es). Should be pretty straight forward to fix these 🙂

awesome! thanks so much!

…d resize observer

stanbaker · 2022-10-27T20:00:56Z

@chriscollins3456 here's a summary of the latest changes pushed:

Foreign key information rendering logic has been refactored to use the native expandable prop. Unfortunately a particular subproperty used to turn off rendering the expansion column was not available originally. This required some minor dependency upgrades: antd for its subdependency on rc-table and rc-table itself to avoid type declaration file conflicts. Typescript caught some compatibility issues in other components after the minor version upgrade as well.
Double scroll bar and render jumpiness has been fixed. rc-resize-observer is used to listen for table size changes and adjust the VList window height accordingly.

What's left? Currently I'm seeing two small errors that may require some help from you:

~~Hovering over a record in the IngestionSourceTable throws:~~
```
Uncaught Error: Rendered fewer hooks than expected. This may be caused by an accidental early return statement.
```
This error can be narrowed down to the use of Apollo's useLazyQuery hook to fetch data platform icon information, but I'm not exactly how to get it to stop complaining about the conditional usage (or why it has even started causing trouble to begin with). I've tried switching to useGetDataPlatformQuery instead of the lazy version with skip conditions added but no luck there. (edit: fixed)
Toggling the foreign key row expansion back to a closed state throws a GraphQL error (DataFetchingException) for the getDataset query. What's interesting though is this query only appears to be used in the mocks.

Appreciate the help and feedback

…rows

stanbaker · 2022-10-28T16:33:25Z

Fixed the ingestion table error -- it turns out the newer version of ant-design adds a hover class to each table cell on mouseEnter and re-renders (causing the conditional graphql call to error out). Disabled pointer events (except on the functional buttons) to prevent this behavior.

chriscollins3456

this is looking real close! I tested it on a huge table and the performance improvements are very noticeable!

Unfortunately I noticed some bugs being introduced with the most recent changes and left those in the comments

chriscollins3456 · 2022-11-03T20:43:23Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/SchemaTable.tsx

+                                    });
+                                }
+                            },
+                            expandedRowRender: ForeignKeyRow,


unfortunately with this add and the showExpandColumn: false add, it breaks our ability to view nested schemas. when you set showExpandedColumn: false then we hide the arrow indicating to expand a column to show its nested columns. and then expandedRowRender will then show a broken ForeignKeyComponent instead of the child rows.

When things are normal they should look like this:
not expanded:

expanded:

Another thing I'm seeing with this ForeignKey component is that if the table is small and you expand the ForeignKey to see it all, the table doesn't expand with it and remains very small with a scroll. As an example:

A small table with a foreign key:

After clicking on ForeignKey to expand:

Maybe we can find a different way to display ForeignKeyRow? Maybe along with the actual row renderer?

Also when I close the ForeignKey view, I'm getting an error because we're requesting to getDataset with an empty string as an urn. I believe this is coming from entityRegistry.renderProfile in ForeignKeyRow where we render a dataset profile component that makes this request.

So my guess is that when we close the foreign key component, we're removing the selectedFkFieldPath but the ForeignKeyRow is still mounted, but since selectedFkFieldPath is null, we're passing in an empty string as the urn in renderProfile as you can see here:

{entityRegistry.renderProfile( EntityType.Dataset, selectedFk?.constraint?.foreignDataset?.urn || '', )}

Here's the error I see on closing it:

chriscollins3456 · 2022-11-03T20:48:41Z

datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/components/ForeignKeyRow.tsx

+    margin-top: 40px;
+`;
+
+const ForeignKeyRow = () => {


love the rename!

chriscollins3456 · 2022-11-03T21:13:56Z

I also noticed another few issues. The rows are no longer dynamic in their height (I suppose because we're using VList for components which if you look at the elements tab in your browser dev tools you can see wraps these column cells in white-space: no-wrap; (as you can see in the screenshot)) so if you add multiple tags and terms, they end up getting cut off. If you remove scroll={{ y: tableHeight }} then we get a horizontal scroll to show everything but then these tables could start looking real ugly and nobody enjoys a horizontal scroll lol

On top of that, the same issue applies to field documentation. if the docs are too long, they're now getting cut off and an ellipsis is being added. This along isn't a problem, but the problem is that we then lose the ability to edit descriptions of columns. You can see in the screenshot below that the edit icon is cut off on the user_name column so it's hard to see, and then it doesn't appear at all on the user_id column since it's hidden.

chriscollins3456 · 2022-11-03T21:14:45Z

@stanbaker I'm so sorry this is such a tough frontend situation with so many variables! However we really really appreciate you looking into this :)

This reverts commit 780624f.

…e table rows" This reverts commit 9c46886.

This reverts commit 6c0f236.

… issues" This reverts commit 5eb0d48.

…p and add resize observer" This reverts commit a3ef0fd.

…n rendering wide table definitions with many columns." This reverts commit 94e52c5.

…rder to support custom row rendering

chriscollins3456

this looks amazing Stan! tried it locally and all the edge cases we've discussed are looking good and I'm noticing a huge performance improvement on tables with many rows :) thanks so much for this!

chriscollins3456 · 2022-11-13T21:55:18Z

@stanbaker it looks like unit tests are failing. I took a look into it and our tests around the schema tab are now failing because some things aren't getting rendered with this new virtualized table. After digging in more, the table renders the first row, but not others. I believe there's something wonky going on in the testing library with render size of the table itself, so the items out of the page scroll aren't getting rendered (exactly the point of this PR lol)

I ended up making a few changes to the tests so that they continue testing the same stuff, but with the row that does indeed get rendered. Check out my diff from this PR to incorporate it yourself! #6420

Once that's in and CI is passing, this guy will be merged

…ahub-project#6287)

perf(ui): virtualized schema table for better performance when render…

94e52c5

…ing wide table definitions with many columns.

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Oct 25, 2022

Merge branch 'master' into master

c1d5164

stanbaker changed the title ~~perf(ui): virtualized schema table for better performance when rendering wide table definitions with many columns.~~ perf(ui): virtualized schema table rows Oct 25, 2022

jjoyce0510 requested a review from chriscollins3456 October 26, 2022 17:32

chriscollins3456 reviewed Oct 26, 2022

View reviewed changes

stanbaker and others added 3 commits October 27, 2022 14:32

deps: update antd/rc-table for access to showExpandColumn prop and ad…

a3ef0fd

…d resize observer

fix: add back foreign key toggle functionality and fix scroll issues

5eb0d48

Merge branch 'master' into master

a446d42

stanbaker requested a review from chriscollins3456 October 27, 2022 20:02

stanbaker added 2 commits October 27, 2022 16:12

fix: avoid mutating previous expanded row state directly

6c0f236

fix: disable hover events causing rerender on ingestion source table …

9c46886

…rows

chore: remove eslint-disable

780624f

maggiehays added the hacktoberfest-accepted Acceptance for hacktoberfest https://hacktoberfest.com/participation/ label Oct 28, 2022

stanbaker and others added 2 commits October 31, 2022 08:59

Merge branch 'master' of github.com:datahub-project/datahub

efed5dc

Merge branch 'master' into master

864b39a

chriscollins3456 reviewed Nov 3, 2022

View reviewed changes

stanbaker added 8 commits November 6, 2022 12:08

Revert "chore: remove eslint-disable"

e923db6

This reverts commit 780624f.

Revert "fix: disable hover events causing rerender on ingestion sourc…

4861a80

…e table rows" This reverts commit 9c46886.

Revert "fix: avoid mutating previous expanded row state directly"

51ffd08

This reverts commit 6c0f236.

Revert "fix: add back foreign key toggle functionality and fix scroll…

ce48943

… issues" This reverts commit 5eb0d48.

Revert "deps: update antd/rc-table for access to showExpandColumn pro…

73f6476

…p and add resize observer" This reverts commit a3ef0fd.

Revert "perf(ui): virtualized schema table for better performance whe…

70d422c

…n rendering wide table definitions with many columns." This reverts commit 94e52c5.

Merge branch 'master' of github.com:datahub-project/datahub

115d513

fix(ui): replace virtuallist-antd for virtualized-table-for-antd in o…

79cb7f1

…rder to support custom row rendering

chriscollins3456 approved these changes Nov 11, 2022

View reviewed changes

Merge branch 'master' into master

74caff3

stanbaker added 2 commits November 13, 2022 17:22

test(ui): add jest mock for table virtualization module

3530169

chore(ui): disable eslint for require import in test

88318c9

chriscollins3456 merged commit 7ea9797 into datahub-project:master Nov 14, 2022

stanbaker added a commit to stanbaker/datahub that referenced this pull request Nov 17, 2022

perf(ui): virtualize schema table rows for rendering performance (dat…

21e4d8e

…ahub-project#6287)

cccs-Dustin pushed a commit to CybercentreCanada/datahub that referenced this pull request Feb 1, 2023

perf(ui): virtualize schema table rows for rendering performance (dat…

120950e

…ahub-project#6287)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(ui): virtualized schema table rows #6287

perf(ui): virtualized schema table rows #6287

stanbaker commented Oct 25, 2022 •

edited

Loading

github-actions bot commented Oct 26, 2022 •

edited

Loading

chriscollins3456 left a comment

chriscollins3456 Oct 26, 2022

stanbaker Oct 26, 2022 •

edited

Loading

chriscollins3456 Oct 27, 2022

chriscollins3456 Oct 26, 2022

chriscollins3456 Oct 26, 2022

chriscollins3456 Oct 26, 2022

stanbaker Oct 26, 2022

chriscollins3456 Oct 27, 2022

stanbaker commented Oct 27, 2022 •

edited

Loading

stanbaker commented Oct 28, 2022

chriscollins3456 left a comment

chriscollins3456 Nov 3, 2022

chriscollins3456 Nov 3, 2022

chriscollins3456 Nov 3, 2022

chriscollins3456 Nov 3, 2022

chriscollins3456 commented Nov 3, 2022

chriscollins3456 commented Nov 3, 2022

chriscollins3456 left a comment

chriscollins3456 commented Nov 13, 2022

perf(ui): virtualized schema table rows #6287

perf(ui): virtualized schema table rows #6287

Conversation

stanbaker commented Oct 25, 2022 • edited Loading

Checklist

github-actions bot commented Oct 26, 2022 • edited Loading

Unit Test Results (build & test)

chriscollins3456 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanbaker Oct 26, 2022 • edited Loading

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

stanbaker commented Oct 27, 2022 • edited Loading

stanbaker commented Oct 28, 2022

chriscollins3456 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

chriscollins3456 commented Nov 3, 2022

chriscollins3456 commented Nov 3, 2022

chriscollins3456 left a comment

Choose a reason for hiding this comment

chriscollins3456 commented Nov 13, 2022

stanbaker commented Oct 25, 2022 •

edited

Loading

github-actions bot commented Oct 26, 2022 •

edited

Loading

stanbaker Oct 26, 2022 •

edited

Loading

stanbaker commented Oct 27, 2022 •

edited

Loading