refactor(plugin-chart-table): migrate to API v1 #889

ktmud · 2021-01-07T21:35:24Z

🏆 Enhancements

Migrate the table viz to use API v1, along with some refactoring related to QueryContext.

Add typing for post_processing
Allow ChartPlugin to have custom ChartProps
Remove groupby from queryFields as it is deprecated.
Add GenericDataType, the chart data type hints for rendering

This PR is a prerequisite of apache/superset#10270

vercel · 2021-01-07T21:35:29Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/6b34umm1l
✅ Preview: https://superset-ui-git-fork-ktmud-table-use-api-v1.superset.vercel.app

packages/superset-ui-core/src/query/extractQueryFields.ts

packages/superset-ui-core/src/query/types/PostProcessing.ts

simcha90 · 2021-01-11T06:18:44Z

packages/superset-ui-core/src/utils/removeDuplicates.ts

+    }
+  });
+  return result;
+}


may be only Set enougph? if I got correctly logic do it like (but not sure):

items.forEach(x => { if (!hash || !hash(x)); seen.add(x); } }); return [...seen]

Does set spread preserve orders?

MDN says yes, but this still wouldn't work for my needs because I need to check uniqueness by hash, not the item itself.

ideas: can short circuit when hash is not used and only check hash once.

function removeDuplicates<T>(items: T[], hash?: (item: T) => unknown): T[] { if (hash) { const seen = new Set(); return items.filter(x => { const itemHash = hash(x); if (seen.has(itemHash)) return false; seen.add(itemHash); return true; }); } return [...new Set(items)]; }

Good idea! Updated.

codecov · 2021-01-11T07:12:23Z

Codecov Report

Merging #889 (7f2c636) into master (a5ab4c6) will increase coverage by 0.09%.
The diff coverage is 68.06%.

@@            Coverage Diff             @@
##           master     #889      +/-   ##
==========================================
+ Coverage   27.04%   27.14%   +0.09%     
==========================================
  Files         407      410       +3     
  Lines        8241     8264      +23     
  Branches     1119     1125       +6     
==========================================
+ Hits         2229     2243      +14     
- Misses       5885     5893       +8     
- Partials      127      128       +1

Impacted Files	Coverage Δ
...registries/ChartTransformPropsRegistrySingleton.ts	`100.00% <ø> (ø)`
.../superset-ui-core/src/query/types/QueryFormData.ts	`100.00% <ø> (ø)`
...acy-preset-chart-big-number/src/BigNumber/index.ts	`0.00% <ø> (ø)`
...t-chart-big-number/src/BigNumber/transformProps.ts	`52.00% <ø> (ø)`
...reset-chart-big-number/src/BigNumberTotal/index.ts	`0.00% <ø> (ø)`
.../plugin-chart-echarts/src/Timeseries/buildQuery.ts	`50.00% <0.00%> (-7.15%)`	⬇️
...ins/plugin-chart-table/src/DataTable/DataTable.tsx	`46.15% <0.00%> (-1.47%)`	⬇️
plugins/plugin-chart-table/src/buildQuery.ts	`0.00% <0.00%> (ø)`
plugins/plugin-chart-table/src/controlPanel.tsx	`38.46% <ø> (ø)`
plugins/plugin-chart-table/src/index.ts	`0.00% <ø> (ø)`
... and 20 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 a5ab4c6...7f2c636. Read the comment docs.

kristw

Just a few things here and there. Overall lgtm

kristw · 2021-01-26T07:28:58Z

packages/superset-ui-core/src/query/extractQueryFields.ts

+    if (normalizedKey === 'groupby') {
+      normalizedKey = 'columns';
+    }
+    if (normalizedKey === 'metrics') {


nit else if or can also use switch

I try to avoid switch nowadays since Python has taught me switch is just a syntactic sugar for if else's. Plus I hate it when I forgot to call break.

kristw · 2021-01-26T08:00:57Z

packages/superset-ui-core/src/utils/removeDuplicates.ts

+    }
+  });
+  return result;
+}


ideas: can short circuit when hash is not used and only check hash once.

