This is an automated email from the ASF dual-hosted git repository. michaelsmolina pushed a commit to branch 5.0 in repository https://gitbox.apache.org/repos/asf/superset.git
commit 39859e0d171df8688a6b1f54514477a852adae68 Author: Elizabeth Thompson <[email protected]> AuthorDate: Wed Feb 12 16:56:53 2025 -0800 fix: remove sort values on stacked totals (#31333) (cherry picked from commit 15fbb195e9a706bcc6f65e37f424e0f24447f4d4) --- .../superset-ui-core/src/query/getMetricLabel.ts | 24 +++-- .../src/MixedTimeseries/transformProps.ts | 13 ++- .../src/Timeseries/transformProps.ts | 7 +- .../plugin-chart-echarts/src/utils/series.ts | 6 +- .../test/Timeseries/transformProps.test.ts | 34 ++++-- .../plugin-chart-echarts/test/utils/series.test.ts | 118 +++++++++++++++++++++ 6 files changed, 175 insertions(+), 27 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/query/getMetricLabel.ts b/superset-frontend/packages/superset-ui-core/src/query/getMetricLabel.ts index 7ac7930c6a..246bf99432 100644 --- a/superset-frontend/packages/superset-ui-core/src/query/getMetricLabel.ts +++ b/superset-frontend/packages/superset-ui-core/src/query/getMetricLabel.ts @@ -19,17 +19,23 @@ import { QueryFormMetric, isSavedMetric, isAdhocMetricSimple } from './types'; -export default function getMetricLabel(metric: QueryFormMetric): string { +export default function getMetricLabel( + metric: QueryFormMetric, + index?: number, + queryFormMetrics?: QueryFormMetric[], + verboseMap?: Record<string, string>, +): string { + let label = ''; if (isSavedMetric(metric)) { - return metric; - } - if (metric.label) { - return metric.label; - } - if (isAdhocMetricSimple(metric)) { - return `${metric.aggregate}(${ + label = metric; + } else if (metric.label) { + ({ label } = metric); + } else if (isAdhocMetricSimple(metric)) { + label = `${metric.aggregate}(${ metric.column.columnName || metric.column.column_name })`; + } else { + label = metric.sqlExpression; } - return metric.sqlExpression; + return verboseMap?.[label] || label; } 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 7526b820d0..63582a860c 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -27,6 +27,7 @@ import { ensureIsArray, GenericDataType, getCustomFormatter, + getMetricLabel, getNumberFormatter, getXAxisLabel, isDefined, @@ -291,12 +292,20 @@ export default function transformProps( const showValueIndexesB = extractShowValueIndexes(rawSeriesB, { stack, }); + + const metricsLabels = metrics + .map(metric => getMetricLabel(metric, undefined, undefined, verboseMap)) + .filter((label): label is string => label !== undefined); + const metricsLabelsB = metricsB.map((metric: QueryFormMetric) => + getMetricLabel(metric, undefined, undefined, verboseMap), + ); + const { totalStackedValues, thresholdValues } = extractDataTotalValues( rebasedDataA, { stack, percentageThreshold, - xAxisCol: xAxisLabel, + metricsLabels, }, ); const { @@ -305,7 +314,7 @@ export default function transformProps( } = extractDataTotalValues(rebasedDataB, { stack: Boolean(stackB), percentageThreshold, - xAxisCol: xAxisLabel, + metricsLabels: metricsLabelsB, }); annotationLayers 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 0fb392a12d..d5bfd88095 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -215,14 +215,18 @@ export default function transformProps( ) { xAxisLabel = verboseMap[xAxisLabel]; } + const metricsLabels = metrics + .map(metric => getMetricLabel(metric, undefined, undefined, verboseMap)) + .filter((label): label is string => label !== undefined); const isHorizontal = orientation === OrientationType.Horizontal; + const { totalStackedValues, thresholdValues } = extractDataTotalValues( rebasedData, { stack, percentageThreshold, - xAxisCol: xAxisLabel, legendState, + metricsLabels, }, ); const extraMetricLabels = extractExtraMetrics(chartProps.rawFormData).map( @@ -296,7 +300,6 @@ export default function transformProps( const entryName = String(entry.name || ''); const seriesName = inverted[entryName] || entryName; const colorScaleKey = getOriginalSeries(seriesName, array); - const transformedSeries = transformSeries( entry, colorScale, 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 bbf222a2a5..3fe8ef3310 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts @@ -60,8 +60,8 @@ export function extractDataTotalValues( opts: { stack: StackType; percentageThreshold: number; - xAxisCol: string; legendState?: LegendState; + metricsLabels: string[]; }, ): { totalStackedValues: number[]; @@ -69,11 +69,11 @@ export function extractDataTotalValues( } { const totalStackedValues: number[] = []; const thresholdValues: number[] = []; - const { stack, percentageThreshold, xAxisCol, legendState } = opts; + const { stack, percentageThreshold, legendState, metricsLabels } = opts; if (stack) { data.forEach(datum => { const values = Object.keys(datum).reduce((prev, curr) => { - if (curr === xAxisCol) { + if (!metricsLabels.includes(curr)) { return prev; } if (legendState && !legendState[curr]) { diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts index bc2aa2bd58..5601158675 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts @@ -36,15 +36,25 @@ describe('EchartsTimeseries transformProps', () => { colorScheme: 'bnbColors', datasource: '3__table', granularity_sqla: 'ds', - metric: 'sum__num', + metrics: ['sum__num'], groupby: ['foo', 'bar'], viz_type: 'my_viz', }; const queriesData = [ { data: [ - { 'San Francisco': 1, 'New York': 2, __timestamp: 599616000000 }, - { 'San Francisco': 3, 'New York': 4, __timestamp: 599916000000 }, + { + 'San Francisco': 1, + 'New York': 2, + __timestamp: 599616000000, + sum__num: 4, + }, + { + 'San Francisco': 3, + 'New York': 4, + __timestamp: 599916000000, + sum__num: 8, + }, ], }, ]; @@ -64,7 +74,7 @@ describe('EchartsTimeseries transformProps', () => { height: 600, echartOptions: expect.objectContaining({ legend: expect.objectContaining({ - data: ['San Francisco', 'New York'], + data: ['sum__num', 'San Francisco', 'New York'], }), series: expect.arrayContaining([ expect.objectContaining({ @@ -101,7 +111,7 @@ describe('EchartsTimeseries transformProps', () => { height: 600, echartOptions: expect.objectContaining({ legend: expect.objectContaining({ - data: ['San Francisco', 'New York'], + data: ['sum__num', 'San Francisco', 'New York'], }), series: expect.arrayContaining([ expect.objectContaining({ @@ -146,7 +156,7 @@ describe('EchartsTimeseries transformProps', () => { height: 600, echartOptions: expect.objectContaining({ legend: expect.objectContaining({ - data: ['San Francisco', 'New York', 'My Formula'], + data: ['sum__num', 'San Francisco', 'New York', 'My Formula'], }), series: expect.arrayContaining([ expect.objectContaining({ @@ -274,7 +284,7 @@ describe('EchartsTimeseries transformProps', () => { expect.objectContaining({ echartOptions: expect.objectContaining({ legend: expect.objectContaining({ - data: ['San Francisco', 'New York', 'My Line'], + data: ['sum__num', 'San Francisco', 'New York', 'My Line'], }), series: expect.arrayContaining([ expect.objectContaining({ @@ -420,7 +430,7 @@ describe('Does transformProps transform series correctly', () => { colorScheme: 'bnbColors', datasource: '3__table', granularity_sqla: 'ds', - metric: 'sum__num', + metrics: ['sum__num'], groupby: ['foo', 'bar'], showValue: true, stack: true, @@ -435,24 +445,28 @@ describe('Does transformProps transform series correctly', () => { 'New York': 2, Boston: 1, __timestamp: 599616000000, + sum__num: 4, }, { 'San Francisco': 3, 'New York': 4, Boston: 1, __timestamp: 599916000000, + sum__num: 8, }, { 'San Francisco': 5, 'New York': 8, Boston: 6, __timestamp: 600216000000, + sum__num: 19, }, { 'San Francisco': 2, 'New York': 7, Boston: 2, __timestamp: 600516000000, + sum__num: 11, }, ], }, @@ -468,7 +482,7 @@ describe('Does transformProps transform series correctly', () => { const totalStackedValues = queriesData[0].data.reduce( (totals, currentStack) => { const total = Object.keys(currentStack).reduce((stackSum, key) => { - if (key === '__timestamp') return stackSum; + if (key === '__timestamp' || key === 'sum__num') return stackSum; return stackSum + currentStack[key as keyof typeof currentStack]; }, 0); totals.push(total); @@ -561,7 +575,6 @@ describe('Does transformProps transform series correctly', () => { const expectedThresholds = totalStackedValues.map( total => ((formData.percentageThreshold || 0) / 100) * total, ); - transformedSeries.forEach((series, seriesIndex) => { expect(series.label.show).toBe(true); series.data.forEach((value, dataIndex) => { @@ -576,7 +589,6 @@ describe('Does transformProps transform series correctly', () => { }); }); }); - it('should not apply percentage threshold when showValue is true and stack is false', () => { const updatedChartPropsConfig = { ...chartPropsConfig, 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 7054f6019a..afac03096b 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 @@ -29,6 +29,7 @@ import { calculateLowerLogTick, dedupSeries, extractGroupbyLabel, + extractDataTotalValues, extractSeries, extractShowValueIndexes, extractTooltipKeys, @@ -1085,6 +1086,123 @@ const forecastValue = [ }, ]; +describe('extractDataTotalValues', () => { + it('test_extractDataTotalValues_withStack', () => { + const data: DataRecord[] = [ + { metric1: 10, metric2: 20, xAxisCol: '2021-01-01' }, + { metric1: 15, metric2: 25, xAxisCol: '2021-01-02' }, + ]; + const metricsLabels = ['metric1', 'metric2']; + const opts = { + stack: true, + percentageThreshold: 10, + metricsLabels, + }; + const result = extractDataTotalValues(data, opts); + expect(result.totalStackedValues).toEqual([30, 40]); + expect(result.thresholdValues).toEqual([3, 4]); + }); + + it('should calculate total and threshold values with stack option enabled', () => { + const data: DataRecord[] = [ + { metric1: 10, metric2: 20, xAxisCol: '2021-01-01' }, + { metric1: 15, metric2: 25, xAxisCol: '2021-01-02' }, + ]; + const metricsLabels = ['metric1', 'metric2']; + const opts = { + stack: true, + percentageThreshold: 10, + metricsLabels, + }; + const result = extractDataTotalValues(data, opts); + expect(result.totalStackedValues).toEqual([30, 40]); + expect(result.thresholdValues).toEqual([3, 4]); + }); + + it('should handle empty data array', () => { + const data: DataRecord[] = []; + const metricsLabels: string[] = []; + const opts = { + stack: true, + percentageThreshold: 10, + metricsLabels, + }; + const result = extractDataTotalValues(data, opts); + expect(result.totalStackedValues).toEqual([]); + expect(result.thresholdValues).toEqual([]); + }); + + it('should calculate total and threshold values with stack option disabled', () => { + const data: DataRecord[] = [ + { metric1: 10, metric2: 20, xAxisCol: '2021-01-01' }, + { metric1: 15, metric2: 25, xAxisCol: '2021-01-02' }, + ]; + const metricsLabels = ['metric1', 'metric2']; + const opts = { + stack: false, + percentageThreshold: 10, + metricsLabels, + }; + const result = extractDataTotalValues(data, opts); + expect(result.totalStackedValues).toEqual([]); + expect(result.thresholdValues).toEqual([]); + }); + + it('should handle data with null or undefined values', () => { + const data: DataRecord[] = [ + { 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 }, + ]; + const metricsLabels = ['x', 'y', 'z']; + const opts = { + stack: true, + percentageThreshold: 10, + metricsLabels, + }; + const result = extractDataTotalValues(data, opts); + expect(result.totalStackedValues).toEqual([3, 15, 14]); + expect(result.thresholdValues).toEqual([ + 0.30000000000000004, 1.5, 1.4000000000000001, + ]); + }); + + it('should handle different percentage thresholds', () => { + const data: DataRecord[] = [ + { metric1: 10, metric2: 20, xAxisCol: '2021-01-01' }, + { metric1: 15, metric2: 25, xAxisCol: '2021-01-02' }, + ]; + const metricsLabels = ['metric1', 'metric2']; + const opts = { + stack: true, + percentageThreshold: 50, + metricsLabels, + }; + const result = extractDataTotalValues(data, opts); + expect(result.totalStackedValues).toEqual([30, 40]); + expect(result.thresholdValues).toEqual([15, 20]); + }); + it('should not add datum not in metrics to the total value when stacked', () => { + const data: DataRecord[] = [ + { xAxisCol: 'foo', xAxisSort: 10, val: 345 }, + { xAxisCol: 'bar', xAxisSort: 20, val: 2432 }, + { xAxisCol: 'baz', xAxisSort: 30, val: 4543 }, + ]; + const metricsLabels = ['val']; + const opts = { + stack: true, + percentageThreshold: 50, + metricsLabels, + }; + + const result = extractDataTotalValues(data, opts); + + // Assuming extractDataTotalValues returns the total value + // without including the 'xAxisCol' category + expect(result.totalStackedValues).toEqual([345, 2432, 4543]); // 10 + 20, excluding the 'xAxisCol' category + }); +}); + test('extractTooltipKeys with rich tooltip', () => { const result = extractTooltipKeys(forecastValue, 1, true, false); expect(result).toEqual(['foo', 'bar']);
