sadpandajoe commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4243528709
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -634,6 +766,24 @@ const baseAggregatorTemplates = {
strValue: null as string | null,
currencySet: new Set<string>(),
push(record: PivotRecord) {
+ // A metric's own value can be a real SQL NULL (e.g. AVG over an
+ // empty group). JS coerces `Number(null)` to 0, which would
+ // silently count a missing group as a real zero in the mean/
+ // variance/stdev instead of excluding it -- skip it entirely, the
+ // same way a group with no matching leaf record at all is
+ // excluded, rather than routing it through the NaN/string branch
+ // below (a real SQL NULL is not a string value to display either).
+ if (record[attr] === null || record[attr] === undefined) {
Review Comment:
Skipping NULL inputs here leaves `n === 0` when every leaf in a scope is
NULL, and `value()` then returns `0 / 0`, so the summary renders literal `NaN`
instead of blank (the leaves and the new all-null Sum/Median handling stay
blank). With rows `[region, store]`, one `AVG(sales)` metric,
`aggregateFunction: 'Average'`, row subtotals on, and North/A and North/B both
NULL, the North subtotal shows `NaN`. Could an empty mean scope return `null`
(and the same for the var/stdev modes if they can hit it) so it renders blank
like the other reducers?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:
##########
@@ -759,6 +760,7 @@ export default function PivotTableChart(props:
PivotTableProps) {
onContextMenu={handleContextMenu}
allowRenderHtml={allowRenderHtml}
showValuesAs={showValuesAs}
+ aggregateFunction={aggregateFunction}
Review Comment:
The only coverage of this prop is the `transformProps` assertion on the
returned value and `PivotData`/`TableRenderer` tests that pass
`aggregateFunction` directly, so dropping
`aggregateFunction={aggregateFunction}` here would leave every test green while
a real chart with Median (store values 10 and 30, totals on) loses its result
aggregation and renders a blank grand total. Could a `PivotTableChart.test.tsx`
case render the real chart with that setup and assert the `.pvtGrandTotal` text
is `20.00`?
--
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]