function removeDuplicates<T>(items: T[], hash?: (item: T) => unknown): T[] { if (hash) { const seen = new Set(); return items.filter(x => { const itemHash = hash(x); if (seen.has(itemHash)) return false; seen.add(itemHash); return true; }); } return [...new Set(items)]; }

kristw · 2021-01-26T08:14:07Z

plugins/plugin-chart-table/test/tsconfig.json

@@ -3,7 +3,7 @@
    "composite": false,
    "emitDeclarationOnly": false,
    "noEmit": true,
-    "rootDir": "."
+    "rootDir": "../../../"


Is this intentional?

Yes as I need to import a contextualized enzyme mount from root: https://github.com/ktmud/superset-ui/blob/8e744a77751fc8863522c8938974262e7e3463bd/plugins/plugin-chart-table/test/TableChart.test.tsx#L21

kristw · 2021-01-26T08:17:00Z

packages/superset-ui-core/src/chart/models/ChartPlugin.ts

-    getChartComponentRegistry().registerLoader(key as string, this.loadChart);
-    getChartControlPanelRegistry().registerValue(key as string, this.controlPanel);
-    getChartTransformPropsRegistry().registerLoader(key as string, this.loadTransformProps);
+    const key: string = this.config.key || isRequired('config.key');


Given we are all in with typescript now. Could also remove the legacy isRequired trick that was before the age of typescript. (No need to do in this pr).

Noted. Let's do this when all major plugins (at least their index.js file) are migrated to typescript.

villebro

This is amazing stuff, thanks so much for all the improvements here. Let's get this in and deal with any potential issues with higi priority, as I feel the improvements here far outweigh any potential regressions caused by it.

villebro · 2021-01-26T09:49:32Z

packages/superset-ui-core/test/utils/removeDuplicates.test.ts

+  it('should remove duplciages from a simple list', () => {
+    expect(removeDuplicates([1, 2, 4, 1, 1, 5, 2])).toEqual([1, 2, 4, 5]);
+  });
+  it('should remove duplciages by key getter', () => {
+    expect(removeDuplicates([{ a: 1 }, { a: 1 }, { b: 2 }], x => x.a)).toEqual([
+      { a: 1 },
+      { b: 2 },
+    ]);
+  });


Suggested change

it('should remove duplciages from a simple list', () => {

expect(removeDuplicates([1, 2, 4, 1, 1, 5, 2])).toEqual([1, 2, 4, 5]);

});

it('should remove duplciages by key getter', () => {

expect(removeDuplicates([{ a: 1 }, { a: 1 }, { b: 2 }], x => x.a)).toEqual([

{ a: 1 },

{ b: 2 },

]);

});

it('should remove duplicates from a simple list', () => {

expect(removeDuplicates([1, 2, 4, 1, 1, 5, 2])).toEqual([1, 2, 4, 5]);

});

it('should remove duplicates by key getter', () => {

expect(removeDuplicates([{ a: 1 }, { a: 1 }, { b: 2 }], x => x.a)).toEqual([

{ a: 1 },

{ b: 2 },

]);

});

ktmud

Adding more comments and replies

ktmud · 2021-01-26T10:43:26Z

packages/superset-ui-core/src/query/types/QueryFormData.ts

@@ -149,7 +154,7 @@ export interface DruidFormData extends BaseFormData {
  druid_time_origin?: string;
 }

-export type QueryFormData = SqlaFormData | DruidFormData;
+export type QueryFormData = DruidFormData | SqlaFormData;


Switching order so if something doesn't add up, it will fallback to SqlaFormData

ktmud · 2021-01-26T10:43:57Z

packages/superset-ui-core/src/utils/removeDuplicates.ts

+    }
+  });
+  return result;
+}


Good idea! Updated.

ktmud · 2021-01-26T10:44:59Z

packages/superset-ui-core/test/utils/removeDuplicates.test.ts

+  it('should remove duplciages from a simple list', () => {
+    expect(removeDuplicates([1, 2, 4, 1, 1, 5, 2])).toEqual([1, 2, 4, 5]);
+  });
+  it('should remove duplciages by key getter', () => {
+    expect(removeDuplicates([{ a: 1 }, { a: 1 }, { b: 2 }], x => x.a)).toEqual([
+      { a: 1 },
+      { b: 2 },
+    ]);
+  });


