From cbbcc8d2e136f949778cda56affb981c2db05880 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Date: Fri, 14 Apr 2023 20:43:15 +0300 Subject: [PATCH] fix(plugin-chart-echarts): reorder totals and support multimetric sort (#23675) --- .../src/shared-controls/customControls.tsx | 30 +- .../src/MixedTimeseries/transformProps.ts | 4 +- .../src/Timeseries/transformProps.ts | 11 +- .../src/Timeseries/types.ts | 2 + .../plugin-chart-echarts/src/utils/series.ts | 35 ++- .../test/utils/series.test.ts | 282 ++++++++++++------ 6 files changed, 244 insertions(+), 120 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx index 28fbfb876..82ba6dfee 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/customControls.tsx @@ -54,27 +54,29 @@ export const contributionModeControl = { }, }; +function isTemporal(controls: ControlStateMapping): boolean { + return !( + isDefined(controls?.x_axis?.value) && + !isTemporalColumn( + getColumnLabel(controls?.x_axis?.value as QueryFormColumn), + controls?.datasource?.datasource, + ) + ); +} + const xAxisSortVisibility = ({ controls }: { controls: ControlStateMapping }) => - isDefined(controls?.x_axis?.value) && - !isTemporalColumn( - getColumnLabel(controls?.x_axis?.value as QueryFormColumn), - controls?.datasource?.datasource, - ) && - Array.isArray(controls?.groupby?.value) && - controls.groupby.value.length === 0; + !isTemporal(controls) && + ensureIsArray(controls?.groupby?.value).length === 0 && + ensureIsArray(controls?.metrics?.value).length === 1; const xAxisMultiSortVisibility = ({ controls, }: { controls: ControlStateMapping; }) => - isDefined(controls?.x_axis?.value) && - !isTemporalColumn( - getColumnLabel(controls?.x_axis?.value as QueryFormColumn), - controls?.datasource?.datasource, - ) && - Array.isArray(controls?.groupby?.value) && - !!controls.groupby.value.length; + !isTemporal(controls) && + (!!ensureIsArray(controls?.groupby?.value).length || + ensureIsArray(controls?.metrics?.value).length > 1); export const xAxisSortControl = { name: 'x_axis_sort', diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts index 883b4daf9..fee6183a2 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -170,12 +170,12 @@ export default function transformProps( } const rebasedDataA = rebaseForecastDatum(data1, verboseMap); - const rawSeriesA = extractSeries(rebasedDataA, { + const [rawSeriesA] = extractSeries(rebasedDataA, { fillNeighborValue: stack ? 0 : undefined, xAxis: xAxisLabel, }); const rebasedDataB = rebaseForecastDatum(data2, verboseMap); - const rawSeriesB = extractSeries(rebasedDataB, { + const [rawSeriesB] = extractSeries(rebasedDataB, { fillNeighborValue: stackB ? 0 : undefined, xAxis: xAxisLabel, }); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index 5565fee6a..84b97fae5 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -126,6 +126,7 @@ export default function transformProps( logAxis, markerEnabled, markerSize, + metrics, minorSplitLine, onlyTotal, opacity, @@ -193,7 +194,9 @@ export default function transformProps( getMetricLabel, ); - const rawSeries = extractSeries(rebasedData, { + const isMultiSeries = groupby.length || metrics.length > 1; + + const [rawSeries, sortedTotalValues] = extractSeries(rebasedData, { fillNeighborValue: stack && !forecastEnabled ? 0 : undefined, xAxis: xAxisLabel, extraMetricLabels, @@ -202,8 +205,8 @@ export default function transformProps( isHorizontal, sortSeriesType, sortSeriesAscending, - xAxisSortSeries: groupby.length ? xAxisSortSeries : undefined, - xAxisSortSeriesAscending: groupby.length + xAxisSortSeries: isMultiSeries ? xAxisSortSeries : undefined, + xAxisSortSeriesAscending: isMultiSeries ? xAxisSortSeriesAscending : undefined, }); @@ -240,7 +243,7 @@ export default function transformProps( formatter, showValue, onlyTotal, - totalStackedValues, + totalStackedValues: sortedTotalValues, showValueIndexes, thresholdValues, richTooltip, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts index bca13a058..54eaab281 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts @@ -23,6 +23,7 @@ import { ContributionType, QueryFormColumn, QueryFormData, + QueryFormMetric, TimeFormatter, TimeGranularity, } from '@superset-ui/core'; @@ -65,6 +66,7 @@ export type EchartsTimeseriesFormData = QueryFormData & { logAxis: boolean; markerEnabled: boolean; markerSize: number; + metrics: QueryFormMetric[]; minorSplitLine: boolean; opacity: number; orderDesc: boolean; diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts index ea1691d11..41554347d 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts @@ -153,11 +153,12 @@ export function sortAndFilterSeries( export function sortRows( rows: DataRecord[], + totalStackedValues: number[], xAxis: string, xAxisSortSeries: SortSeriesType, xAxisSortSeriesAscending: boolean, ) { - const sortedRows = rows.map(row => { + const sortedRows = rows.map((row, idx) => { let sortKey: DataRecordValue = ''; let aggregate: number | undefined; let entries = 0; @@ -219,6 +220,7 @@ export function sortRows( key: sortKey, value, row, + totalStackedValue: totalStackedValues[idx], }; }); @@ -226,7 +228,7 @@ export function sortRows( sortedRows, ['value'], [xAxisSortSeriesAscending ? 'asc' : 'desc'], - ).map(({ row }) => row); + ).map(({ row, totalStackedValue }) => ({ row, totalStackedValue })); } export function extractSeries( @@ -244,7 +246,7 @@ export function extractSeries( xAxisSortSeries?: SortSeriesType; xAxisSortSeriesAscending?: boolean; } = {}, -): SeriesOption[] { +): [SeriesOption[], number[]] { const { fillNeighborValue, xAxis = DTTM_ALIAS, @@ -258,7 +260,7 @@ export function extractSeries( xAxisSortSeries, xAxisSortSeriesAscending, } = opts; - if (data.length === 0) return []; + if (data.length === 0) return [[], []]; const rows: DataRecord[] = data.map(datum => ({ ...datum, [xAxis]: datum[xAxis], @@ -272,14 +274,23 @@ export function extractSeries( ); const sortedRows = isDefined(xAxisSortSeries) && isDefined(xAxisSortSeriesAscending) - ? sortRows(rows, xAxis, xAxisSortSeries!, xAxisSortSeriesAscending!) - : rows; + ? sortRows( + rows, + totalStackedValues, + xAxis, + xAxisSortSeries!, + xAxisSortSeriesAscending!, + ) + : rows.map((row, idx) => ({ + row, + totalStackedValue: totalStackedValues[idx], + })); - return sortedSeries.map(name => ({ + const finalSeries = sortedSeries.map(name => ({ id: name, name, data: sortedRows - .map((row, idx) => { + .map(({ row, totalStackedValue }, idx) => { const isNextToDefinedValue = isDefined(rows[idx - 1]?.[name]) || isDefined(rows[idx + 1]?.[name]); const isFillNeighborValue = @@ -291,15 +302,19 @@ export function extractSeries( value = fillNeighborValue; } else if ( stack === StackControlsValue.Expand && - totalStackedValues.length > 0 + totalStackedValue !== undefined ) { - value = ((value || 0) as number) / totalStackedValues[idx]; + value = ((value || 0) as number) / totalStackedValue; } return [row[xAxis], value]; }) .filter(obs => !removeNulls || (obs[0] !== null && obs[1] !== null)) .map(obs => (isHorizontal ? [obs[1], obs[0]] : obs)), })); + return [ + finalSeries, + sortedRows.map(({ totalStackedValue }) => totalStackedValue), + ]; } export function formatSeriesName( diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts index 69570ff00..8a2229fbe 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts @@ -56,83 +56,165 @@ const sortData: DataRecord[] = [ { my_x_axis: null, x: 4, y: 3, z: 7 }, ]; +const totalStackedValues = [3, 15, 14]; + test('sortRows by name ascending', () => { - expect(sortRows(sortData, 'my_x_axis', SortSeriesType.Name, true)).toEqual([ - { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, - { my_x_axis: 'foo', x: null, y: 10, z: 5 }, - { my_x_axis: null, x: 4, y: 3, z: 7 }, + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + SortSeriesType.Name, + true, + ), + ).toEqual([ + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, ]); }); test('sortRows by name descending', () => { - expect(sortRows(sortData, 'my_x_axis', SortSeriesType.Name, false)).toEqual([ - { my_x_axis: null, x: 4, y: 3, z: 7 }, - { my_x_axis: 'foo', x: null, y: 10, z: 5 }, - { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + SortSeriesType.Name, + false, + ), + ).toEqual([ + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, ]); }); test('sortRows by sum ascending', () => { - expect(sortRows(sortData, 'my_x_axis', SortSeriesType.Sum, true)).toEqual([ - { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, - { my_x_axis: null, x: 4, y: 3, z: 7 }, - { my_x_axis: 'foo', x: null, y: 10, z: 5 }, + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + SortSeriesType.Sum, + true, + ), + ).toEqual([ + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, ]); }); test('sortRows by sum descending', () => { - expect(sortRows(sortData, 'my_x_axis', SortSeriesType.Sum, false)).toEqual([ - { my_x_axis: 'foo', x: null, y: 10, z: 5 }, - { my_x_axis: null, x: 4, y: 3, z: 7 }, - { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + SortSeriesType.Sum, + false, + ), + ).toEqual([ + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, ]); }); test('sortRows by avg ascending', () => { - expect(sortRows(sortData, 'my_x_axis', SortSeriesType.Avg, true)).toEqual([ - { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, - { my_x_axis: null, x: 4, y: 3, z: 7 }, - { my_x_axis: 'foo', x: null, y: 10, z: 5 }, + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + SortSeriesType.Avg, + true, + ), + ).toEqual([ + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, ]); }); test('sortRows by avg descending', () => { - expect(sortRows(sortData, 'my_x_axis', SortSeriesType.Avg, false)).toEqual([ - { my_x_axis: 'foo', x: null, y: 10, z: 5 }, - { my_x_axis: null, x: 4, y: 3, z: 7 }, - { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + SortSeriesType.Avg, + false, + ), + ).toEqual([ + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, ]); }); test('sortRows by min ascending', () => { - expect(sortRows(sortData, 'my_x_axis', SortSeriesType.Min, true)).toEqual([ - { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, - { my_x_axis: null, x: 4, y: 3, z: 7 }, - { my_x_axis: 'foo', x: null, y: 10, z: 5 }, + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + SortSeriesType.Min, + true, + ), + ).toEqual([ + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, ]); }); test('sortRows by min descending', () => { - expect(sortRows(sortData, 'my_x_axis', SortSeriesType.Min, false)).toEqual([ - { my_x_axis: 'foo', x: null, y: 10, z: 5 }, - { my_x_axis: null, x: 4, y: 3, z: 7 }, - { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + SortSeriesType.Min, + false, + ), + ).toEqual([ + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, ]); }); test('sortRows by max ascending', () => { - expect(sortRows(sortData, 'my_x_axis', SortSeriesType.Min, true)).toEqual([ - { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, - { my_x_axis: null, x: 4, y: 3, z: 7 }, - { my_x_axis: 'foo', x: null, y: 10, z: 5 }, + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + SortSeriesType.Min, + true, + ), + ).toEqual([ + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, ]); }); test('sortRows by max descending', () => { - expect(sortRows(sortData, 'my_x_axis', SortSeriesType.Min, false)).toEqual([ - { my_x_axis: 'foo', x: null, y: 10, z: 5 }, - { my_x_axis: null, x: 4, y: 3, z: 7 }, - { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, + expect( + sortRows( + sortData, + totalStackedValues, + 'my_x_axis', + SortSeriesType.Min, + false, + ), + ).toEqual([ + { row: { my_x_axis: 'foo', x: null, y: 10, z: 5 }, totalStackedValue: 15 }, + { row: { my_x_axis: null, x: 4, y: 3, z: 7 }, totalStackedValue: 14 }, + { row: { my_x_axis: 'abc', x: 1, y: 0, z: 2 }, totalStackedValue: 3 }, ]); }); @@ -215,25 +297,29 @@ describe('extractSeries', () => { abc: 5, }, ]; - expect(extractSeries(data)).toEqual([ - { - id: 'Hulk', - name: 'Hulk', - data: [ - ['2000-01-01', null], - ['2000-02-01', 2], - ['2000-03-01', 1], - ], - }, - { - id: 'abc', - name: 'abc', - data: [ - ['2000-01-01', 2], - ['2000-02-01', 10], - ['2000-03-01', 5], - ], - }, + const totalStackedValues = [2, 12, 6]; + expect(extractSeries(data, { totalStackedValues })).toEqual([ + [ + { + id: 'Hulk', + name: 'Hulk', + data: [ + ['2000-01-01', null], + ['2000-02-01', 2], + ['2000-03-01', 1], + ], + }, + { + id: 'abc', + name: 'abc', + data: [ + ['2000-01-01', 2], + ['2000-02-01', 10], + ['2000-03-01', 5], + ], + }, + ], + totalStackedValues, ]); }); @@ -255,20 +341,30 @@ describe('extractSeries', () => { abc: 5, }, ]; - expect(extractSeries(data, { xAxis: 'x', removeNulls: true })).toEqual([ - { - id: 'Hulk', - name: 'Hulk', - data: [[2, 1]], - }, - { - id: 'abc', - name: 'abc', - data: [ - [1, 2], - [2, 5], - ], - }, + const totalStackedValues = [3, 12, 8]; + expect( + extractSeries(data, { + totalStackedValues, + xAxis: 'x', + removeNulls: true, + }), + ).toEqual([ + [ + { + id: 'Hulk', + name: 'Hulk', + data: [[2, 1]], + }, + { + id: 'abc', + name: 'abc', + data: [ + [1, 2], + [2, 5], + ], + }, + ], + totalStackedValues, ]); }); @@ -315,23 +411,29 @@ describe('extractSeries', () => { abc: null, }, ]; - expect(extractSeries(data, { fillNeighborValue: 0 })).toEqual([ - { - id: 'abc', - name: 'abc', - data: [ - ['2000-01-01', null], - ['2000-02-01', 0], - ['2000-03-01', 1], - ['2000-04-01', 0], - ['2000-05-01', null], - ['2000-06-01', 0], - ['2000-07-01', 2], - ['2000-08-01', 3], - ['2000-09-01', 0], - ['2000-10-01', null], - ], - }, + const totalStackedValues = [0, 0, 1, 0, 0, 0, 2, 3, 0, 0]; + expect( + extractSeries(data, { totalStackedValues, fillNeighborValue: 0 }), + ).toEqual([ + [ + { + id: 'abc', + name: 'abc', + data: [ + ['2000-01-01', null], + ['2000-02-01', 0], + ['2000-03-01', 1], + ['2000-04-01', 0], + ['2000-05-01', null], + ['2000-06-01', 0], + ['2000-07-01', 2], + ['2000-08-01', 3], + ['2000-09-01', 0], + ['2000-10-01', null], + ], + }, + ], + totalStackedValues, ]); }); });