sadpandajoe commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4132977420
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -408,7 +428,7 @@ const cellValue =
}
},
value() {
- return this.val;
+ return this.mixedMetrics ? null : this.val;
Review Comment:
This correctly blanks the mixed-metric slot's own value, but
`fractionOf.value()` (a few hundred lines below, wrapping this aggregator for
"Show values as" percent modes) can still leak a non-blank result: it looks up
the denominator via the last-pushed metric's own total, and if that metric is
string-valued (e.g. `MAX()`/`MIN()` on a text column), its `typeof acc ===
'string'` branch returns that unrelated metric's raw string immediately —
before ever checking whether `this.inner.value()` (this now-null mixed value)
is null. The mixed-metric corner then renders that string instead of staying
blank in "% of Grand Total" (or row/column) mode.
Could `fractionOf.value()` check `this.inner.value() === null` before its
denominator lookup, so a mixed slot always blanks regardless of what type the
last-pushed metric happens to be?
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx:
##########
@@ -1022,7 +1032,7 @@ test('TableRenderer keeps the grand-total corner cell
self-consistent when it mi
.getAllByRole('gridcell')
.filter(cell => cell.classList.contains('pvtGrandTotal'));
expect(grandTotalCells).toHaveLength(1);
- expect(grandTotalCells[0]).toHaveTextContent('100.0%');
+ expect(grandTotalCells[0]).toHaveTextContent('');
Review Comment:
This asserts only the grand-total corner (`.pvtGrandTotal`) blanks when
metrics mix; nothing here or in the two new `utilities.test.ts` cases (which
use `rows: []` and query only `getAggregator([], [])`) asserts a row/column
Total cell for the same fixture. `processRecord`'s metric-collapse mirroring
routes the same mixed records into `rowTotals`/`colTotals` whenever the Metric
axis collapses for a specific row or column (e.g. this fixture's "blue" row
receives both m1=10 and m2=250 into its own Total slot) through the identical
`cellValue`-based aggregator, so a future change that special-cases only the
grand corner would ship a regression here unnoticed.
Could a `.pvtTotal`-classed assertion for the "blue" row (or
`pivotData.getAggregator(['blue'], []).value() === null` in utilities.test.ts)
be added alongside this test?
--
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]