ktmud · 2021-01-26T10:48:55Z

plugins/plugin-chart-table/src/transformProps.ts

      metrics: metrics_,
-      percentMetrics: percentMetrics_,
+      percent_metrics: percentMetrics_,


I'm just sick and tire of worrying about casing so I choose to use raw form data here. Should probably gradually deprecating this auto-case-transformation for the final charting API as well...

ktmud · 2021-01-26T10:50:10Z

plugins/plugin-chart-table/src/types.ts

-  colorPn?: boolean;
-  includeSearch?: boolean;
-  pageLength?: string | number | null; // null means auto-paginate
+export type TableChartFormData = QueryFormData & {


The benefit of not doing case transformation is that we can directly reuse this typing for both formData sent to the API and received by the chart renderer (e.g. in both buildQuery and transformProps.)

ktmud · 2021-01-26T10:58:05Z

This is amazing stuff, thanks so much for all the improvements here. Let's get this in and deal with any potential issues with higi priority, as I feel the improvements here far outweigh any potential regressions caused by it.

Thank you, @villebro ! I've tested ECharts, NVD3, Box plot, Word Cloud and tried to be as comprehensive as I can. Also wanted to add the missing Cypress tests for more chart types (boxplot, time series table, for example) before merging, but didn't have enough time since a lot of my time has been spent on updating Cypress itself..

It would be super nice if someone from Preset could prioritize adding E2E tests for viz types or completing existing ones with more use cases so we can have more confidence in doing bigger viz plugin improvements like this.

cc @junlincc

ktmud · 2021-01-26T11:01:43Z

I'm going to a new minor version and update apache/superset#10270

ktmud requested a review from a team as a code owner January 7, 2021 21:35

pull-request-size bot added the size/L label Jan 7, 2021

vercel bot had a problem deploying to Preview January 7, 2021 21:35 Failure

ktmud mentioned this pull request Jan 7, 2021

refactor: migrate table chart to new API apache/superset#10270

Merged

6 tasks

vercel bot had a problem deploying to Preview January 7, 2021 23:33 Failure

pull-request-size bot removed the size/L label Jan 8, 2021

vercel bot had a problem deploying to Preview January 8, 2021 02:04 Failure

pull-request-size bot added the size/XL label Jan 8, 2021

ktmud force-pushed the table-use-api-v1 branch from 4faffc4 to addec6e Compare January 9, 2021 08:36

vercel bot had a problem deploying to Preview January 9, 2021 08:36 Failure

vercel bot deployed to Preview January 9, 2021 09:05 View deployment

ktmud mentioned this pull request Jan 10, 2021

[Explore] Saved metrics label overrides not working apache/superset#12375

Closed

3 tasks

ktmud force-pushed the table-use-api-v1 branch from 4d9faf1 to 38640a7 Compare January 10, 2021 06:16

pull-request-size bot removed the size/XL label Jan 10, 2021

vercel bot deployed to Preview January 10, 2021 06:17 View deployment

pull-request-size bot added the size/XXL label Jan 10, 2021

vercel bot deployed to Preview January 10, 2021 06:44 View deployment

ktmud force-pushed the table-use-api-v1 branch 2 times, most recently from 6f10263 to 7fb0819 Compare January 10, 2021 06:47

vercel bot deployed to Preview January 10, 2021 06:53 View deployment

ktmud force-pushed the table-use-api-v1 branch from 7fb0819 to 63a9943 Compare January 11, 2021 01:15

vercel bot deployed to Preview January 11, 2021 01:15 View deployment

vercel bot deployed to Preview January 11, 2021 01:55 View deployment

vercel bot had a problem deploying to Preview January 11, 2021 02:34 Failure

simcha90 reviewed Jan 11, 2021

View reviewed changes

vercel bot deployed to Preview January 11, 2021 07:00 View deployment

ktmud force-pushed the table-use-api-v1 branch from 2225370 to 7f6bea2 Compare January 11, 2021 23:55

vercel bot deployed to Preview January 11, 2021 23:55 View deployment

ktmud force-pushed the table-use-api-v1 branch from 7f6bea2 to 244eeed Compare January 14, 2021 21:50

vercel bot deployed to Preview January 14, 2021 21:50 View deployment

vercel bot deployed to Preview January 14, 2021 23:12 View deployment

vercel bot deployed to Preview January 15, 2021 19:22 View deployment

ktmud mentioned this pull request Jan 15, 2021

fix(plugin-chart-echarts): remove columns from formData #901

Merged

ktmud force-pushed the table-use-api-v1 branch from 5d3f1e2 to 70accce Compare January 15, 2021 21:04

vercel bot deployed to Preview January 15, 2021 21:04 View deployment

ktmud force-pushed the table-use-api-v1 branch from 70accce to 9a4d061 Compare January 20, 2021 19:03

vercel bot deployed to Preview January 20, 2021 19:03 View deployment

kristw approved these changes Jan 26, 2021

View reviewed changes

ktmud added 4 commits January 26, 2021 01:19

refactor(plugin-chart-table): migrate to API v1

bf14235

Gray out null (N/A) values

2f9a092

Upgrade match-sorter

943ab9c

Address PR comments

8ae7f07

ktmud force-pushed the table-use-api-v1 branch from 9a4d061 to 8ae7f07 Compare January 26, 2021 09:20

vercel bot deployed to Preview January 26, 2021 09:20 View deployment

villebro approved these changes Jan 26, 2021

View reviewed changes

Fix typo

7f2c636

vercel bot deployed to Preview January 26, 2021 10:44 View deployment

ktmud commented Jan 26, 2021

View reviewed changes

ktmud merged commit 57643c9 into apache-superset:master Jan 26, 2021

ktmud deleted the table-use-api-v1 branch January 26, 2021 11:01

ktmud mentioned this pull request Jan 28, 2021

feat(core): add orderby to extractQueryFields #919

Merged

ktmud mentioned this pull request Feb 4, 2021

fix(plugin-chart-table): always sort descending by first metric #935

Merged

ktmud mentioned this pull request Feb 18, 2021

plugin-chart-table not using new API? #859

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(plugin-chart-table): migrate to API v1 #889

refactor(plugin-chart-table): migrate to API v1 #889

ktmud commented Jan 7, 2021 •

edited

Loading

vercel bot commented Jan 7, 2021 •

edited

Loading

simcha90 Jan 11, 2021

ktmud Jan 11, 2021

ktmud Jan 11, 2021

kristw Jan 26, 2021

ktmud Jan 26, 2021

codecov bot commented Jan 11, 2021 •

edited

Loading

kristw left a comment

kristw Jan 26, 2021

ktmud Jan 26, 2021

kristw Jan 26, 2021

kristw Jan 26, 2021

ktmud Jan 26, 2021

kristw Jan 26, 2021

ktmud Jan 26, 2021 •

edited

Loading

villebro left a comment

villebro Jan 26, 2021

ktmud Jan 26, 2021

ktmud left a comment

ktmud Jan 26, 2021

ktmud Jan 26, 2021

ktmud Jan 26, 2021

ktmud Jan 26, 2021

ktmud Jan 26, 2021

ktmud commented Jan 26, 2021 •

edited

Loading

ktmud commented Jan 26, 2021 •

edited

Loading

refactor(plugin-chart-table): migrate to API v1 #889

refactor(plugin-chart-table): migrate to API v1 #889

Conversation

ktmud commented Jan 7, 2021 • edited Loading

vercel bot commented Jan 7, 2021 • edited Loading

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

codecov bot commented Jan 11, 2021 • edited Loading

Codecov Report

kristw 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

ktmud Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud 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

ktmud commented Jan 26, 2021 • edited Loading

ktmud commented Jan 26, 2021 • edited Loading

ktmud commented Jan 7, 2021 •

edited

Loading

vercel bot commented Jan 7, 2021 •

edited

Loading

codecov bot commented Jan 11, 2021 •

edited

Loading

ktmud Jan 26, 2021 •

edited

Loading

ktmud commented Jan 26, 2021 •

edited

Loading

ktmud commented Jan 26, 2021 •

edited

Loading