feat: improve loading speed for legacy table chart by ktmud · Pull Request #9234 · apache/superset

ktmud · 2020-03-03T23:14:10Z

SUMMARY

One of the main pain points with the table chart is that it loads too slow for large datasets. This PR upgrades it to an updated version with improved performance. See superset-ui-plugins PR#385 for details.

In addition to upgrade the table chart plugin, this PR also refactors some styling code related to it.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Tested with a ~10,000 rows, 7 columns table in Superset, pagination enabled:

Before

Takes about 6 secs to load.

After

Takes only 3 secs to load!

TEST PLAN

Query a large dataset, increase row limit to a very high number
Feel the difference in reactiveness and loading speed of the table chart

ADDITIONAL INFORMATION

Has associated issue: this does not fix Table filter not working #8273
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

REVIEWERS

@kristw @graceguo-supercat @etr2460 @rusackas

ktmud

This is still a DRAFT. Waiting for apache-superset/superset-ui-plugins#385

ktmud · 2020-03-03T23:31:00Z

This adds a little space between dashboard chart container header and visualizations.

graceguo-supercat · 2020-03-04T21:19:41Z

this change will automatic/force to show pagination on large table? Or it's up to user to select show pagination?
The perf improvement from 6 seconds to 3 seconds, will only help table with pagination, or all table charts?

ktmud · 2020-03-04T21:27:08Z

this change will automatic/force to show pagination on large table? Or it's up to user to select show pagination?
The perf improvement from 6 seconds to 3 seconds, is because the table only show top 50 rows?

No, it will not. This PR only refactors the chart module and disabled a couple of already-broken features. The perf gain mainly comes from other optimizations.

etr2460

a couple comments. Will approve once 0.11.16 exists

codecov-io · 2020-03-05T01:55:00Z

Codecov Report

Merging #9234 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #9234      +/-   ##
==========================================
+ Coverage   58.93%   58.94%   +0.01%     
==========================================
  Files         373      373              
  Lines       12014    12017       +3     
  Branches     2945     2946       +1     
==========================================
+ Hits         7080     7083       +3     
  Misses       4755     4755              
  Partials      179      179

