[SQL Lab] Improve result table rendering performance #7690

etr2460 · 2019-06-11T17:13:42Z

SUMMARY

When rendering the FilterableTable component we were rendering and measuring the width of every cell to determine the width of each column. For tables with many columns or columns with large amounts of content, this would hang the page for seconds at a time:

This fix changes the font for the FilterableTable to monospaced so that we can calculate the max width of a column without rendering the each cell. This resulted in a 665x improvement in perf for the function (>3 s down to 4.8 ms in some nasty edge cases)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TEST PLAN

Ensure the column widths still fit all the content. Pass CI

ADDITIONAL INFORMATION

REVIEWERS

@graceguo-supercat @soboko @mistercrunch @john-bodley

betodealmeida

This is awesome, I think it will greatly improve with the wide tables we've been previewing. I have just a comment on the font, we should have a designer chime in, since the new font is much harder to read IMHO.

betodealmeida · 2019-06-11T23:05:50Z

superset/assets/src/components/FilterableTable/FilterableTableStyles.css

@@ -43,6 +43,11 @@
  border-right: 1px solid #ccc;
  align-self: center;
  padding: 12px;
+  font-family: monospace;


I wonder if we could have a more specific font here first (falling back to monospace if not available), so the results look better? At least in the screenshot you posted the monospaced font seems harder to read than before.

I tested with 'Roboto Mono', monospace and it looks somewhat better IMHO.

We'd need to include roboto mono with the website then, not sure if it's worth adding a dependency there on a font. Let's see what someone more design minded thinks

Not necessarily, it would fall back to monospace if Roboto Mono is not installed.

GitHub uses SFMono-Regular,Consolas,Liberation Mono,Menlo,Courier,monospace;, eg.

Different fonts would have different character widths, which would make this even harder to implement though..

betodealmeida · 2019-06-11T23:13:21Z

superset/assets/src/components/FilterableTable/FilterableTable.jsx

-  return getTextDimension({ text, style: { font } }).width;
-}
-
+const CHARACTER_WIDTH = 8;


@etr2460, can you test your modifications with Unicode data? You can try a query like this:

SELECT 'ｱｲｳ1234', 'ＱＲＳ12', '台北1234';

My concern is that strings with the same number of characters may have different widths when rendering in monospace. For example, these 3 strings all have 3 characters:

'ｱｲｳ', '123', 'ＱＲＳ'

Note that they're rendered with different widths, since some characters are half-width and others are double.

>>> a = ['123', 'ｱｲｳ', 'ＱＲＳ'] >>> [len(i) for i in a] [3, 3, 3]

Ah, I see your point here. It is possible to get the columns to overflow with enough double wide characters, but in that case you can still hover over the column to see the full results, or copy/paste to the same effect.

betodealmeida · 2019-06-12T20:08:53Z

@etr2460 I'm doing some tests trying to optimize getTextDimension to avoid modifying the DOM, reusing the nodes instead of creating them from scratch all the time.

etr2460 · 2019-06-13T00:17:34Z

@etr2460 I'm doing some tests trying to optimize getTextDimension to avoid modifying the DOM, reusing the nodes instead of creating them from scratch all the time.

That should definitely help performance too! I think separate of the performance question, we should consider if monospace is the right direction to go for the query results. For some types of data, I think it's actually easier to read in monospace, but it probably depends on people's preference

etr2460 · 2019-06-13T00:22:23Z

Design change: @elibrumbaugh do you wanna take a look? Context is above

Before:

After:

elibrumbaugh · 2019-06-13T18:44:55Z

@etr2460 Thanks for the tag. I find the monospace easier to read and it sounds like a big win for performance. My biggest concern is how "spaced" out the letters are now. In design we call this tracking or more recently letter-spacing. Check out this bit about legibility.

I think you should try to reduce the amount of spacing.
Run it past SQL Lab users. You would be surprised how sensitive people can be about this kind of typographic change.

Keep me posted!

elibrumbaugh · 2019-06-13T18:49:27Z

I apologize Erik, I made a mistake. Monospace of course is a fixed width.

I wonder if it's worth the extra space because of the advantage of spacing out compressed syntax per the link I shared in this comment.

etr2460 · 2019-06-13T20:08:41Z

@elibrumbaugh spacing out JSON is definitely a perk for using a monospace font here. I'll try to find some folks who use sql lab on a daily basis for feedback!

betodealmeida · 2019-06-13T20:48:35Z

