feat(table-viz): rewrite with react-table and add query mode switch mode #10113

ktmud · 2020-06-19T22:48:23Z

SUMMARY

This PR combines two major updates for the table chart:

Replace its rendering from jquery.DataTables to react-table: feat(plugin-chart-table): rewrite with react-table apache-superset/superset-ui#623
Rearrange the control panel, replace GROUP BY and NOT GROUP BY with a "Query Mode" switch: feat(legacy-table-chart): add query mode switch apache-superset/superset-ui#609 . It was not super clear what do GROUP BY and NOT GROUP BY mean and that they are mutually exclusive. Adding a tab switch should make things a little bit easier to understand.

In addition to these, there are also some smaller refactoring and UX improvements:

Clean up the legacy backend API for TableViz, add support for a new query_mode field.
Refactor control panel config management
Change default customize options for table chart
- Time format now defaults to smartDate, which formats the time column based on time grain
- Page length now defaults to NULL, which automatically add pagination of 200 rows per page when number of cells > 10,000

Must wait for release of new versions of @superset-ui/chart-controls and @superset-ui/plugin-chart-table.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

Manual testing

ADDITIONAL INFORMATION

superset/viz.py

kristw

approve the js part

superset-frontend/src/explore/components/ControlPanelsContainer.jsx

ktmud

@john-bodley @michellethomas would you mind having a look on the python part?

ktmud · 2020-06-23T22:05:12Z

