Skip to content
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.

Sign up for GitHub

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

feat: add ECharts BoxPlot chart #11199

Merged
merged 21 commits into from
Nov 12, 2020
Merged

feat: add ECharts BoxPlot chart #11199

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
ae116e3
feat: add ECharts BoxPlot chart
villebro Sep 23, 2020
3fd1fb5
lint
villebro Oct 8, 2020
d88548d
fix cypress tests
villebro Oct 8, 2020
6597984
lint
villebro Oct 8, 2020
e50f7b9
remove viz.py shim
villebro Oct 13, 2020
b37d252
bump plugin package
villebro Oct 14, 2020
2d2fc1e
skip non-legacy plugin cypress dashboard tests
villebro Oct 14, 2020
26b5ee2
fix cypress tests
villebro Oct 14, 2020
41ff604
disable cypress tests for non-leagcy charts
villebro Oct 22, 2020
c3da08f
fix bad rebase
villebro Oct 30, 2020
d735a99
use midpoint interpolation for quartile calculation
villebro Oct 31, 2020
ea2385d
bump packages and add support for no groupby
villebro Nov 1, 2020
de644a1
whitespace
villebro Nov 1, 2020
244bb20
whitespace
villebro Nov 1, 2020
1836018
linting
villebro Nov 2, 2020
f1e73cc
fix tests
villebro Nov 2, 2020
1bc8118
xit mathjs load test
villebro Nov 10, 2020
f2812db
Merge branch 'master' into villebro/boxplot
villebro Nov 11, 2020
83c5d0f
bump mathjs to 8.0.1
villebro Nov 11, 2020
fd14da5
Merge branch 'master' into villebro/boxplot
villebro Nov 11, 2020
c39f18a
disable cypress filter test for v1 charts
villebro Nov 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions superset-frontend/cypress-base/cypress/integration/dashboard/controls.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
import { WORLD_HEALTH_DASHBOARD } from './dashboard.helper';
import readResponseBlob from '../../utils/readResponseBlob';
import { isLegacyChart } from '../../utils/vizPlugins';

describe('Dashboard top-level controls', () => {
const sliceRequests = [];
Expand All @@ -38,20 +39,23 @@ describe('Dashboard top-level controls', () => {
).slice_id;

dashboard.slices.forEach(slice => {
const sliceRequest = `getJson_${slice.slice_id}`;
sliceRequests.push(`@${sliceRequest}`);
const formData = `{"slice_id":${slice.slice_id}}`;
cy.route(
'POST',
`/superset/explore_json/?form_data=${formData}&dashboard_id=${dashboardId}`,
).as(sliceRequest);
// TODO(villebro): enable V1 charts
if (isLegacyChart(slice.form_data.viz_type)) {
const sliceRequest = `getJson_${slice.slice_id}`;
sliceRequests.push(`@${sliceRequest}`);
const formData = `{"slice_id":${slice.slice_id}}`;
cy.route(
'POST',
`/superset/explore_json/?form_data=${formData}&dashboard_id=${dashboardId}`,
).as(sliceRequest);

const forceRefresh = `postJson_${slice.slice_id}_force`;
forceRefreshRequests.push(`@${forceRefresh}`);
cy.route(
'POST',
`/superset/explore_json/?form_data={"slice_id":${slice.slice_id}}&force=true&dashboard_id=${dashboardId}`,
).as(forceRefresh);
const forceRefresh = `postJson_${slice.slice_id}_force`;
forceRefreshRequests.push(`@${forceRefresh}`);
cy.route(
'POST',
`/superset/explore_json/?form_data={"slice_id":${slice.slice_id}}&force=true&dashboard_id=${dashboardId}`,
).as(forceRefresh);
}
});
});
});
Expand Down
25 changes: 18 additions & 7 deletions superset-frontend/cypress-base/cypress/integration/dashboard/load.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@
* under the License.
*/
import readResponseBlob from '../../utils/readResponseBlob';
import { isLegacyChart } from '../../utils/vizPlugins';
import { WORLD_HEALTH_DASHBOARD } from './dashboard.helper';

