You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/06/26 05:35:04 UTC
(superset) 01/01: fix(sunburst): make "Show Null Values" non-breaking and cover all layers
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix/sunburst-null-values-followup
in repository https://gitbox.apache.org/repos/asf/superset.git
commit e89d2c7dcec91963dc3d3d6cf88c3d210073c380
Author: Claude Code <no...@anthropic.com>
AuthorDate: Thu Jun 25 22:34:02 2026 -0700
fix(sunburst): make "Show Null Values" non-breaking and cover all layers
Follow-up to #31477. Three issues with the new "Show Null Values" control:
1. Legacy charts (saved before the control existed) have no `showNullValues`
in form data, so it read as `undefined` and `!undefined === true` forced
filtering on — silently hiding nulls that previously rendered, the opposite
of the control's `default: true`. Default it to `true` in transformProps so
existing charts keep showing nulls.
2. The filter only ran in the multi-level branch, so single-column sunbursts
and null values in the root column slipped through unfiltered. Filter at
every recursion level instead, covering roots and leaves uniformly.
3. Children were filtered after the parent reduced over them, so the parent
arc kept the hidden null's value and rendered an empty gap. Children are now
filtered before the reduce, so parent totals match their visible children.
Adds tests for legacy/undefined default, single-level filtering on/off,
root-level filtering on/off, and secondary-metric total correctness.
Co-Authored-By: Claude Fable 5 <no...@anthropic.com>
---
.../src/Sunburst/transformProps.ts | 4 +-
.../plugin-chart-echarts/src/utils/treeBuilder.ts | 13 ++-
.../test/Sunburst/transformProps.test.ts | 46 ++++++++
.../test/utils/treeBuilder.test.ts | 116 ++++++++++++++++++++-
4 files changed, 170 insertions(+), 9 deletions(-)
diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts
index 0b1873441e4..f04d67857cc 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts
@@ -186,7 +186,9 @@ export default function transformProps(
showLabels,
showLabelsThreshold,
showTotal,
- showNullValues,
+ // Default to true so charts saved before this control existed keep
+ // showing null values instead of silently hiding them on upgrade.
+ showNullValues = true,
sliceId,
} = formData;
const {
diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/utils/treeBuilder.ts b/superset-frontend/plugins/plugin-chart-echarts/src/utils/treeBuilder.ts
index fe7ec42e446..4a938a590ff 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/treeBuilder.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/treeBuilder.ts
@@ -40,7 +40,7 @@ export function treeBuilder(
): TreeNode[] {
const [curGroupBy, ...restGroupby] = groupBy;
const curData = _groupBy(data, curGroupBy);
- return transform(
+ const nodes = transform(
curData,
(result, value, key) => {
const name = curData[key][0][curGroupBy]!;
@@ -59,6 +59,9 @@ export function treeBuilder(
result.push(item);
});
} else {
+ // Children are already null-filtered by the recursive call, so the
+ // parent's value/secondaryValue exclude hidden nulls. This keeps the
+ // parent arc sized to its visible children (no empty gap).
const children = treeBuilder(
value,
restGroupby,
@@ -76,12 +79,9 @@ export function treeBuilder(
0,
)
: metricValue;
- const validChildren = filterNullNames
- ? children.filter(child => child.name !== null)
- : children;
result.push({
name,
- children: validChildren,
+ children,
value: metricValue,
secondaryValue,
groupBy: curGroupBy,
@@ -90,4 +90,7 @@ export function treeBuilder(
},
[] as TreeNode[],
);
+ // Filter at every level so single-level charts and root nodes are covered,
+ // not just nested children.
+ return filterNullNames ? nodes.filter(node => node.name !== null) : nodes;
}
diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts
index 043b0395a64..0839c343044 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts
@@ -51,3 +51,49 @@ test('series label has no textBorderColor or textBorderWidth', () => {
expect(series.label).not.toHaveProperty('textBorderColor');
expect(series.label).not.toHaveProperty('textBorderWidth');
});
+
+const nullValueProps = (showNullValues?: boolean) =>
+ new ChartProps({
+ formData: {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ columns: ['category'],
+ metric: 'sum__value',
+ ...(showNullValues === undefined ? {} : { showNullValues }),
+ },
+ width: 800,
+ height: 600,
+ queriesData: [
+ {
+ data: [
+ { category: 'A', sum__value: 10 },
+ { category: 'B', sum__value: 20 },
+ { category: null, sum__value: 5 },
+ ],
+ },
+ ],
+ theme: supersetTheme,
+ });
+
+const seriesValues = (props: ChartProps) =>
+ (
+ (transformProps(props as EchartsSunburstChartProps).echartOptions as any)
+ .series[0].data as { value: number }[]
+ )
+ .map(node => node.value)
+ .sort((a, b) => a - b);
+
+// Charts saved before the "Show Null Values" control existed have no
+// `showNullValues` in form data; they must keep showing nulls (non-breaking).
+test('keeps null values when showNullValues is unset (legacy charts)', () => {
+ expect(seriesValues(nullValueProps(undefined))).toEqual([5, 10, 20]);
+});
+
+test('keeps null values when showNullValues is true', () => {
+ expect(seriesValues(nullValueProps(true))).toEqual([5, 10, 20]);
+});
+
+// Single-column sunburst: the toggle must actually drop the null node.
+test('removes null values when showNullValues is false', () => {
+ expect(seriesValues(nullValueProps(false))).toEqual([10, 20]);
+});
diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/utils/treeBuilder.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/utils/treeBuilder.test.ts
index 4d99bce8c94..28cfa5ab790 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/test/utils/treeBuilder.test.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/test/utils/treeBuilder.test.ts
@@ -394,7 +394,7 @@ describe('test treeBuilder', () => {
]);
});
- test('filter null values', () => {
+ test('filter null values in a nested layer (parent total excludes hidden nulls)', () => {
const tree = treeBuilder(
[
...data,
@@ -426,6 +426,8 @@ describe('test treeBuilder', () => {
value: 2,
},
{
+ // The null `bar` child is removed AND its value is excluded from the
+ // parent total, so the arc stays sized to its visible children (no gap).
children: [
{
groupBy: 'bar',
@@ -436,8 +438,8 @@ describe('test treeBuilder', () => {
],
groupBy: 'foo',
name: 'a-2',
- secondaryValue: 4,
- value: 4,
+ secondaryValue: 2,
+ value: 2,
},
{
children: [
@@ -511,4 +513,112 @@ describe('test treeBuilder', () => {
},
]);
});
+
+ // Regression: a single-level (single column) sunburst previously never
+ // filtered, because filtering only happened in the multi-level branch.
+ test('single-level: shows null nodes when filtering is off', () => {
+ const tree = treeBuilder(
+ [
+ { foo: 'a', count: 2, count2: 3 },
+ { foo: null, count: 5, count2: 7 },
+ ],
+ ['foo'],
+ 'count',
+ );
+ expect(tree).toEqual([
+ { groupBy: 'foo', name: 'a', secondaryValue: 2, value: 2 },
+ { groupBy: 'foo', name: null, secondaryValue: 5, value: 5 },
+ ]);
+ });
+
+ test('single-level: removes null nodes when filtering is on', () => {
+ const tree = treeBuilder(
+ [
+ { foo: 'a', count: 2, count2: 3 },
+ { foo: null, count: 5, count2: 7 },
+ ],
+ ['foo'],
+ 'count',
+ undefined,
+ true,
+ );
+ expect(tree).toEqual([
+ { groupBy: 'foo', name: 'a', secondaryValue: 2, value: 2 },
+ ]);
+ });
+
+ // Regression: a null in the *root* (first) column previously slipped through
+ // because the top-level result array was never filtered.
+ test('multi-level: shows null root node when filtering is off', () => {
+ const tree = treeBuilder(
+ [
+ { foo: 'a-1', bar: 'a', count: 2, count2: 3 },
+ { foo: null, bar: 'x', count: 5, count2: 7 },
+ ],
+ ['foo', 'bar'],
+ 'count',
+ );
+ expect(tree).toEqual([
+ {
+ children: [{ groupBy: 'bar', name: 'a', secondaryValue: 2, value: 2 }],
+ groupBy: 'foo',
+ name: 'a-1',
+ secondaryValue: 2,
+ value: 2,
+ },
+ {
+ children: [{ groupBy: 'bar', name: 'x', secondaryValue: 5, value: 5 }],
+ groupBy: 'foo',
+ name: null,
+ secondaryValue: 5,
+ value: 5,
+ },
+ ]);
+ });
+
+ test('multi-level: removes null root node (and its subtree) when filtering is on', () => {
+ const tree = treeBuilder(
+ [
+ { foo: 'a-1', bar: 'a', count: 2, count2: 3 },
+ { foo: null, bar: 'x', count: 5, count2: 7 },
+ ],
+ ['foo', 'bar'],
+ 'count',
+ undefined,
+ true,
+ );
+ expect(tree).toEqual([
+ {
+ children: [{ groupBy: 'bar', name: 'a', secondaryValue: 2, value: 2 }],
+ groupBy: 'foo',
+ name: 'a-1',
+ secondaryValue: 2,
+ value: 2,
+ },
+ ]);
+ });
+
+ // With a secondary metric, the parent's secondaryValue must also exclude the
+ // hidden null child rather than leaving a stale (inflated) total.
+ test('filtering excludes hidden nulls from secondary-metric totals', () => {
+ const tree = treeBuilder(
+ [
+ { foo: 'p', bar: 'a', count: 2, count2: 3 },
+ { foo: 'p', bar: null, count: 2, count2: 7 },
+ ],
+ ['foo', 'bar'],
+ 'count',
+ 'count2',
+ true,
+ );
+ expect(tree).toEqual([
+ {
+ children: [{ groupBy: 'bar', name: 'a', secondaryValue: 3, value: 2 }],
+ groupBy: 'foo',
+ name: 'p',
+ secondaryValue: 3,
+ value: 2,
+ },
+ ]);
+ });
});