Skip to content

Commit 85760b8

Browse files
rusackasclaude
andcommitted
fix(pivot-table): keep fractionOf metricAxis/null-numerator consistent
Two issues in the fractionOf denominator lookup flagged in review: - metricAxis was only captured from the first record pushed into a shared Total/corner slot, but `inner` (e.g. cellValue) keeps the value from the last record pushed. For a multi-metric chart this could point the denominator lookup at a different metric than the one the numerator value actually belongs to. metricAxis is now updated on every push so it stays in sync with `inner`. - A null numerator (a DB rollup that's null, rendered blank in actual mode) coerced to 0 under JS's `/` operator, turning a blank cell into a measured 0.0% in fraction mode. `value()` now returns null in that case instead of dividing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent d8d8c72 commit 85760b8

1 file changed

Lines changed: 38 additions & 27 deletions

File tree

  • superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable

superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts

Lines changed: 38 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -749,37 +749,39 @@ const baseAggregatorTemplates = {
749749
],
750750
inner: wrapped(...Array.from(x || []))(data, rowKey, colKey),
751751
// The metric this cell belongs to, and which axis carries it (see the
752-
// "Metric" pseudo-dimension in PivotTableChart). Captured from the
753-
// first pushed record. With multiple metrics, the axis holding the
754-
// metric is never actually empty, so collapsing it to `[]` (as the
755-
// `selector` above does) would route every metric's lookup to the
756-
// same shared total slot -- see `processRecord`'s "Metric-collapse
757-
// totals". Keeping the metric's own key segment instead routes the
758-
// lookup to the per-metric total that's already correctly split out.
752+
// "Metric" pseudo-dimension in PivotTableChart). With multiple
753+
// metrics, the axis holding the metric is never actually empty, so
754+
// collapsing it to `[]` (as the `selector` above does) would route
755+
// every metric's lookup to the same shared total slot -- see
756+
// `processRecord`'s "Metric-collapse totals". Keeping the metric's
757+
// own key segment instead routes the lookup to the per-metric total
758+
// that's already correctly split out. Updated on every push (not
759+
// just the first) to stay in sync with `inner`, which likewise
760+
// reflects the last-pushed record for a shared Total/corner slot --
761+
// otherwise the numerator (last metric) and the denominator lookup
762+
// (first metric's axis) could point at two different metrics.
759763
metricAxis: undefined as
760764
| { axis: 'row' | 'col'; value: string }
761765
| null
762766
| undefined,
763767
push(record: PivotRecord) {
764-
if (this.metricAxis === undefined) {
765-
const metricDim = record.__metricKey as unknown as
766-
| string
767-
| undefined;
768-
const cols = data.props.cols as string[] | undefined;
769-
const rows = data.props.rows as string[] | undefined;
770-
if (metricDim && cols?.includes(metricDim)) {
771-
this.metricAxis = {
772-
axis: 'col',
773-
value: String(record[metricDim]),
774-
};
775-
} else if (metricDim && rows?.includes(metricDim)) {
776-
this.metricAxis = {
777-
axis: 'row',
778-
value: String(record[metricDim]),
779-
};
780-
} else {
781-
this.metricAxis = null;
782-
}
768+
const metricDim = record.__metricKey as unknown as
769+
| string
770+
| undefined;
771+
const cols = data.props.cols as string[] | undefined;
772+
const rows = data.props.rows as string[] | undefined;
773+
if (metricDim && cols?.includes(metricDim)) {
774+
this.metricAxis = {
775+
axis: 'col',
776+
value: String(record[metricDim]),
777+
};
778+
} else if (metricDim && rows?.includes(metricDim)) {
779+
this.metricAxis = {
780+
axis: 'row',
781+
value: String(record[metricDim]),
782+
};
783+
} else {
784+
this.metricAxis = null;
783785
}
784786
this.inner.push(record);
785787
},
@@ -814,7 +816,16 @@ const baseAggregatorTemplates = {
814816
return acc;
815817
}
816818

817-
return this.inner.value() / acc;
819+
const numerator = this.inner.value();
820+
// A `null` numerator (e.g. a DB-computed rollup that is null, as
821+
// `cellValue`'s own comment documents) is intentionally blank in
822+
// actual mode. `null` coerces to `0` under `/`, which would turn
823+
// that blank into a measured `0.0%` instead of staying blank.
824+
if (numerator === null) {
825+
return null;
826+
}
827+
828+
return numerator / acc;
818829
},
819830
getCurrencies() {
820831
return this.inner.getCurrencies ? this.inner.getCurrencies() : [];

0 commit comments

Comments
 (0)