superset-frontend/src/explore/controlUtils.js

  const validators = control.validators;
  if (validators && validators.length > 0) {
    const validatedControl = { ...control };
    const validationErrors = [];
    validators.forEach(f => {
-      const v = f(control.value);
+      const v = f.call(control, control.value, processedState);


Extend the validators API to allow access to processed state and the control config itself (not really in use anywhere yet).

ktmud · 2020-06-23T22:08:32Z

tests/viz_tests.py

@@ -344,26 +344,21 @@ def test_adhoc_filters_overwrite_legacy_filters(self):
        self.assertEqual("(value3 in ('North America'))", query_obj["extras"]["where"])
        self.assertEqual("", query_obj["extras"]["having"])

-    @patch("superset.viz.BaseViz.query_obj")


Not sure why the patch is here, but removing it doesn't hurt.

ktmud · 2020-06-24T02:04:29Z

tests/viz_tests.py

        with self.assertRaises(Exception):
+            test_viz = viz.TableViz(datasource, form_data)


The exception now raises earlier.

ktmud · 2020-06-24T02:05:10Z

tests/viz_tests.py

-        self.assertEqual(["colA", "colB", "__time__"], query_obj["metrics"])
-        self.assertEqual([("__time__", True)], query_obj["orderby"])
+        self.assertEqual(["colA", "colB", "sort_column"], query_obj["metrics"])
+        self.assertEqual([("sort_column", True)], query_obj["orderby"])


Rename so not to be confused with the time column (__timestamp).

superset-frontend/spec/javascripts/explore/exploreActions_spec.js

villebro

Quick first pass. We probably should add query_mode into QueryObject both here and on @superset-ui/query, and use that as the primary means of either grouping or not grouping a query. The plan in SIP-38 is to deprecate the groupby property and only use columns, and then perform grouping only if grouping is requested (we also hope to be introducing non-aggregating metrics soon)

superset-frontend/src/explore/components/Control.jsx

superset/viz.py

ktmud · 2020-06-24T16:23:01Z

We probably should add query_mode into QueryObject both here and on @superset-ui/query

Do you mind if I do this in a separate PR?

villebro · 2020-06-24T16:26:49Z

We probably should add query_mode into QueryObject both here and on @superset-ui/query

Do you mind if I do this in a separate PR?

@ktmud sure thing, just wanted to put the comments here to make sure I don't forget mentioning it. I can also do it, I have an idea of how we can do it with minimal disruption.

superset-frontend/src/explore/components/Control.jsx

Also optimize hot loader for ChartRenderer

codecov-commenter · 2020-06-26T17:21:31Z

Codecov Report

Merging #10113 into master will decrease coverage by 0.16%.
The diff coverage is 98.21%.

@@            Coverage Diff             @@
##           master   #10113      +/-   ##
==========================================
- Coverage   70.57%   70.41%   -0.17%     
==========================================
  Files         594      594              
  Lines       31461    31483      +22     
  Branches     3228     3222       -6     
==========================================
- Hits        22205    22170      -35     
- Misses       9140     9198      +58     
+ Partials      116      115       -1

Flag	Coverage Δ
#cypress	`52.80% <74.24%> (-0.77%)`	⬇️
#javascript	`59.42% <80.88%> (-0.19%)`	⬇️
#python	`70.40% <100.00%> (+<0.01%)`	⬆️

Impacted Files	Coverage Δ
...et-frontend/src/explore/controlPanels/sections.jsx	`100.00% <ø> (ø)`
...uperset-frontend/src/views/chartList/ChartList.tsx	`61.22% <ø> (ø)`
...-frontend/src/visualizations/presets/MainPreset.js	`100.00% <ø> (ø)`
superset-frontend/src/welcome/DashboardTable.jsx	`75.00% <ø> (ø)`
superset-frontend/src/chart/ChartRenderer.jsx	`78.37% <83.33%> (+0.11%)`	⬆️
superset-frontend/src/explore/controlUtils.js	`96.19% <96.42%> (-0.72%)`	⬇️
...ontend/src/components/ListView/TableCollection.tsx	`93.10% <100.00%> (ø)`
...perset-frontend/src/explore/components/Control.jsx	`100.00% <100.00%> (ø)`
.../src/explore/components/ControlPanelsContainer.jsx	`92.75% <100.00%> (+14.49%)`	⬆️
...et-frontend/src/explore/reducers/exploreReducer.js	`35.71% <100.00%> (+3.63%)`	⬆️
... and 12 more

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 9de9e1c...d5ab573. Read the comment docs.

ktmud · 2020-06-26T17:24:54Z

@ktmud I tried restarting this a few times, but it appears some unit tests are failing. Cypress seems to be fine, despite being red now (flaky test).

Rebased and fixed another issue with js-dom upgrade and now it seems fine?

1, Replace table chart rendering from jquery.DataTables to react-table: apache-superset/superset-ui#623 2. Rearrange the control panel, replace GROUP BY and NOT GROUP BY with a "Query Mode" switch: apache-superset/superset-ui#609

pull-request-size bot added the size/XL label Jun 19, 2020

ktmud mentioned this pull request Jun 22, 2020

feat(legacy-table-chart): add query mode switch apache-superset/superset-ui#609

Merged

ktmud force-pushed the table-control branch from e3b2372 to e1e16ae Compare June 22, 2020 23:10

pull-request-size bot added size/L and removed size/XL labels Jun 22, 2020

ktmud force-pushed the table-control branch from e1e16ae to f146b1e Compare June 23, 2020 02:07

ktmud commented Jun 23, 2020

View reviewed changes

superset/viz.py Show resolved Hide resolved

ktmud force-pushed the table-control branch 3 times, most recently from 1b2eaac to ff486d1 Compare June 23, 2020 18:23

ktmud marked this pull request as ready for review June 23, 2020 20:15

ktmud force-pushed the table-control branch from ff486d1 to 67292b9 Compare June 23, 2020 20:18

kristw approved these changes Jun 23, 2020

View reviewed changes

ktmud commented Jun 23, 2020

View reviewed changes

superset-frontend/src/explore/components/ControlPanelsContainer.jsx Show resolved Hide resolved

ktmud force-pushed the table-control branch 5 times, most recently from c01fd1e to 0c4ac2b Compare June 24, 2020 02:23

ktmud commented Jun 24, 2020

View reviewed changes

superset-frontend/spec/javascripts/explore/exploreActions_spec.js Show resolved Hide resolved

villebro reviewed Jun 24, 2020

View reviewed changes

superset-frontend/src/explore/components/Control.jsx Show resolved Hide resolved

superset/viz.py Outdated Show resolved Hide resolved

ktmud force-pushed the table-control branch 5 times, most recently from d845c07 to c3b765f Compare June 24, 2020 18:41

rusackas reviewed Jun 24, 2020

View reviewed changes

superset-frontend/src/explore/components/Control.jsx Outdated Show resolved Hide resolved

ktmud added 4 commits June 26, 2020 10:05

feat: upgrade react-table and table viz

bf7e942

Also optimize hot loader for ChartRenderer

feat(viz): add query mode switch to table chart

dd5d434

Run npm dedupe to simplify npm-lock.json

f1739d7

Upgrade jest and jsdom

d5ab573

ktmud force-pushed the table-control branch from a2f4da4 to d5ab573 Compare June 26, 2020 17:11

Disable a buggy a11y linting rule

a9fbafb

ktmud force-pushed the table-control branch from 413e4f5 to a9fbafb Compare June 26, 2020 17:45

ktmud added 2 commits June 26, 2020 10:52

Run npm dedupe && npm ci

ef4f3dd

try to fix a flaky test

a77490f

villebro approved these changes Jun 29, 2020

View reviewed changes

ktmud changed the title ~~feat(viz): add query mode switch for table chart~~ feat(viz): add query mode switch to table chart Jun 29, 2020

ktmud merged commit 9bdfa05 into apache:master Jun 29, 2020

ktmud deleted the table-control branch June 29, 2020 04:37

ktmud mentioned this pull request Jun 30, 2020

fix: datatable dependencies for pivot-table in dev #10205

Merged

6 tasks

dpgaspar mentioned this pull request Jul 1, 2020

Explore option in viz shows "Empty Query" error #10215

Closed

3 tasks

ktmud mentioned this pull request Jul 1, 2020

fix: better backward compatibility for table viz #10219

Merged

6 tasks

nytai mentioned this pull request Jul 2, 2020

fix(listview): use disableSortBy to disable sorting in table columns #10223

Merged

6 tasks

This was referenced Jul 2, 2020

fix: chart validation error not cleared on control value update #10224

Merged

fix(table-viz): format non-numeric metrics #10234

Merged

feat(viz): refine table chart, remove jquery.DataTables #10040

Closed

ktmud mentioned this pull request Jul 9, 2020

fix: optimize mapStateToProps for chart controls #10264

Merged

6 tasks

mistercrunch added the changelog-highlight label Jul 16, 2020

ktmud changed the title ~~feat(viz): add query mode switch to table chart~~ feat(table-viz): rewrite with react-table and add query mode switch mode Jul 27, 2020

ktmud mentioned this pull request Jul 27, 2020

[SIP-50] Proposal for using ECharts as our main charting library #10418

Closed

ktmud mentioned this pull request Aug 7, 2020

fix: table chart query mode initial value #10544

Merged

6 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(table-viz): rewrite with react-table and add query mode switch mode #10113

feat(table-viz): rewrite with react-table and add query mode switch mode #10113

ktmud commented Jun 19, 2020 •

edited

kristw left a comment

ktmud left a comment

ktmud Jun 23, 2020

ktmud Jun 23, 2020

ktmud Jun 24, 2020

ktmud Jun 24, 2020

villebro left a comment

ktmud commented Jun 24, 2020

villebro commented Jun 24, 2020

codecov-commenter commented Jun 26, 2020 •

edited

ktmud commented Jun 26, 2020

		with self.assertRaises(Exception):
		test_viz = viz.TableViz(datasource, form_data)

feat(table-viz): rewrite with react-table and add query mode switch mode #10113

feat(table-viz): rewrite with react-table and add query mode switch mode #10113

Conversation

ktmud commented Jun 19, 2020 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

ADDITIONAL INFORMATION

kristw left a comment

Choose a reason for hiding this comment

ktmud left a comment

Choose a reason for hiding this comment

ktmud Jun 23, 2020

Choose a reason for hiding this comment

ktmud Jun 23, 2020

Choose a reason for hiding this comment

ktmud Jun 24, 2020

Choose a reason for hiding this comment

ktmud Jun 24, 2020

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

ktmud commented Jun 24, 2020

villebro commented Jun 24, 2020

codecov-commenter commented Jun 26, 2020 • edited

Codecov Report

ktmud commented Jun 26, 2020

ktmud commented Jun 19, 2020 •

edited

codecov-commenter commented Jun 26, 2020 •

edited