-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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.
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(explore): virtualized datasource field sections #27625
perf(explore): virtualized datasource field sections #27625
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #27625 +/- ##
==========================================
+ Coverage 67.46% 67.50% +0.03%
==========================================
Files 1910 1911 +1
Lines 74802 74840 +38
Branches 8345 8374 +29
==========================================
+ Hits 50467 50517 +50
+ Misses 22284 22270 -14
- Partials 2051 2053 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the improvement @justinpark!
totalColumns: number; | ||
width: number; | ||
showAllMetrics: boolean; | ||
setShowAllMetrics: (showAll: boolean) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think from the component's viewpoint setShowAllMetrics
should be named showAllMetricsChanged
or onShowAllMetrics
. I know that the parent component will set a state with this value but the component's API shouldn't reflect that. This component can be used elsewhere and the event can be handled differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
`} | ||
`; | ||
|
||
const DatasourcePanelItem: React.FC<Props> = ({ index, style, data }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work extracting the component into its own file and adding tests 👏🏼
/testenv up |
@michael-s-molina Ephemeral environment spinning up at http://34.221.95.247:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left a non-blocking comment about nomenclature.
Ephemeral environment shutdown and build artifacts deleted. |
(cherry picked from commit 38eecfc)
SUMMARY
At Airbnb, we have a dataset that contains > 10k columns and metrics.
Given the size of this dataset, the current rendering of the show all datasource target list is not realistic.
To address this issue, this commit introduces virtualized rendering for the field sections, which significantly improves performance and supports handling large-sized metadata datasets.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
before--virtualized-list.mov
After:
after--virtualized-list.mov
TESTING INSTRUCTIONS
Go to "create a chart" and then
ADDITIONAL INFORMATION