Personally I prefer the old style, but I've learned to trust designers in cases like this. If @elibrumbaugh says it's better I'm happy to go with it. :)

elibrumbaugh · 2019-06-14T04:23:49Z

Thanks Beto 🙏We'll def confirm with users first!

mistercrunch · 2019-06-14T05:04:58Z

Side note outside the scope of this PR: I thought before that it could be good to limit/collapse wide columns to say 100 chars by default and allow users to expand it. Maybe a right caret in the column header that widens the column to its full extend?

For json specifically, when detected we could also allow a modal on click that would auto-json-format.

etr2460

@betodealmeida let me know what sorts of perf improvements you get by changing the measurement component. if that fixes it enough, then we can make the table monospaced with better looking fonts, otherwise this is probably fine to merge as is. I spoke with some other users of SQL Lab and they all were ok with the change

etr2460 · 2019-06-14T17:26:43Z

superset/assets/src/components/FilterableTable/FilterableTableStyles.css

@@ -43,6 +43,11 @@
  border-right: 1px solid #ccc;
  align-self: center;
  padding: 12px;
+  font-family: monospace;


Different fonts would have different character widths, which would make this even harder to implement though..

etr2460 · 2019-06-14T17:26:47Z

superset/assets/src/components/FilterableTable/FilterableTable.jsx

-  return getTextDimension({ text, style: { font } }).width;
-}
-
+const CHARACTER_WIDTH = 8;


Ah, I see your point here. It is possible to get the columns to overflow with enough double wide characters, but in that case you can still hover over the column to see the full results, or copy/paste to the same effect.

etr2460 · 2019-08-08T17:50:30Z

Superseded by #8011

[SQL Lab] Improve result table rendering performance

849a695

pull-request-size bot added the size/S label Jun 11, 2019

etr2460 mentioned this pull request Jun 11, 2019

Render columns dynamically on wide tables #7693

Merged

12 tasks

betodealmeida reviewed Jun 11, 2019

View reviewed changes

betodealmeida added the design-review label Jun 11, 2019

betodealmeida reviewed Jun 11, 2019

View reviewed changes

betodealmeida mentioned this pull request Jun 12, 2019

feat: reuse svg element in getTextDimension apache-superset/superset-ui#173

Closed

etr2460 commented Jun 14, 2019

View reviewed changes

etr2460 mentioned this pull request Jun 17, 2019

[SQL Lab] Add JSON modal when clicking on cells with JSON objects #7720

Merged

12 tasks

etr2460 closed this Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SQL Lab] Improve result table rendering performance #7690

[SQL Lab] Improve result table rendering performance #7690

etr2460 commented Jun 11, 2019

betodealmeida left a comment

betodealmeida Jun 11, 2019

betodealmeida Jun 12, 2019

etr2460 Jun 13, 2019

betodealmeida Jun 13, 2019

etr2460 Jun 14, 2019

betodealmeida Jun 11, 2019

etr2460 Jun 13, 2019

betodealmeida Jun 13, 2019 •

edited

Loading

betodealmeida Jun 13, 2019

etr2460 Jun 14, 2019

betodealmeida commented Jun 12, 2019

etr2460 commented Jun 13, 2019

etr2460 commented Jun 13, 2019

elibrumbaugh commented Jun 13, 2019

elibrumbaugh commented Jun 13, 2019

etr2460 commented Jun 13, 2019

betodealmeida commented Jun 13, 2019

elibrumbaugh commented Jun 14, 2019

mistercrunch commented Jun 14, 2019 •

edited

Loading

etr2460 left a comment

etr2460 Jun 14, 2019

etr2460 Jun 14, 2019

etr2460 commented Aug 8, 2019

[SQL Lab] Improve result table rendering performance #7690

[SQL Lab] Improve result table rendering performance #7690

Conversation

etr2460 commented Jun 11, 2019

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

betodealmeida 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida commented Jun 12, 2019

etr2460 commented Jun 13, 2019

etr2460 commented Jun 13, 2019

elibrumbaugh commented Jun 13, 2019

elibrumbaugh commented Jun 13, 2019

etr2460 commented Jun 13, 2019

betodealmeida commented Jun 13, 2019

elibrumbaugh commented Jun 14, 2019

mistercrunch commented Jun 14, 2019 • edited Loading

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 commented Aug 8, 2019

betodealmeida Jun 13, 2019 •

edited

Loading

mistercrunch commented Jun 14, 2019 •

edited

Loading