rusackas commented on code in PR #42761:
URL: https://github.com/apache/superset/pull/42761#discussion_r3723287856
##########
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:
Confirmed, this is exactly the ambiguity `processRecord`'s own comment
already flags as deferred future work, just reachable through the fraction
denominator too. Rather than rush a fix into already-reviewed code here, split
it out to #42810 along with the null-value one below, both fixed and covered by
regression tests there.
##########
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:
Confirmed, fixed in #42810.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:
##########
@@ -242,6 +243,7 @@ export default function transformProps(chartProps:
ChartProps<QueryFormData>) {
columnFormats,
currencyFormats,
metricsLayout,
+ showValuesAs,
Review Comment:
Confirmed, this only touches the React render path, exports and reports
still run the raw `pivot_table_v2()` postprocessing which has no concept of
`showValuesAs`. Filed #42809 to track it since fixing it properly means
teaching the export/report query path to request the same rollup levels Explore
does, not a quick patch.
--
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]
