This is an automated email from the ASF dual-hosted git repository. elizabeth pushed a commit to branch elizabeth/revert-stacked-totals in repository https://gitbox.apache.org/repos/asf/superset.git
commit 583622277e338cf4bc09d03cdb48ccc1c8361784 Author: Elizabeth Thompson <[email protected]> AuthorDate: Thu Feb 20 15:23:17 2025 -0800 Revert "fix: remove sort values on stacked totals (#31333)" This reverts commit 39859e0d171df8688a6b1f54514477a852adae68. --- .../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, 27 insertions(+), 175 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 246bf99432..7ac7930c6a 100644 --- a/superset-frontend/packages/superset-ui-core/src/query/getMetricLabel.ts +++ b/superset-frontend/packages/superset-ui-core/src/query/getMetricLabel.ts @@ -19,23 +19,17 @@ import { QueryFormMetric, isSavedMetric, isAdhocMetricSimple } from './types'; -export default function getMetricLabel( - metric: QueryFormMetric, - index?: number, - queryFormMetrics?: QueryFormMetric[], - verboseMap?: Record<string, string>, -): string { - let label = ''; +export default function getMetricLabel(metric: QueryFormMetric): string { if (isSavedMetric(metric)) { - label = metric; - } else if (metric.label) { - ({ label } = metric); - } else if (isAdhocMetricSimple(metric)) { - label = `${metric.aggregate}(${ + return metric; + } + if (metric.label) { + return metric.label; + } + if (isAdhocMetricSimple(metric)) { + return `${metric.aggregate}(${ metric.column.columnName || metric.column.column_name })`; - } else { - label = metric.sqlExpression; } - return verboseMap?.[label] || label; + return metric.sqlExpression; } 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 63582a860c..7526b820d0 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -27,7 +27,6 @@ import { ensureIsArray, GenericDataType, getCustomFormatter, - getMetricLabel, getNumberFormatter, getXAxisLabel, isDefined, @@ -292,20 +291,12 @@ 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, - metricsLabels, + xAxisCol: xAxisLabel, }, ); const { @@ -314,7 +305,7 @@ export default function transformProps( } = extractDataTotalValues(rebasedDataB, { stack: Boolean(stackB), percentageThreshold, - metricsLabels: metricsLabelsB, + xAxisCol: xAxisLabel, }); 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 d5bfd88095..0fb392a12d 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -215,18 +215,14 @@ 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( @@ -300,6 +296,7 @@ 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 3fe8ef3310..bbf222a2a5 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, legendState, metricsLabels } = opts; + const { stack, percentageThreshold, xAxisCol, legendState } = opts; if (stack) { data.forEach(datum => { const values = Object.keys(datum).reduce((prev, curr) => { - if (!metricsLabels.includes(curr)) { + if (curr === xAxisCol) { 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 5601158675..bc2aa2bd58 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,25 +36,15 @@ describe('EchartsTimeseries transformProps', () => { colorScheme: 'bnbColors', datasource: '3__table', granularity_sqla: 'ds', - metrics: ['sum__num'], + metric: 'sum__num', groupby: ['foo', 'bar'], viz_type: 'my_viz', }; const queriesData = [ { data: [ - { - 'San Francisco': 1, - 'New York': 2, - __timestamp: 599616000000, - sum__num: 4, - }, - { - 'San Francisco': 3, - 'New York': 4, - __timestamp: 599916000000, - sum__num: 8, - }, + { 'San Francisco': 1, 'New York': 2, __timestamp: 599616000000 }, + { 'San Francisco': 3, 'New York': 4, __timestamp: 599916000000 }, ], }, ]; @@ -74,7 +64,7 @@ describe('EchartsTimeseries transformProps', () => { height: 600, echartOptions: expect.objectContaining({ legend: expect.objectContaining({ - data: ['sum__num', 'San Francisco', 'New York'], + data: ['San Francisco', 'New York'], }), series: expect.arrayContaining([ expect.objectContaining({ @@ -111,7 +101,7 @@ describe('EchartsTimeseries transformProps', () => { height: 600, echartOptions: expect.objectContaining({ legend: expect.objectContaining({ - data: ['sum__num', 'San Francisco', 'New York'], + data: ['San Francisco', 'New York'], }), series: expect.arrayContaining([ expect.objectContaining({ @@ -156,7 +146,7 @@ describe('EchartsTimeseries transformProps', () => { height: 600, echartOptions: expect.objectContaining({ legend: expect.objectContaining({ - data: ['sum__num', 'San Francisco', 'New York', 'My Formula'], + data: ['San Francisco', 'New York', 'My Formula'], }), series: expect.arrayContaining([ expect.objectContaining({ @@ -284,7 +274,7 @@ describe('EchartsTimeseries transformProps', () => { expect.objectContaining({ echartOptions: expect.objectContaining({ legend: expect.objectContaining({ - data: ['sum__num', 'San Francisco', 'New York', 'My Line'], + data: ['San Francisco', 'New York', 'My Line'], }), series: expect.arrayContaining([ expect.objectContaining({ @@ -430,7 +420,7 @@ describe('Does transformProps transform series correctly', () => { colorScheme: 'bnbColors', datasource: '3__table', granularity_sqla: 'ds', - metrics: ['sum__num'], + metric: 'sum__num', groupby: ['foo', 'bar'], showValue: true, stack: true, @@ -445,28 +435,24 @@ 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, }, ], }, @@ -482,7 +468,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' || key === 'sum__num') return stackSum; + if (key === '__timestamp') return stackSum; return stackSum + currentStack[key as keyof typeof currentStack]; }, 0); totals.push(total); @@ -575,6 +561,7 @@ 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) => { @@ -589,6 +576,7 @@ 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 afac03096b..7054f6019a 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,7 +29,6 @@ import { calculateLowerLogTick, dedupSeries, extractGroupbyLabel, - extractDataTotalValues, extractSeries, extractShowValueIndexes, extractTooltipKeys, @@ -1086,123 +1085,6 @@ 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']);