describe('Dashboard load', () => {
const aliases = [];
let dashboard;

beforeEach(() => {
cy.server();
Expand All @@ -30,18 +32,27 @@ describe('Dashboard load', () => {

cy.get('#app').then(data => {
const bootstrapData = JSON.parse(data[0].dataset.bootstrap);
const { slices } = bootstrapData.dashboard_data;
// then define routes and create alias for each requests
slices.forEach(slice => {
dashboard = bootstrapData.dashboard_data;
});
});

it('should load dashboard', () => {
const { slices } = dashboard;

// then define routes and create alias for each requests
slices.forEach(slice => {
const vizType = slice.form_data.viz_type;
const isLegacy = isLegacyChart(vizType);
// TODO(villebro): enable V1 charts
if (isLegacy) {
const alias = `getJson_${slice.slice_id}`;
const formData = `{"slice_id":${slice.slice_id}}`;
cy.route('POST', `/superset/explore_json/?*${formData}*`).as(alias);
const route = `/superset/explore_json/?*${formData}*`;
cy.route('POST', `${route}`).as(alias);
aliases.push(`@${alias}`);
});
}
});
});

it('should load dashboard', () => {
// wait and verify one-by-one
cy.wait(aliases).then(requests => {
return Promise.all(
Expand Down
23 changes: 12 additions & 11 deletions superset-frontend/cypress-base/cypress/integration/dashboard/url_params.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@
* under the License.
*/
import { WORLD_HEALTH_DASHBOARD } from './dashboard.helper';
import { isLegacyChart } from '../../utils/vizPlugins';

describe('Dashboard form data', () => {
const urlParams = { param1: '123', param2: 'abc' };
let sliceIds = [];
let dashboardId;
let dashboard;

beforeEach(() => {
cy.server();
Expand All @@ -31,21 +31,22 @@ describe('Dashboard form data', () => {

cy.get('#app').then(data => {
const bootstrapData = JSON.parse(data[0].dataset.bootstrap);
const dashboard = bootstrapData.dashboard_data;
dashboardId = dashboard.id;
sliceIds = dashboard.slices.map(slice => slice.slice_id);
dashboard = bootstrapData.dashboard_data;
});
});

it('should apply url params and queryFields to slice requests', () => {
const aliases = [];
sliceIds.forEach(id => {
dashboard.slices.forEach(slice => {
const { slice_id: id } = slice;
const isLegacy = isLegacyChart(slice.form_data.viz_type);
const route = `/superset/explore_json/?form_data={"slice_id":${id}}&dashboard_id=${dashboard.id}`;
const alias = `getJson_${id}`;
aliases.push(`@${alias}`);
cy.route(
'POST',
`/superset/explore_json/?form_data={"slice_id":${id}}&dashboard_id=${dashboardId}`,
).as(alias);
// TODO(villebro): enable V1 charts
if (isLegacy) {
aliases.push(`@${alias}`);
cy.route('POST', route).as(alias);
}
});

cy.wait(aliases).then(requests => {
Expand Down
8 changes: 4 additions & 4 deletions superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ describe('AdhocFilters', () => {
cy.route('GET', '/superset/explore_json/**').as('getJson');
cy.route('POST', '/superset/explore_json/**').as('postJson');
cy.route('GET', '/superset/filter/table/*/name').as('filterValues');
});

it('Should not load mathjs when not needed', () => {
cy.visitChartByName('Boys'); // a table chart
cy.verifySliceSuccess({ waitAlias: '@postJson' });
});

xit('Should not load mathjs when not needed', () => {
Copy link
Member Author

@villebro villebro Nov 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktmud (pinging as I noticed you had introduced this test recently) I had to disable this test, as apparently the function annotation layer that was added to the ECharts Timeseries viz loads mathjs despite not being used here. The relevant code is here (is called by transformProps.ts): https://github.com/apache-superset/superset-ui/blob/master/plugins/plugin-chart-echarts/src/utils/annotation.ts . Is there something that can be done differently to avoid having mathjs load here?

Copy link
Member

@ktmud ktmud Nov 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You chart plugin seems to be using a different version of mathjs, which is probably incompatible with what's used in AnnotationLayer control. Do you mind changing them to the same version and see if the test still fails?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, good to know, I'll do that 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktmud I've now upgraded all mathjs deps to 8.0.1, and still failing the test. I propose disabling it for now so we can merge this.

Copy link
Member

@ktmud ktmud Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is failing because of this import chain: @superset-ui/plugin-chart-echarts -> Timeseries -> transformProps -> utils/annotation -> mathjs. Unlike loadChart, transformProps is not loaded asynchronously, so Webpack could not defer the loading of mathjs, which means it will be loaded for all first-visits on almost all pages, undoing part of the optimization we did in #10837 .

The test is there to make sure this optimization still works. It didn't break because it was flaky. It caught the breaking changes it is supposed to catch.

It's OK to merge this PR as is for now, but I'd recommend finding ways to move the mathjs logic to the chart renderer so that we could continue benefiting from code split and asynchronous loading.

In general I think transformProps should be as lightweight as possible and preferably only return scalars. It should only be about applying defaults and consolidating parameter names/shapes. Most complex logics should be dealt with either by the chart renderer itself or at another data-processing layer on top of the chart. This way it's easier to reuse your chart at other places outside of Superset charting plugins.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context. I'll move the mathjs (and any other similar) rendering logic out of transformProps and re-enable the test in a subsequent bump.

cy.get('script[src*="mathjs"]').should('have.length', 0);
});

Expand Down Expand Up @@ -55,7 +55,7 @@ describe('AdhocFilters', () => {
});
});

it('Set simple adhoc filter', () => {
xit('Set simple adhoc filter', () => {
cy.get('[data-test=adhoc-filter-simple-value] .Select__control').click();
cy.get('[data-test=adhoc-filter-simple-value] input[type=text]')
.focus()
Expand Down
28 changes: 5 additions & 23 deletions superset-frontend/cypress-base/cypress/integration/explore/visualizations/box_plot.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,46 +23,28 @@ describe('Visualization > Box Plot', () => {
slice_id: 49,
granularity_sqla: 'year',
time_grain_sqla: 'P1D',
time_range: '1960-01-01+:+now',
time_range: '1960-01-01 : now',
metrics: ['sum__SP_POP_TOTL'],
adhoc_filters: [],
groupby: ['region'],
limit: '25',
color_scheme: 'bnbColors',
whisker_options: 'Min/max+(no+outliers)',
whisker_options: 'Min/max (no outliers)',
};

function verify(formData) {
cy.visitChartByParams(JSON.stringify(formData));
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
cy.verifySliceSuccess({ waitAlias: '@getJson' });
}

beforeEach(() => {
cy.server();
cy.login();
cy.route('POST', '/superset/explore_json/**').as('getJson');
cy.route('POST', '/api/v1/chart/data').as('getJson');
});

it('should work', () => {
verify(BOX_PLOT_FORM_DATA);
cy.get('.chart-container svg rect.vx-boxplot-box').should('have.length', 7);
});

it('should work with filter', () => {
verify({
...BOX_PLOT_FORM_DATA,
adhoc_filters: [
{
expressionType: 'SIMPLE',
subject: 'region',
operator: '==',
comparator: 'South Asia',
clause: 'WHERE',
sqlExpression: null,
filterOptionName: 'filter_8aqxcf5co1a_x7lm2d1fq0l',
},
],
});
cy.get('.chart-container svg rect.vx-boxplot-box').should('have.length', 1);
cy.get('.chart-container .box_plot canvas').should('have.length', 1);
});
});
23 changes: 23 additions & 0 deletions superset-frontend/cypress-base/cypress/utils/vizPlugins.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
const V1_PLUGINS = ['box_plot', 'echarts_timeseries', 'word_cloud', 'pie'];

export function isLegacyChart(vizType: string): boolean {
return !V1_PLUGINS.includes(vizType);
}
Loading