rusackas commented on code in PR #42761:
URL: https://github.com/apache/superset/pull/42761#discussion_r3723319920
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -748,14 +748,67 @@ const baseAggregatorTemplates = {
type
],
inner: wrapped(...Array.from(x || []))(data, rowKey, colKey),
+ // The metric this cell belongs to, and which axis carries it (see
the
+ // "Metric" pseudo-dimension in PivotTableChart). Captured from the
+ // first pushed record. With multiple metrics, the axis holding the
+ // metric is never actually empty, so collapsing it to `[]` (as the
+ // `selector` above does) would route every metric's lookup to the
+ // same shared total slot -- see `processRecord`'s "Metric-collapse
+ // totals". Keeping the metric's own key segment instead routes the
+ // lookup to the per-metric total that's already correctly split out.
+ metricAxis: undefined as
+ | { axis: 'row' | 'col'; value: string }
+ | null
+ | undefined,
push(record: PivotRecord) {
+ if (this.metricAxis === undefined) {
Review Comment:
Good catch. metricAxis was only set from the first push, but inner keeps
overwriting with the last one for a shared Total slot, so I switched it to
update on every push so it stays in sync with inner. Doesn't fully solve the
underlying 'cross-metric total isn't well-defined' limitation already called
out in processRecord, that's still true and still future work, but at least
numerator and denominator now agree on which metric they're looking at instead
of picking two different ones. Pushed.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -748,14 +748,67 @@ const baseAggregatorTemplates = {
type
],
inner: wrapped(...Array.from(x || []))(data, rowKey, colKey),
+ // The metric this cell belongs to, and which axis carries it (see
the
+ // "Metric" pseudo-dimension in PivotTableChart). Captured from the
+ // first pushed record. With multiple metrics, the axis holding the
+ // metric is never actually empty, so collapsing it to `[]` (as the
+ // `selector` above does) would route every metric's lookup to the
+ // same shared total slot -- see `processRecord`'s "Metric-collapse
+ // totals". Keeping the metric's own key segment instead routes the
+ // lookup to the per-metric total that's already correctly split out.
+ metricAxis: undefined as
+ | { axis: 'row' | 'col'; value: string }
+ | null
+ | undefined,
push(record: PivotRecord) {
+ if (this.metricAxis === undefined) {
+ const metricDim = record.__metricKey as unknown as
+ | string
+ | undefined;
+ const cols = data.props.cols as string[] | undefined;
+ const rows = data.props.rows as string[] | undefined;
+ if (metricDim && cols?.includes(metricDim)) {
+ this.metricAxis = {
+ axis: 'col',
+ value: String(record[metricDim]),
+ };
+ } else if (metricDim && rows?.includes(metricDim)) {
+ this.metricAxis = {
+ axis: 'row',
+ value: String(record[metricDim]),
+ };
+ } else {
+ this.metricAxis = null;
+ }
+ }
this.inner.push(record);
},
format: fmtNonString(formatter),
value() {
- const acc = data
- .getAggregator(...Array.from(this.selector || []))
- .inner.value();
+ // `buildGroupbyCombinations` requests the denominator's rollup
+ // level whenever a percent `showValuesAs` is selected, but fall
+ // back to `null` (rendered blank) instead of throwing if it is
+ // ever missing -- e.g. a denominator aggregator with no matching
+ // rows in the response.
+ let [selRow, selCol] = (this.selector || [[], []]) as [
+ string[],
+ string[],
+ ];
+ if (this.metricAxis) {
+ if (this.metricAxis.axis === 'col' && selCol.length === 0) {
+ selCol = [this.metricAxis.value];
+ } else if (
+ this.metricAxis.axis === 'row' &&
+ selRow.length === 0
+ ) {
+ selRow = [this.metricAxis.value];
+ }
+ }
+ const denominatorAggregator = data.getAggregator(selRow, selCol);
+ if (!denominatorAggregator.inner) {
Review Comment:
Good catch, null/acc does coerce to 0 under JS division. Added a check to
return null when the numerator itself is null so it stays blank like actual
mode instead of showing a measured 0.0%. Pushed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
