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

perf(ui): virtualized schema table rows #6287

Merged
merged 21 commits into from
Nov 14, 2022
Merged

perf(ui): virtualized schema table rows #6287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
94e52c5
perf(ui): virtualized schema table for better performance when render…
stanbaker Oct 25, 2022
c1d5164
Merge branch 'master' into master
stanbaker Oct 25, 2022
a3ef0fd
deps: update antd/rc-table for access to showExpandColumn prop and ad…
stanbaker Oct 27, 2022
5eb0d48
fix: add back foreign key toggle functionality and fix scroll issues
stanbaker Oct 27, 2022
a446d42
Merge branch 'master' into master
stanbaker Oct 27, 2022
6c0f236
fix: avoid mutating previous expanded row state directly
stanbaker Oct 27, 2022
9c46886
fix: disable hover events causing rerender on ingestion source table …
stanbaker Oct 28, 2022
780624f
chore: remove eslint-disable
stanbaker Oct 28, 2022
efed5dc
Merge branch 'master' of github.com:datahub-project/datahub
stanbaker Oct 31, 2022
864b39a
Merge branch 'master' into master
stanbaker Nov 1, 2022
e923db6
Revert "chore: remove eslint-disable"
stanbaker Nov 6, 2022
4861a80
Revert "fix: disable hover events causing rerender on ingestion sourc…
stanbaker Nov 6, 2022
51ffd08
Revert "fix: avoid mutating previous expanded row state directly"
stanbaker Nov 6, 2022
ce48943
Revert "fix: add back foreign key toggle functionality and fix scroll…
stanbaker Nov 6, 2022
73f6476
Revert "deps: update antd/rc-table for access to showExpandColumn pro…
stanbaker Nov 6, 2022
70d422c
Revert "perf(ui): virtualized schema table for better performance whe…
stanbaker Nov 6, 2022
115d513
Merge branch 'master' of github.com:datahub-project/datahub
stanbaker Nov 6, 2022
79cb7f1
fix(ui): replace virtuallist-antd for virtualized-table-for-antd in o…
stanbaker Nov 7, 2022
74caff3
Merge branch 'master' into master
stanbaker Nov 11, 2022
3530169
test(ui): add jest mock for table virtualization module
stanbaker Nov 13, 2022
88318c9
chore(ui): disable eslint for require import in test
stanbaker Nov 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions datahub-web-react/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
"styled-components": "^5.2.1",
"typescript": "^4.1.3",
"uuid": "^8.3.2",
"virtualizedtableforantd4": "^1.2.1",
"vx": "^0.0.1",
"web-vitals": "^0.2.4",
"yamljs": "^0.3.0"
Expand Down
9 changes: 9 additions & 0 deletions datahub-web-react/src/app/entity/dataset/profile/__tests__/Schema.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ import { SchemaTab } from '../../../shared/tabs/Dataset/Schema/SchemaTab';
import EntityContext from '../../../shared/EntityContext';
import { EntityType, SchemaMetadata } from '../../../../../types.generated';

jest.mock('virtualizedtableforantd4', () => {
/* eslint-disable-next-line */
const { SchemaRow } = require('../../../shared/tabs/Dataset/Schema/components/SchemaRow');
return {
...jest.requireActual('virtualizedtableforantd4'),
useVT: () => [{ body: { row: SchemaRow } }, jest.fn()],
};
});

