feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) - #42976
Open
Abdulrehman-PIAIC80387 wants to merge 3 commits into
Open
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
Contributor
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}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42976 +/- ##
==========================================
- Coverage 66.41% 66.34% -0.08%
==========================================
Files 2858 2861 +3
Lines 161446 161770 +324
Branches 37190 37266 +76
==========================================
+ Hits 107222 107320 +98
- Misses 52187 52401 +214
- Partials 2037 2049 +12
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:
|
sadpandajoe
requested review from
eschutho,
rusackas,
sadpandajoe and
villebro
and
a lite review from Copilot
August 10, 2026 22:43
Contributor
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.
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. |
…pivot_table CI caught a real bug in the marginal_distributions test — with ``margins=True`` and no ``margins_name`` value, pandas raises ``ValueError: margins_name argument must be a string`` before our InvalidPostProcessingError guard at the bottom of ``pivot()`` ever fires. Test failed with the pandas error instead of our intended one. Hoisting the ``show_values_as`` validation (unknown-mode + margins-combination) to the *top* of ``pivot()`` — right after the existing index/aggregates validation and before ``pivot_table`` runs — means: 1. The test now sees ``InvalidPostProcessingError`` as intended, regardless of whether ``marginal_distribution_name`` is set. 2. Users misconfiguring the argument get an immediate, actionable error instead of paying for a full pivot they were about to reject. 3. The narrower typing (``percent_mode: Optional[str]``) satisfies mypy without ``cast``/``type: ignore`` at the apply-site. No behavior change for valid configurations.
Contributor
Code Review Agent Run #a17e2fActionable 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 |
rusackas
approved these changes
Aug 13, 2026
rusackas
left a comment
rusackas
left a comment
Member
There was a problem hiding this comment.
Let's land this first and do the frontend wiring as a follow-up, keeps review manageable. Nice work chasing down the codeant and copilot threads, looks like the NaN-numerator test and the zero row/col denominator cases are both fixed in 80585a9. LGTM, approving.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)