feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) - #42976
Conversation
Extends ``superset/utils/pandas_postprocessing/pivot.py`` with an optional ``show_values_as`` argument that expresses each metric cell as a fraction of the row / column / grand total after pivoting, mirroring the pivot chart's client-side ``fractionOf`` semantic in ``plugin-chart-pivot-table/src/react-pivottable/utilities.ts:739`` so server-side rendering paths (CSV / XLSX exports, scheduled reports) can eventually reproduce the browser output. Values: - ``percent_row``: cell / row-total (denominator sums across columns) - ``percent_col``: cell / column-total (denominator sums across rows) - ``percent_total``: cell / grand-total (denominator sums the DataFrame) - ``None`` / ``"actual"``: no-op (default) — no behavior change for existing callers of ``pivot`` postprocessing (echarts Timeseries, BigNumber, etc.) Edge cases mirror the client-side apache#42810 guards: - NaN/NULL numerator stays NaN — a genuine SQL NULL renders blank rather than a measured "0.0%". - Zero or NaN denominator produces NaN cells rather than Infinity from division-by-zero. - On a multi-metric pivot (MultiIndex columns) the totals are computed *within each metric group* so metric A's percentages are never contaminated by metric B's values. This is the server-side foundation for apache#42809. The pivot chart's frontend ``buildQuery.ts`` still needs to add ``pivot`` postprocessing with ``show_values_as`` so exports pick it up — that is a separate PR because it requires a design decision on whether to emit the operation for all query paths or only for export-format queries (client tolerance for a pre-pivoted DataFrame). Refs apache#42809
Code Review Agent Run #f9d2c3Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| result = pivot( | ||
| df=df, | ||
| index=["row"], | ||
| columns=["col"], | ||
| aggregates={"v": {"operator": "sum"}}, | ||
| show_values_as="percent_row", | ||
| ) |
There was a problem hiding this comment.
Suggestion: This regression test cannot exercise NaN numerator preservation with operator='sum': pandas skips NaN values and returns zero for the all-NaN pivot cell, so the value is no longer NaN when _div_preserving_nan runs. The assertion that the cell remains NaN therefore fails (or, if the aggregation behavior changes, does not validate the intended SQL NULL case). Use an aggregation/input fixture that preserves the missing value before the percentage transform. [logic error]
Severity Level: Major ⚠️
- ❌ The new regression test fails under the configured sum aggregator.
- ⚠️ NaN-preservation behavior remains unverified.
- ⚠️ CI cannot validate the intended NULL rendering contract.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/pandas_postprocessing/test_pivot.py
**Line:** 564:570
**Comment:**
*Logic Error: This regression test cannot exercise NaN numerator preservation with `operator='sum'`: pandas skips NaN values and returns zero for the all-NaN pivot cell, so the value is no longer NaN when `_div_preserving_nan` runs. The assertion that the cell remains NaN therefore fails (or, if the aggregation behavior changes, does not validate the intended SQL NULL case). Use an aggregation/input fixture that preserves the missing value before the percentage transform.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in 80585a9678. Verified empirically that pd.Series([np.nan]).sum(skipna=True) == 0.0, so the previous fixture never actually produced a NaN pivot cell. Rewrote the fixture to omit the (r1, c2) row entirely — pivot_table then fills that cell with a genuine NaN, which does exercise _div_preserving_nan's NaN-preservation path. Test asserts both that the NaN cell stays NaN AND that the surviving cell divides against just its own value (r1 c1 == 1.0 since the row-total after skipna is 10, not 10+0).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42976 +/- ##
==========================================
- Coverage 66.41% 57.06% -9.36%
==========================================
Files 2858 2861 +3
Lines 161446 161764 +318
Branches 37190 37264 +74
==========================================
- Hits 107222 92305 -14917
- Misses 52187 68598 +16411
+ Partials 2037 861 -1176
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds server-side support for pivot-table “Show values as” percentage modes in the pandas postprocessing pivot() operator, so non-browser rendering paths (CSV/XLSX exports, scheduled reports) can match the pivot table’s client-side fraction display semantics.
Changes:
- Extend
pivot()with an optionalshow_values_asargument to compute percent-of-row/column/grand-total after pivoting, including multi-metric isolation. - Add unit tests covering the new percentage modes and key edge cases (NaN numerator preservation, zero grand total, invalid mode).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
superset/utils/pandas_postprocessing/pivot.py |
Implements show_values_as percent-of-row/col/total post-pivot transforms and validates mode values. |
tests/unit_tests/pandas_postprocessing/test_pivot.py |
Adds regression tests for show_values_as behaviors and edge cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def test_pivot_show_values_as_invalid_mode_raises() -> None: | ||
| """An unknown ``show_values_as`` value raises ``InvalidPostProcessingError`` | ||
| rather than silently falling through to a no-op.""" | ||
| with pytest.raises(InvalidPostProcessingError): | ||
| pivot( |
Per review on apache#42976: 1. **marginal_distributions guard** — combining ``show_values_as`` with ``marginal_distributions`` would include the ``All`` margin row/col in the row/col/grand-total denominators, producing wrong percentages. Raise ``InvalidPostProcessingError`` explicitly rather than silently returning wrong numbers. Combining the two needs a first-class design (probably compute on the non-margin subset then re-insert the margins), out of scope here. 2. **Flat-multi-metric percent_total** — a multi-metric pivot with no ``columns`` groupby produces a flat column index where each column IS a metric. Verified with pandas 2.3.3 that the previous code summed across metrics for the grand total (metric a's magnitude leaking into metric b's percentages). Now iterate each column as its own single-metric block, matching the MultiIndex per-metric semantics. 3. **NaN numerator test was broken** — the previous fixture used ``operator="sum"`` on a value that included ``NaN``; ``pandas`` ``.sum(skipna=True)`` on a single-element ``[NaN]`` group returns ``0.0``, not ``NaN``, so the test never actually exercised ``_div_preserving_nan``'s NaN-preservation path. Rewrote the fixture to use a missing (row, col) combination, which ``pivot_table`` fills with a genuine ``NaN`` cell — verified empirically. 4. **Falsy-string validation** — the previous ``if show_values_as and show_values_as != "actual"`` guard skipped validation for empty strings, silently no-oping bad input. Now an explicit sentinel check ``if show_values_as not in (None, "", "actual")`` rejects unknown modes uniformly and lets both ``None`` and ``""`` route to the no-op path. 5. **New zero-row/col-denominator tests** — pin the guard against division-by-zero producing ``Infinity`` in row/column modes (already handled for grand-total mode). Explicit tests for both axes. 6. **Pandas-3 compat (found while fixing)** — ``df.groupby(level=0, axis=1)`` is deprecated in pandas 2.3 (FutureWarning) and removed in pandas 3.x. Refactored to iterate ``columns.get_level_values(0)`` explicitly with ``df.xs``, avoiding the deprecated call and keeping the fix forward-compatible. All six fix paths verified against pandas 2.3.3 with a standalone repro script before writing tests, mirroring the process discipline learned from apache#42793.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
SUMMARY
First half of #42809. Extends
superset/utils/pandas_postprocessing/pivot.pywith an optionalshow_values_asargument that computes percent-of-row, percent-of-column, or percent-of-grand-total after pivoting, mirroring the client-sidefractionOfsemantic inreact-pivottable/utilities.ts:739(hardened by @rusackas in #42810).WHY THIS IS HALF THE FIX
#42809 has two halves:
pivot()needs to know how to compute percentages. This PR.buildQuery.tsneeds to includepivotpostprocessing (withshow_values_as) in the query so CSV/XLSX exports pick it up. Follow-up PR — needs a design decision on whether to emit the operation always (client would need to tolerate pre-pivoted DataFrames) or only for export flows (result_typein{CSV, XLSX}).@rusackas / @sadpandajoe — happy to fold Path B into this PR if you have a preferred design, or land this first and open the follow-up. Let me know which shape you want.
FIX
show_values_asparam onpivot(), acceptingpercent_row,percent_col,percent_total, orNone/"actual"(no-op).MultiIndexcolumns) the totals are computed within each metric group — never across metrics — so metric A's percentages are never contaminated by metric B's values (matches the client'smetricAxishandling).None— no behavior change for existing callers ofpivotpostprocessing (echarts Timeseries, BigNumber, MixedTimeseries).EDGE CASES (mirror #42810's client-side guards)
0.0%Infinityshow_values_asvalueInvalidPostProcessingError(no silent no-op)BEHAVIOR MATRIX
None/"actual""percent_row""percent_col""percent_total"TESTING INSTRUCTIONS
Eight new tests: actual-is-noop, percent_row, percent_col, percent_total, NaN-numerator-preserved, zero-grand-total-produces-NaN, multi-metric-keeps-metrics-separate, invalid-mode-raises.
ADDITIONAL INFORMATION
Nonedefault)