describe('Schema', () => {
it('renders', () => {
const { getByText } = render(
Expand Down
5 changes: 4 additions & 1 deletion datahub-web-react/src/app/entity/shared/components/styled/StyledTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import styled from 'styled-components';
import { Table } from 'antd';
import styled from 'styled-components';
import { ANTD_GRAY } from '../../constants';

export const StyledTable = styled(Table)`
overflow: inherit;
height: inherit;

&&& .ant-table-cell {
background-color: #fff;
}
Expand Down
74 changes: 41 additions & 33 deletions datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/SchemaTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import React, { useEffect, useMemo, useState } from 'react';
import { ColumnsType } from 'antd/es/table';
import { useVT } from 'virtualizedtableforantd4';
import ResizeObserver from 'rc-resize-observer';
import styled from 'styled-components';
import {} from 'antd';
import {
EditableSchemaMetadata,
ForeignKeyConstraint,
Expand All @@ -24,6 +25,9 @@ import { ANTD_GRAY } from '../../../constants';
import MenuColumn from './components/MenuColumn';

const TableContainer = styled.div`
overflow: inherit;
height: inherit;

&&& .ant-table-tbody > tr > .ant-table-cell-with-append {
border-right: none;
padding: 0px;
Expand Down Expand Up @@ -51,6 +55,7 @@ export type Props = {
};

const EMPTY_SET: Set<string> = new Set();
const TABLE_HEADER_HEIGHT = 52;

export default function SchemaTable({
rows,
Expand All @@ -64,7 +69,7 @@ export default function SchemaTable({
filterText = '',
}: Props): JSX.Element {
const hasUsageStats = useMemo(() => (usageStats?.aggregations?.fields?.length || 0) > 0, [usageStats]);

const [tableHeight, setTableHeight] = useState(0);
const [tagHoveredIndex, setTagHoveredIndex] = useState<string | undefined>(undefined);
const [selectedFkFieldPath, setSelectedFkFieldPath] =
useState<null | { fieldPath: string; constraint?: ForeignKeyConstraint | null }>(null);
Expand Down Expand Up @@ -206,40 +211,43 @@ export default function SchemaTable({
});
}, [expandedRowsFromFilter]);

const [VT, setVT] = useVT(() => ({ scroll: { y: tableHeight } }), [tableHeight]);

useMemo(() => setVT({ body: { row: SchemaRow } }), [setVT]);

return (
<FkContext.Provider value={selectedFkFieldPath}>
<TableContainer>
<StyledTable
rowClassName={(record) =>
record.fieldPath === selectedFkFieldPath?.fieldPath ? 'open-fk-row' : ''
}
columns={allColumns}
dataSource={rows}
rowKey="fieldPath"
components={{
body: {
row: SchemaRow,
},
}}
Comment on lines -219 to -223
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:
image
and you click on "Foreign Key" you get this:
image

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.

Copy link
Contributor Author

@stanbaker stanbaker Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

expandable={{
expandedRowKeys: [...Array.from(expandedRows)],
defaultExpandAllRows: false,
expandRowByClick: false,
expandIcon: ExpandIcon,
onExpand: (expanded, record) => {
if (expanded) {
setExpandedRows((previousRows) => new Set(previousRows.add(record.fieldPath)));
} else {
setExpandedRows((previousRows) => {
previousRows.delete(record.fieldPath);
return new Set(previousRows);
});
}
},
indentSize: 0,
}}
pagination={false}
/>
<ResizeObserver onResize={(dimensions) => setTableHeight(dimensions.height - TABLE_HEADER_HEIGHT)}>
<StyledTable
rowClassName={(record) =>
record.fieldPath === selectedFkFieldPath?.fieldPath ? 'open-fk-row' : ''
}
columns={allColumns}
dataSource={rows}
rowKey="fieldPath"
scroll={{ y: tableHeight }}
components={VT}
expandable={{
expandedRowKeys: [...Array.from(expandedRows)],
defaultExpandAllRows: false,
expandRowByClick: false,
expandIcon: ExpandIcon,
onExpand: (expanded, record) => {
if (expanded) {
setExpandedRows((previousRows) => new Set(previousRows.add(record.fieldPath)));
} else {
setExpandedRows((previousRows) => {
previousRows.delete(record.fieldPath);
return new Set(previousRows);
});
}
},
indentSize: 0,
}}
pagination={false}
/>
</ResizeObserver>
</TableContainer>
</FkContext.Provider>
);
Expand Down
20 changes: 8 additions & 12 deletions datahub-web-react/src/app/entity/shared/tabs/Dataset/Schema/components/SchemaRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,23 +87,19 @@ const ArrowContainer = styled.div`
margin-top: 40px;
`;

export const SchemaRow = ({
children,
className,
'data-row-key': fieldPath,
}: {
children: any;
className: string;
'data-row-key': string;
}) => {
export const SchemaRow = React.forwardRef<HTMLTableRowElement>((props, ref) => {
/* eslint-disable react/prop-types */
const { children, ...rest } = props;
const selectedFk = useContext(FkContext);
const entityRegistry = useEntityRegistry();
const baseEntity = useBaseEntity<GetDatasetQuery>();

return (
<>
<tr className={className}>{children}</tr>
{fieldPath && fieldPath === selectedFk?.fieldPath && (
<tr {...rest} ref={ref}>
{children}
</tr>
{selectedFk?.fieldPath && props['data-row-key'] === selectedFk?.fieldPath && (
<ForeignKeyContent>
<ForiegnKeyTd>
<HeaderContent>
Expand Down Expand Up @@ -151,4 +147,4 @@ export const SchemaRow = ({
)}
</>
);
};
});
5 changes: 5 additions & 0 deletions datahub-web-react/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16956,6 +16956,11 @@ vfile@^4.0.0:
unist-util-stringify-position "^2.0.0"
vfile-message "^2.0.0"

virtualizedtableforantd4@^1.2.1:
version "1.2.1"
resolved "https://registry.yarnpkg.com/virtualizedtableforantd4/-/virtualizedtableforantd4-1.2.1.tgz#331e8d2f203cdee6667cb5b9cbd7af823f99f65a"
integrity sha512-Hl21jF3WZESanz/iKIjvbjeZ5gGX2t85h2cWQFJAagOQnN7t/pvC4kXhfYNseJtaiU6QHOm5RgX3ud+oXeST1Q==

vm-browserify@^1.0.1:
version "1.1.2"
resolved "https://registry.yarnpkg.com/vm-browserify/-/vm-browserify-1.1.2.tgz#78641c488b8e6ca91a75f511e7a3b32a86e5dda0"
Expand Down