Abdulrehman-PIAIC80387 commented on PR #42976:
URL: https://github.com/apache/superset/pull/42976#issuecomment-5449191803
Verified all three points empirically and fixed in 6ad7fd5.
**1. SQL NULL through `sum` — confirmed, scoped the contract.**
Reproduced: `pivot_table(aggfunc='sum')` collapses a group of `[NaN]` to
`0.0` (pandas `sum([NaN]) == 0`) before `_div_preserving_nan`'s mask ever sees
it, so the transform returned a measured `0%` for a value that should have
stayed blank. Since reconstructing "blank vs measured zero" from an
already-aggregated value isn't possible at this layer, I've narrowed the
NULL-preservation guarantee to the case where it's actually recoverable — cells
that `pivot_table` left as `NaN` because the (row, column) group had no input
rows at all. That case still round-trips as `NaN`; value-is-NULL cases collapse
to `0%` and are now documented as such on `_apply_show_values_as`.
**2. Non-additive rollup — confirmed, rejected up front.**
Reproduced your AVG example: cells `[5, 10]` from underlying `[0, 10]` and
`[10]`, true row mean `20/3 ≈ 6.67`. Our transform normalized each cell against
`5 + 10 = 15`, yielding `0.333 / 0.667` — sums to 1.0 but doesn't match the DB
rollup any exporter would recompute. Since `sum(cells)` only equals the rollup
for additive operators, `pivot()` now raises `InvalidPostProcessingError` when
`show_values_as` is combined with a non-additive aggregate. The allow-list is
`{"sum", "nansum", "count", "count_nonzero"}` — anything else (mean, median,
min, max, distinct, callables, …) is rejected before the pivot runs. This joins
the existing `marginal_distributions` guard.
**3. Empty pivot crash — confirmed, early-return.**
Reproduced the `ValueError: No objects to concatenate`. Empty pivots with a
column grouping carry `MultiIndex([], names=[...])` with zero level-0 groups,
so the metric-iteration loop fed `pd.concat` an empty list. Added a `df.empty`
early-return at the top of `_apply_show_values_as` — the empty frame passes
through unchanged. Regression test at
`test_pivot_show_values_as_on_empty_pivot_returns_empty_frame`.
Also added regression tests for #1's structural-NaN preservation
(`test_pivot_show_values_as_preserves_structural_nan`) and #2's rejection
(`test_pivot_show_values_as_rejects_non_additive_aggregate`) so the three
inline threads are covered by the diff.
--
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]
