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/08/10 20:03:19 UTC
(superset) branch fix/followup-41442-follow-up-from-41442-fix-sunburst-make-s created (now 0a8e09da243)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a change to branch fix/followup-41442-follow-up-from-41442-fix-sunburst-make-s
in repository https://gitbox.apache.org/repos/asf/superset.git
at 0a8e09da243 fix(sunburst): keep SQL null and literal string "null" as distinct groups
This branch includes the following new commits:
new 0a8e09da243 fix(sunburst): keep SQL null and literal string "null" as distinct groups
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/01: fix(sunburst): keep SQL null and literal string "null" as distinct groups
Posted by ru...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix/followup-41442-follow-up-from-41442-fix-sunburst-make-s
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 0a8e09da2435d595e3434f3edcea4be690ddf1d9
Author: rusackas <ev...@rusackas.com>
AuthorDate: Mon Aug 10 13:02:55 2026 -0700
fix(sunburst): keep SQL null and literal string "null" as distinct groups
Follow-up to #41442. The Sunburst chart's null-filtering path grouped
records with lodash's `groupBy`, which coerces the group-by field's value
to a string to use it as an object property key. That collapses a SQL
NULL and the literal string "null" into the same "null" bucket, merging
otherwise-distinct data and making the "Show Null Values" filter
non-deterministic (it filters out or keeps both together, depending on
which row lands in the group first).
Regroups with a Map instead of a plain-object accumulator, so keys keep
their original type and identity instead of being stringified. SQL NULL
and the string "null" now always produce separate tree nodes, and the
existing `node.name !== null` filter check only removes the real null.
Co-Authored-By: Claude <no...@anthropic.com>
---
.../plugin-chart-echarts/src/utils/treeBuilder.ts | 108 ++++++++++++---------
.../test/utils/treeBuilder.test.ts | 38 ++++++++
2 files changed, 101 insertions(+), 45 deletions(-)
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 a0fbb34640f..e4a754e35cf 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/treeBuilder.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/treeBuilder.ts
@@ -17,7 +17,6 @@
* under the License.
*/
import { DataRecord, DataRecordValue } from '@superset-ui/core';
-import { groupBy as _groupBy, transform } from 'lodash-es';
export type TreeNode = {
name: DataRecordValue;
@@ -31,6 +30,30 @@ function getMetricValue(datum: DataRecord, metric: string) {
return typeof datum[metric] === 'number' ? (datum[metric] as number) : 0;
}
+// Groups records by the raw value of `groupByKey`, keyed with a Map instead
+// of a plain object. A plain-object accumulator (e.g. lodash's `groupBy`)
+// has to coerce every key to a string to use it as a property name, so a SQL
+// NULL and the literal string "null" both collapse to the same "null" key
+// and get merged into a single group. A Map keeps them as distinct keys, so
+// null filtering stays deterministic regardless of what else is in the
+// column.
+function groupByValue(
+ data: DataRecord[],
+ groupByKey: string,
+): Map<DataRecordValue, DataRecord[]> {
+ const groups = new Map<DataRecordValue, DataRecord[]>();
+ data.forEach(datum => {
+ const key = datum[groupByKey];
+ const group = groups.get(key);
+ if (group) {
+ group.push(datum);
+ } else {
+ groups.set(key, [datum]);
+ }
+ });
+ return groups;
+}
+
export function treeBuilder(
data: DataRecord[],
groupBy: string[],
@@ -39,57 +62,52 @@ export function treeBuilder(
filterNullNames?: boolean,
): TreeNode[] {
const [curGroupBy, ...restGroupby] = groupBy;
- const curData = _groupBy(data, curGroupBy);
- const nodes = transform(
- curData,
- (result, value, key) => {
- const name = curData[key][0][curGroupBy]!;
- if (!restGroupby.length) {
- (value ?? []).forEach(datum => {
- const metricValue = getMetricValue(datum, metric);
- const secondaryValue = secondaryMetric
- ? getMetricValue(datum, secondaryMetric)
- : metricValue;
- const item = {
- name,
- value: metricValue,
- secondaryValue,
- groupBy: curGroupBy,
- };
- 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,
- metric,
- secondaryMetric,
- filterNullNames,
- );
- const metricValue = children.reduce(
- (prev, cur) => prev + (cur.value as number),
- 0,
- );
+ const curData = groupByValue(data, curGroupBy);
+ const nodes: TreeNode[] = [];
+ curData.forEach((value, name) => {
+ if (!restGroupby.length) {
+ value.forEach(datum => {
+ const metricValue = getMetricValue(datum, metric);
const secondaryValue = secondaryMetric
- ? children.reduce(
- (prev, cur) => prev + (cur.secondaryValue as number),
- 0,
- )
+ ? getMetricValue(datum, secondaryMetric)
: metricValue;
- result.push({
+ nodes.push({
name,
- children,
value: metricValue,
secondaryValue,
groupBy: curGroupBy,
});
- }
- },
- [] as TreeNode[],
- );
+ });
+ } 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,
+ metric,
+ secondaryMetric,
+ filterNullNames,
+ );
+ const metricValue = children.reduce(
+ (prev, cur) => prev + (cur.value as number),
+ 0,
+ );
+ const secondaryValue = secondaryMetric
+ ? children.reduce(
+ (prev, cur) => prev + (cur.secondaryValue as number),
+ 0,
+ )
+ : metricValue;
+ nodes.push({
+ name,
+ children,
+ value: metricValue,
+ secondaryValue,
+ groupBy: curGroupBy,
+ });
+ }
+ });
// Filter at every level so single-level charts and root nodes are covered,
// not just nested children. A parent whose children were all null-filtered
// is dropped too: keeping it would leave a zero-value arc that yields a NaN
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 ca5dd2a9622..f040de4b8ad 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
@@ -646,4 +646,42 @@ describe('test treeBuilder', () => {
},
]);
});
+
+ // Regression: a plain-object grouping accumulator coerces every key to a
+ // string, so a SQL NULL and the literal string "null" both land under the
+ // "null" property and merge into one group. Keep them as separate nodes.
+ test('keeps a SQL null and the literal string "null" as distinct groups', () => {
+ const tree = treeBuilder(
+ [
+ { foo: null, count: 2, count2: 3 },
+ { foo: 'null', count: 5, count2: 7 },
+ { foo: 'a', count: 1, count2: 1 },
+ ],
+ ['foo'],
+ 'count',
+ );
+ expect(tree).toEqual([
+ { groupBy: 'foo', name: null, secondaryValue: 2, value: 2 },
+ { groupBy: 'foo', name: 'null', secondaryValue: 5, value: 5 },
+ { groupBy: 'foo', name: 'a', secondaryValue: 1, value: 1 },
+ ]);
+ });
+
+ test('filters only the SQL null, not the literal string "null"', () => {
+ const tree = treeBuilder(
+ [
+ { foo: null, count: 2, count2: 3 },
+ { foo: 'null', count: 5, count2: 7 },
+ { foo: 'a', count: 1, count2: 1 },
+ ],
+ ['foo'],
+ 'count',
+ undefined,
+ true,
+ );
+ expect(tree).toEqual([
+ { groupBy: 'foo', name: 'null', secondaryValue: 5, value: 5 },
+ { groupBy: 'foo', name: 'a', secondaryValue: 1, value: 1 },
+ ]);
+ });
});