-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Render columns dynamically on wide tables #7693
Conversation
The issue you're solving might be fixed by my PR here: #7690. If you pull that branch, do you still see the degraded performance? |
return ( | ||
<TooltipWrapper key={key} label="header" tooltip={label}> | ||
<div | ||
style={{ ...style, top: style.top - 4 }} |
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.
Can we add a const above for 4.
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.
Ah, good catch! I'll fix it.
columnCount={orderedColumnKeys.length} | ||
columnWidth={getColumnWidth} | ||
height={rowHeight} | ||
ref={(ref) => { this.container = ref; }} |
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.
Let's pull this out into a const so we aren't creating a new function every time. Comment applies elsewhere.
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.
+1
@etr2460 that's awesome! I'll take a look. I have a feeling that we might still need both solutions combined, since this one didn't solve all the performance problems. And our table when rendered has more than 2 millions pixels of width, so your solution might not be enough. |
Codecov Report
@@ Coverage Diff @@
## master #7693 +/- ##
==========================================
- Coverage 73.92% 65.76% -8.17%
==========================================
Files 106 459 +353
Lines 11444 21914 +10470
Branches 0 2408 +2408
==========================================
+ Hits 8460 14411 +5951
- Misses 2984 7382 +4398
- Partials 0 121 +121
Continue to review full report at Codecov.
|
CATEGORY
Choose one
SUMMARY
With #7625 SQL Lab now displays complex column types as extended columns. This might result in very wide tables that degrade performance when visualizing preview samples or query results.
This PR modifies SQL Lab so that wide tables (greater than 50 columns) are displayed using
react-virtualized
'sGrid
instead of aTable
. This ensures that columns are loaded dynamically as the user scrolls horizontally. The only downside is that the user is no longer able to sort the data by clicking on a column header.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Here's a demo showing the
Grid
component with thequery
table (the table has only 27 columns, but I modified the threshold temporarily in order to test):I also tested with a table with over 300 columns, but I can't share the data.
TEST PLAN
Tested locally (see above). Added unit test.
ADDITIONAL INFORMATION
This removes the ability of sorting on the client side on tables with more than 50 columns.
REVIEWERS
@mistercrunch @khtruong