Impacted Files	Coverage Δ
superset-frontend/src/chart/ChartRenderer.jsx	`41.77% <100%> (+2.29%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 000a038...1500051. Read the comment docs.

kristw · 2020-03-06T01:44:35Z

@ktmud Please resolve conflict

kristw · 2020-03-06T01:46:06Z

could use const and ternary

const chartClassName = vizType === 'table' ? `superset-chart-${chartClassName}` : snakeCase(vizType);

Was planning to allow more viz types in the future.

if (vizType === 'table' || vizType === 'another-one' ) {

Ternary makes this code look one-off but we actually have more planned.

you could still use ternary for that when adding more vizType or use set if the list grows longer.

const visSet = new Set([...]); const chartClassName = visSet.has(vizType) ? ... : ... ;

Makes sense, will update.

Oh, I think another reason why I didn't use ternary is that the new class is based on an existing variable. It just feels more natural to override it via if statement than either creating two variables or call snakeCase(vizType) twice.

let chartClassName = snakeCase(vizType); if (vizType === 'table') { chartClassName = `superset-chart-${chartClassName}`; }

vs

let chartClassName = snakeCase(vizType); chartClassName = vizType === 'table' ? `superset-chart-${chartClassName}` : chartClassName;

vs

const chartClassName = vizType === 'table' ? `superset-chart-${snakeCase(vizType)}` : snakeCase(vizType);

Still thinks the if block is more readable... 😬

Upgrade table chart `@superset-ui/legacy-plugins-chart-table` to apache-superset/superset-ui-plugins#385

kristw · 2020-03-06T20:41:07Z

+    // container. It may cause css conflicts as in the case of table chart.
+    // When migrating legacy chart types, we should gradually add the prefix
+    // `superset-chart-` to each one of them.
+    chartClassName =


What do you think of

const snakeCaseVizType = snakeCase(vizType); const chartClassName = vizType === 'table' ? `superset-chart-${snakeCaseVizType}` : snakeCaseVizType;

I generally avoid reassigning variables. NVD3Vis.js is an example of many reassignments that made it very difficult to refactor because order of operations matter a lot.

What do you think of

const snakeCaseVizType = snakeCase(vizType); const chartClassName = vizType === 'table' ? `superset-chart-${snakeCaseVizType}` : snakeCaseVizType;

I like this one.

I generally avoid reassigning variables. NVD3Vis.js is an example of many reassignments that made it very difficult to refactor because order of operations matter a lot.

I think it makes sense to use a combination of both. const should be default, but let can be acceptable in small scopes.

* perf(table-chart): upgrade to 0.11.6 Upgrade table chart `@superset-ui/legacy-plugins-chart-table` to apache-superset/superset-ui-plugins#385 * refactor: use ternary instead of if * fix: rename variables

pull-request-size Bot added the size/S label Mar 3, 2020

ktmud force-pushed the react-legacy-table-chart branch from 0f49084 to 6589e7b Compare March 3, 2020 23:23

ktmud commented Mar 3, 2020

View reviewed changes

ktmud force-pushed the react-legacy-table-chart branch from 6589e7b to 9e7b7b5 Compare March 3, 2020 23:38

ktmud mentioned this pull request Mar 4, 2020

perf: faster legacy table chart apache-superset/superset-ui-plugins#385

Merged

ktmud force-pushed the react-legacy-table-chart branch 2 times, most recently from ccd87b7 to 34d784d Compare March 4, 2020 02:57

ktmud force-pushed the react-legacy-table-chart branch from b423458 to 3cbb1af Compare March 5, 2020 00:10

etr2460 reviewed Mar 5, 2020

View reviewed changes

Comment thread superset/translations/zh/LC_MESSAGES/messages.json Outdated

Comment thread superset-frontend/src/setup/setupApp.ts Outdated

Comment thread superset-frontend/src/chart/ChartRenderer.jsx Outdated

ktmud force-pushed the react-legacy-table-chart branch 3 times, most recently from 27f313b to 75234d1 Compare March 5, 2020 01:44

kristw changed the title ~~Perf: improve loading speed for legacy table chart~~ feat: improve loading speed for legacy table chart Mar 5, 2020

kristw reviewed Mar 6, 2020

View reviewed changes

perf(table-chart): upgrade to 0.11.6

8243b91

Upgrade table chart `@superset-ui/legacy-plugins-chart-table` to apache-superset/superset-ui-plugins#385

ktmud force-pushed the react-legacy-table-chart branch from 75234d1 to 8243b91 Compare March 6, 2020 02:01

refactor: use ternary instead of if

1500051

kristw approved these changes Mar 6, 2020

View reviewed changes

fix: rename variables

632397b

kristw merged commit f784af2 into apache:master Mar 6, 2020

This was referenced Mar 10, 2020

fix: upgrade legacy table chart to 0.11.17 #9269

Merged

fix(table-chart): bump legacy-table-chart to 0.11.18 #9275

Merged

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 First shipped in 0.36.0 labels Feb 28, 2024

Conversation

ktmud commented Mar 3, 2020

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

Uh oh!

ktmud left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ktmud Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

graceguo-supercat commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktmud commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etr2460 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kristw commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristw Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

ktmud Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristw Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

ktmud Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktmud Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristw Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristw Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

ktmud Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

ktmud Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

graceguo-supercat commented Mar 4, 2020 •

edited

Loading

ktmud commented Mar 4, 2020 •

edited

Loading

codecov-io commented Mar 5, 2020 •

edited

Loading

kristw commented Mar 6, 2020 •

edited

Loading

ktmud Mar 6, 2020 •

edited

Loading

ktmud Mar 6, 2020 •

edited

Loading

ktmud Mar 6, 2020 •

edited

Loading

kristw Mar 6, 2020 •

edited

Loading