Skip to content

Commit

Permalink
fix: remove sort values on stacked totals (#31333)
Browse files Browse the repository at this point in the history
  • Loading branch information
@eschutho
eschutho authored Feb 13, 2025
1 parent 5867b87 commit 15fbb19
Show file tree
Hide file tree
Showing 6 changed files with 175 additions and 27 deletions.
24 changes: 15 additions & 9 deletions superset-frontend/packages/superset-ui-core/src/query/getMetricLabel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
13 changes: 11 additions & 2 deletions superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
ensureIsArray,
GenericDataType,
getCustomFormatter,
getMetricLabel,
getNumberFormatter,
getXAxisLabel,
isDefined,
Expand Down Expand Up @@ -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 {
Expand All @@ -305,7 +314,7 @@ export default function transformProps(
} = extractDataTotalValues(rebasedDataB, {
stack: Boolean(stackB),
percentageThreshold,
xAxisCol: xAxisLabel,
metricsLabels: metricsLabelsB,
});

annotationLayers
Expand Down
7 changes: 5 additions & 2 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,20 +60,20 @@ export function extractDataTotalValues(
opts: {
stack: StackType;
percentageThreshold: number;
xAxisCol: string;
legendState?: LegendState;
metricsLabels: string[];
},
): {
totalStackedValues: number[];
thresholdValues: number[];
} {
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]) {
Expand Down
34 changes: 23 additions & 11 deletions superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
],
},
];
Expand All @@ -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({
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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,
Expand All @@ -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,
},
],
},
Expand All @@ -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);
Expand Down Expand Up @@ -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) => {
Expand All @@ -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,
Expand Down
118 changes: 118 additions & 0 deletions superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
calculateLowerLogTick,
dedupSeries,
extractGroupbyLabel,
extractDataTotalValues,
extractSeries,
extractShowValueIndexes,
extractTooltipKeys,
Expand Down Expand Up @@ -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']);
Expand Down

0 comments on commit 15fbb19

Please sign in to comment.