mirror of
https://github.com/apache/superset.git
synced 2026-07-16 19:55:39 +00:00
Compare commits
7 Commits
codex/refa
...
tdd/issue-
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
9826ce9100 | ||
|
|
c346fba656 | ||
|
|
8794b8a729 | ||
|
|
53076ec5e7 | ||
|
|
8c959a3008 | ||
|
|
5881cc8de0 | ||
|
|
b94aa33de9 |
@@ -62,24 +62,29 @@ export function formatLabel({
|
||||
params: CallbackDataParams;
|
||||
labelType: EchartsRadarLabelType;
|
||||
numberFormatter: NumberFormatter;
|
||||
getDenormalizedSeriesValue: (seriesName: string, value: string) => number;
|
||||
getDenormalizedSeriesValue: (
|
||||
seriesName: string,
|
||||
value: string,
|
||||
) => number | null;
|
||||
metricsWithCustomBounds: Set<string>;
|
||||
metricLabels: string[];
|
||||
}): string {
|
||||
const { name = '', value, dimensionIndex = 0 } = params;
|
||||
const metricLabel = metricLabels[dimensionIndex];
|
||||
|
||||
const formattedValue = numberFormatter(
|
||||
metricsWithCustomBounds.has(metricLabel)
|
||||
? (value as number)
|
||||
: (getDenormalizedSeriesValue(name, String(value)) as number),
|
||||
);
|
||||
const rawValue = metricsWithCustomBounds.has(metricLabel)
|
||||
? (value as number | null)
|
||||
: getDenormalizedSeriesValue(name, String(value));
|
||||
|
||||
// A missing metric is preserved as null so it renders as a gap; skip
|
||||
// formatting it into a misleading "NaN" label.
|
||||
const formattedValue = rawValue == null ? '' : numberFormatter(rawValue);
|
||||
|
||||
switch (labelType) {
|
||||
case EchartsRadarLabelType.Value:
|
||||
return formattedValue;
|
||||
case EchartsRadarLabelType.KeyValue:
|
||||
return `${name}: ${formattedValue}`;
|
||||
return formattedValue ? `${name}: ${formattedValue}` : name;
|
||||
default:
|
||||
return name;
|
||||
}
|
||||
@@ -134,9 +139,20 @@ export default function transformProps(
|
||||
const getDenormalizedSeriesValue = (
|
||||
seriesName: string,
|
||||
normalizedValue: string,
|
||||
): number =>
|
||||
denormalizedSeriesValues?.[seriesName]?.[normalizedValue] ??
|
||||
Number(normalizedValue);
|
||||
): number | null => {
|
||||
const seriesMap = denormalizedSeriesValues?.[seriesName];
|
||||
// A missing metric is recorded explicitly as `null`, so use
|
||||
// `hasOwnProperty` rather than `??`/`?.` to distinguish "recorded as
|
||||
// null" from "never recorded" (which falls back to parsing the raw
|
||||
// normalized value).
|
||||
if (
|
||||
seriesMap &&
|
||||
Object.prototype.hasOwnProperty.call(seriesMap, normalizedValue)
|
||||
) {
|
||||
return seriesMap[normalizedValue];
|
||||
}
|
||||
return Number(normalizedValue);
|
||||
};
|
||||
|
||||
const metricLabels = metrics.map(getMetricLabel);
|
||||
|
||||
@@ -254,14 +270,31 @@ export default function transformProps(
|
||||
{},
|
||||
);
|
||||
|
||||
const normalizeArray = (arr: number[], decimals = 10, seriesName: string) =>
|
||||
const normalizeArray = (
|
||||
arr: (number | null)[],
|
||||
decimals = 10,
|
||||
seriesName: string,
|
||||
): (number | null)[] =>
|
||||
arr.map((value, index) => {
|
||||
const metricLabel = metricLabels[index];
|
||||
if (metricsWithCustomBounds.has(metricLabel)) {
|
||||
return value;
|
||||
}
|
||||
|
||||
const max = Math.max(...arr);
|
||||
// Preserve missing (null/undefined) metric values so they render as a
|
||||
// gap. Dividing null/undefined by max coerces it to 0, which would plot
|
||||
// the point at the center of the radar as if it were a real zero.
|
||||
// Explicitly record the denormalized entry as null (rather than
|
||||
// leaving it unset) so downstream label/tooltip lookups can tell a
|
||||
// recorded gap apart from an unrecorded value and skip formatting it.
|
||||
if (value == null || !Number.isFinite(value)) {
|
||||
denormalizedSeriesValues[seriesName][String(null)] = null;
|
||||
return null;
|
||||
}
|
||||
|
||||
const max = Math.max(
|
||||
...arr.filter((v): v is number => v != null && Number.isFinite(v)),
|
||||
);
|
||||
const normalizedValue = Number((value / max).toFixed(decimals));
|
||||
|
||||
denormalizedSeriesValues[seriesName][String(normalizedValue)] = value;
|
||||
@@ -276,7 +309,11 @@ export default function transformProps(
|
||||
|
||||
return {
|
||||
...series,
|
||||
value: normalizeArray(series.value as number[], 10, seriesName),
|
||||
value: normalizeArray(
|
||||
series.value as (number | null)[],
|
||||
10,
|
||||
seriesName,
|
||||
),
|
||||
};
|
||||
}
|
||||
return series;
|
||||
@@ -356,7 +393,7 @@ export default function transformProps(
|
||||
params: CallbackDataParams & {
|
||||
color: string;
|
||||
name: string;
|
||||
value: number[];
|
||||
value: (number | null)[];
|
||||
},
|
||||
) =>
|
||||
renderNormalizedTooltip(
|
||||
|
||||
@@ -85,10 +85,12 @@ export type RadarChartTransformedProps =
|
||||
CrossFilterTransformedProps;
|
||||
|
||||
/**
|
||||
* Represents a mapping from a normalized value (as string) to an original numeric value.
|
||||
* Represents a mapping from a normalized value (as string) to an original
|
||||
* numeric value. A `null` entry is an explicit record of a missing metric,
|
||||
* distinct from an absent key (which has not been recorded at all).
|
||||
*/
|
||||
interface NormalizedValueMap {
|
||||
[normalized: string]: number;
|
||||
[normalized: string]: number | null;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -16,6 +16,7 @@
|
||||
* specific language governing permissions and limitations
|
||||
* under the License.
|
||||
*/
|
||||
import { t } from '@apache-superset/core/translation';
|
||||
import { NumberFormatter } from '@superset-ui/core';
|
||||
|
||||
/*
|
||||
@@ -44,7 +45,7 @@ export const findGlobalMax = (
|
||||
interface TooltipParams {
|
||||
color: string;
|
||||
name?: string;
|
||||
value: number[];
|
||||
value: (number | null)[];
|
||||
}
|
||||
|
||||
interface TooltipMetricValue {
|
||||
@@ -55,7 +56,7 @@ interface TooltipMetricValue {
|
||||
export const renderNormalizedTooltip = (
|
||||
params: TooltipParams,
|
||||
metrics: string[],
|
||||
getDenormalizedValue: (seriesName: string, value: string) => number,
|
||||
getDenormalizedValue: (seriesName: string, value: string) => number | null,
|
||||
metricsWithCustomBounds: Set<string>,
|
||||
formatter?: NumberFormatter,
|
||||
): string => {
|
||||
@@ -73,7 +74,14 @@ export const renderNormalizedTooltip = (
|
||||
|
||||
return {
|
||||
metric,
|
||||
value: formatter ? formatter(originalValue) : originalValue,
|
||||
// A missing metric stays null; show it plainly rather than running it
|
||||
// through the formatter, which would render a misleading "NaN".
|
||||
value:
|
||||
originalValue == null
|
||||
? t('N/A')
|
||||
: formatter
|
||||
? formatter(originalValue)
|
||||
: originalValue,
|
||||
};
|
||||
});
|
||||
|
||||
|
||||
@@ -42,6 +42,13 @@ interface RadarChartConfig {
|
||||
interface RadarSeriesData {
|
||||
value: number[];
|
||||
name: string;
|
||||
label?: {
|
||||
formatter: (params: {
|
||||
name: string;
|
||||
value: number | null;
|
||||
dimensionIndex: number;
|
||||
}) => string;
|
||||
};
|
||||
}
|
||||
|
||||
const formData: Partial<EchartsRadarFormData> = {
|
||||
@@ -204,6 +211,72 @@ describe('legend sorting', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// Regression for #30270: "Wrong visualization of missing values in radar
|
||||
// charts". A null metric value must NOT be transformed into 0. In
|
||||
// `normalizeArray`, `null / max` coerces the null to 0, so a missing data
|
||||
// point ends up plotted at the center of the radar as if it were a real
|
||||
// zero, instead of being left out (a gap). This test feeds a datum with a
|
||||
// null metric and asserts the null is preserved in the normalized series
|
||||
// value rather than silently becoming 0.
|
||||
const missingValueData = [
|
||||
{
|
||||
data: [
|
||||
{
|
||||
name: 'Series A',
|
||||
'SUM(jp_sales)': 10,
|
||||
'SUM(other_sales)': null,
|
||||
'SUM(eu_sales)': 30,
|
||||
},
|
||||
],
|
||||
},
|
||||
];
|
||||
|
||||
const missingValueProps = new ChartProps({
|
||||
formData: {
|
||||
...formData,
|
||||
// No columnConfig custom bounds so every metric is normalized.
|
||||
columnConfig: {},
|
||||
groupby: ['name'],
|
||||
metrics: ['SUM(jp_sales)', 'SUM(other_sales)', 'SUM(eu_sales)'],
|
||||
},
|
||||
width: 800,
|
||||
height: 600,
|
||||
queriesData: missingValueData,
|
||||
theme: supersetTheme,
|
||||
});
|
||||
|
||||
test('preserves a null metric instead of plotting it as 0', () => {
|
||||
const result = transformProps(missingValueProps as EchartsRadarChartProps);
|
||||
const series = result.echartOptions.series as RadarSeriesOption[];
|
||||
const value = (series[0].data as RadarSeriesData[])[0].value as (
|
||||
number | null
|
||||
)[];
|
||||
|
||||
// Index 1 corresponds to 'SUM(other_sales)', which was null in the datum.
|
||||
// The correct behavior is to keep the gap (null/undefined) so ECharts does
|
||||
// not draw a point at the center. On master this is 0, so this fails.
|
||||
expect(value[1]).not.toBe(0);
|
||||
expect(value[1] == null).toBe(true);
|
||||
});
|
||||
|
||||
test('label formatter renders a missing metric as blank instead of NaN', () => {
|
||||
const result = transformProps(missingValueProps as EchartsRadarChartProps);
|
||||
const series = result.echartOptions.series as RadarSeriesOption[];
|
||||
const seriesData = (series[0].data as RadarSeriesData[])[0];
|
||||
const { label } = seriesData;
|
||||
if (!label) throw new Error('expected series data to have a label config');
|
||||
|
||||
// Index 1 corresponds to 'SUM(other_sales)', which is null. Denormalizing
|
||||
// it must not fall through to `Number('null')` (NaN) in the label text.
|
||||
const formatted = label.formatter({
|
||||
name: 'Series A',
|
||||
value: null,
|
||||
dimensionIndex: 1,
|
||||
});
|
||||
|
||||
expect(formatted).not.toContain('NaN');
|
||||
});
|
||||
|
||||
describe('radar center positioning', () => {
|
||||
const getCenter = (overrides: Partial<EchartsRadarFormData> = {}) => {
|
||||
const props = new ChartProps({
|
||||
|
||||
@@ -52,4 +52,22 @@ describe('renderNormalizedTooltip', () => {
|
||||
expect(tooltip).toContain('100');
|
||||
expect(tooltip).toContain('200');
|
||||
});
|
||||
|
||||
test('should render a missing metric as N/A instead of NaN', () => {
|
||||
// A missing metric is denormalized to `null` (see #30270); the tooltip
|
||||
// must not run it through the formatter, which would print "NaN".
|
||||
const getDenormalizedValueWithGap = jest.fn((_, value) =>
|
||||
value === 'null' ? null : Number(value),
|
||||
);
|
||||
const formatter = getNumberFormatter(',.2f');
|
||||
const tooltip = renderNormalizedTooltip(
|
||||
{ ...params, value: [100, null] },
|
||||
metrics,
|
||||
getDenormalizedValueWithGap,
|
||||
metricsWithCustomBounds,
|
||||
formatter,
|
||||
);
|
||||
expect(tooltip).toContain('N/A');
|
||||
expect(tooltip).not.toContain('NaN');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user