You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Abdulrehman-PIAIC80387 (via GitHub)" <gi...@apache.org> on 2026/08/10 07:44:45 UTC
[PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Abdulrehman-PIAIC80387 opened a new pull request, #42976:
URL: https://github.com/apache/superset/pull/42976
### SUMMARY
First half of #42809. Extends `superset/utils/pandas_postprocessing/pivot.py` with an optional `show_values_as` argument that computes percent-of-row, percent-of-column, or percent-of-grand-total after pivoting, mirroring the client-side `fractionOf` semantic in [`react-pivottable/utilities.ts:739`](../blob/master/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts#L739) (hardened by @rusackas in #42810).
### WHY THIS IS HALF THE FIX
#42809 has two halves:
1. **Server-side capability** — `pivot()` needs to know how to compute percentages. **This PR.**
2. **Frontend wiring** — the pivot chart's `buildQuery.ts` needs to include `pivot` postprocessing (with `show_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_type` in `{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
- Adds `show_values_as` param on `pivot()`, accepting `percent_row`, `percent_col`, `percent_total`, or `None` / `"actual"` (no-op).
- Post-pivot, when the mode is set, each cell is divided by the appropriate rollup total.
- On a **multi-metric pivot** (`MultiIndex` columns) 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's `metricAxis` handling).
- Default is `None` — no behavior change for existing callers of `pivot` postprocessing (echarts Timeseries, BigNumber, MixedTimeseries).
### EDGE CASES (mirror #42810's client-side guards)
| Guard | Behavior |
|---|---|
| NaN/NULL numerator | Stays NaN (renders blank) — a genuine SQL NULL does not become `0.0%` |
| Zero row/column denominator | Cells for that row/column become NaN, not `Infinity` |
| Zero grand total | All cells become NaN |
| Invalid `show_values_as` value | Raises `InvalidPostProcessingError` (no silent no-op) |
### BEHAVIOR MATRIX
| Mode | Effect |
|---|---|
| `None` / `"actual"` | pivoted DataFrame unchanged (no-op) — default |
| `"percent_row"` | each cell = cell / row-total |
| `"percent_col"` | each cell = cell / column-total |
| `"percent_total"` | each cell = cell / grand-total |
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/pandas_postprocessing/test_pivot.py -v -k show_values_as
```
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
- [x] Has associated issue: partial fix for #42809
- [x] Required feature flags: none
- [x] Changes UI: no
- [x] Includes DB Migration: no
- [x] Includes CLI or Node.js commands: no
- [x] Breaking change: no (additive param with `None` default)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42976:
URL: https://github.com/apache/superset/pull/42976#issuecomment-5355870508
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "Abdulrehman-PIAIC80387 (via GitHub)" <gi...@apache.org>.
Abdulrehman-PIAIC80387 commented on code in PR #42976:
URL: https://github.com/apache/superset/pull/42976#discussion_r3878498131
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -26,6 +27,92 @@
validate_column_args,
)
+_PERCENT_MODES = frozenset({"percent_row", "percent_col", "percent_total"})
+
+
+def _div_preserving_nan(numerator: DataFrame, denominator: Any, axis: int) -> DataFrame:
+ """Divide ``numerator`` by ``denominator``, preserving NaN numerators.
+
+ A genuine SQL NULL numerator must stay NaN (rendered blank) rather than
+ become ``0.0`` — matches the client-side #42810 semantics guarding
+ against measured "0.0%" values for values that should stay blank.
+ """
+ result = numerator.div(denominator, axis=axis)
+ return result.mask(numerator.isna(), other=float("nan"))
Review Comment:
Addressed in 6ad7fd5 — kept the guarantee for structurally-missing cells only, and documented on the docstring that value-is-NULL rows collapse to `0.0` inside `pivot_table` before this mask runs, so they render as `0%` not blank. Full write-up in the top-level comment: https://github.com/apache/superset/pull/42976#issuecomment-5449191803
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -26,6 +27,92 @@
validate_column_args,
)
+_PERCENT_MODES = frozenset({"percent_row", "percent_col", "percent_total"})
+
+
+def _div_preserving_nan(numerator: DataFrame, denominator: Any, axis: int) -> DataFrame:
+ """Divide ``numerator`` by ``denominator``, preserving NaN numerators.
+
+ A genuine SQL NULL numerator must stay NaN (rendered blank) rather than
+ become ``0.0`` — matches the client-side #42810 semantics guarding
+ against measured "0.0%" values for values that should stay blank.
+ """
+ result = numerator.div(denominator, axis=axis)
+ return result.mask(numerator.isna(), other=float("nan"))
+
+
+def _apply_percent_transform_to_group(g: DataFrame, mode: str) -> DataFrame:
+ """Apply a percent-of-{row,col,total} transform to a single-metric block.
+
+ Called both for the whole DataFrame when it holds one metric, and
+ per-metric-group for MultiIndex / flat-multi-metric pivots. A zero or
+ NaN denominator produces NaN cells rather than ``Infinity``/``NaN``
+ from division-by-zero, matching the client's ``if (acc === null)
+ return null`` guard in ``fractionOf``.
+ """
+ if mode == "percent_row":
+ row_totals = g.sum(axis=PandasAxis.COLUMN, skipna=True).replace(0, float("nan"))
Review Comment:
Addressed in 6ad7fd5 — `show_values_as` now rejects non-additive aggregates up front with `InvalidPostProcessingError`. Allow-list is `{sum, nansum, count, count_nonzero}`; mean / median / min / max / distinct / callables are all rejected before the pivot runs. Regression test `test_pivot_show_values_as_rejects_non_additive_aggregate`. Full write-up: https://github.com/apache/superset/pull/42976#issuecomment-5449191803
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -26,6 +27,92 @@
validate_column_args,
)
+_PERCENT_MODES = frozenset({"percent_row", "percent_col", "percent_total"})
+
+
+def _div_preserving_nan(numerator: DataFrame, denominator: Any, axis: int) -> DataFrame:
+ """Divide ``numerator`` by ``denominator``, preserving NaN numerators.
+
+ A genuine SQL NULL numerator must stay NaN (rendered blank) rather than
+ become ``0.0`` — matches the client-side #42810 semantics guarding
+ against measured "0.0%" values for values that should stay blank.
+ """
+ result = numerator.div(denominator, axis=axis)
+ return result.mask(numerator.isna(), other=float("nan"))
+
+
+def _apply_percent_transform_to_group(g: DataFrame, mode: str) -> DataFrame:
+ """Apply a percent-of-{row,col,total} transform to a single-metric block.
+
+ Called both for the whole DataFrame when it holds one metric, and
+ per-metric-group for MultiIndex / flat-multi-metric pivots. A zero or
+ NaN denominator produces NaN cells rather than ``Infinity``/``NaN``
+ from division-by-zero, matching the client's ``if (acc === null)
+ return null`` guard in ``fractionOf``.
+ """
+ if mode == "percent_row":
+ row_totals = g.sum(axis=PandasAxis.COLUMN, skipna=True).replace(0, float("nan"))
+ return _div_preserving_nan(g, row_totals, axis=PandasAxis.ROW)
+ if mode == "percent_col":
+ col_totals = g.sum(axis=PandasAxis.ROW, skipna=True).replace(0, float("nan"))
+ return _div_preserving_nan(g, col_totals, axis=PandasAxis.COLUMN)
+ # percent_total
+ grand = g.sum(skipna=True).sum(skipna=True)
+ if pd.isna(grand) or grand == 0:
+ return g * float("nan")
+ return _div_preserving_nan(g, grand, axis=PandasAxis.ROW)
+
+
+def _apply_show_values_as(df: DataFrame, mode: str) -> DataFrame:
+ """Divide each metric cell by the appropriate rollup total.
+
+ Mirrors the client-side ``fractionOf`` semantic in
+ ``plugin-chart-pivot-table/src/react-pivottable/utilities.ts:739``:
+
+ - ``percent_row``: cell / row-total (sum across the columns axis)
+ - ``percent_col``: cell / column-total (sum across the rows axis)
+ - ``percent_total``: cell / grand-total (sum of the metric block)
+
+ Per-metric isolation — the totals are computed *within each metric* so
+ one metric's numerator is never divided by another metric's total,
+ matching the client's ``metricAxis`` handling. This applies to both
+ shapes ``pivot_table`` can produce:
+
+ - **MultiIndex columns** (level 0 = metric): iterate the level-0
+ groups explicitly. Explicit iteration avoids the deprecated
+ ``df.groupby(level=0, axis=1)`` pattern (removed in pandas 3.x).
+ - **Flat columns with >1 column** (multi-metric pivot with no
+ ``columns`` groupby — each column IS a metric): treat each column
+ as its own single-column metric block.
+ - **Flat columns with 1 column** (single-metric pivot with no
+ ``columns`` groupby): the whole block is one metric.
+ """
+ is_multi_metric_wide = isinstance(df.columns, pd.MultiIndex)
+ is_flat_multi_metric = not is_multi_metric_wide and df.shape[1] > 1
+
+ if is_multi_metric_wide:
+ # Iterate level-0 groups explicitly (pandas-3-safe).
+ metrics = df.columns.get_level_values(0).unique()
+ parts = []
+ for metric in metrics:
+ block = df.xs(metric, axis=PandasAxis.COLUMN, level=0, drop_level=False)
+ parts.append(_apply_percent_transform_to_group(block, mode))
+ # ``concat`` along columns preserves the MultiIndex; reorder to
+ # match the original column layout deterministically.
+ combined = pd.concat(parts, axis=PandasAxis.COLUMN)
Review Comment:
Addressed in 6ad7fd5 — added a `df.empty` early-return at the top of `_apply_show_values_as`, so this `concat` is never reached with an empty list. Zero-row regression test `test_pivot_show_values_as_on_empty_pivot_returns_empty_frame`. Full write-up: https://github.com/apache/superset/pull/42976#issuecomment-5449191803
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #42976:
URL: https://github.com/apache/superset/pull/42976#issuecomment-5250369312
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 80585a96781e56472bc9a2fdcb7469d53d5c9745 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a7ad0ee0d561400083539b6 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-42976--superset-docs-preview.netlify.app](https://deploy-preview-42976--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctbDkAwEEDRvcy3xqCtxw4sY5gKCTpRz4i969-9H-cF2fwiOzQACXQbrf0Ye5juVKbT70poZQoqjP5SJ82HCyquziusI2Ans39ajsRSSYzOIRubaUSsClPUnYXvBw) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-42976--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #42976:
URL: https://github.com/apache/superset/pull/42976#discussion_r3753984651
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -168,4 +261,15 @@ def pivot( # pylint: disable=too-many-arguments
# be silently dropped by stack's default dropna=True behavior.
df = df.stack(level=0, dropna=False).unstack()
+ if show_values_as and show_values_as != "actual":
+ if show_values_as not in _PERCENT_MODES:
+ raise InvalidPostProcessingError(
+ _(
+ "Unsupported show_values_as value: %(mode)s. "
+ "Expected one of: percent_row, percent_col, percent_total, actual.",
+ mode=show_values_as,
+ )
+ )
+ df = _apply_show_values_as(df, show_values_as)
Review Comment:
`show_values_as` validation is skipped for falsy strings (e.g. `""`) because the guard uses truthiness (`if show_values_as and ...`). That contradicts the documented behavior that invalid `show_values_as` values should raise, and can lead to silent no-ops for bad inputs. Use an explicit sentinel check (None/"actual") and validate everything else.
##########
tests/unit_tests/pandas_postprocessing/test_pivot.py:
##########
@@ -446,3 +446,191 @@ def test_pivot_only_entirely_absent_metrics_are_restored():
assert ("metric_partial", "A") in df.columns
assert ("metric_partial", "B") not in df.columns
assert df[("metric_partial", "A")].iloc[0] == 1.0
+
+
+# --- show_values_as regression tests (#42809) --------------------------------
+#
+# ``show_values_as`` expresses each metric cell as a fraction of the row,
+# column, or grand total after pivoting. Mirrors the 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)
+# match the browser output. See #42809.
+#
+# Fixture: a tiny 3-column DataFrame that keeps row/col/grand totals easy
+# to eyeball. Two rows (``r1``, ``r2``), two columns (``c1``, ``c2``),
+# single metric ``v``. Grand total is 100 so every percent-of-total
+# assertion is trivially checkable.
+
+
+def _show_values_as_fixture() -> DataFrame:
+ """Long-format input that pivots to::
+
+ v
+ col c1 c2
+ row
+ r1 10 20
+ r2 30 40
+
+ row totals: r1=30, r2=70; col totals: c1=40, c2=60; grand=100.
+ """
+ return DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "v": [10, 20, 30, 40],
+ }
+ )
+
+
+def test_pivot_show_values_as_actual_is_noop() -> None:
+ """``show_values_as='actual'`` (and ``None``) leaves values unchanged."""
+ df = _show_values_as_fixture()
+ aggregates = {"v": {"operator": "sum"}}
+ baseline = pivot(df=df, index=["row"], columns=["col"], aggregates=aggregates)
+
+ for mode in (None, "actual"):
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates=aggregates,
+ show_values_as=mode,
+ )
+ pd.testing.assert_frame_equal(result, baseline)
+
+
+def test_pivot_show_values_as_percent_row() -> None:
+ """Each cell = cell / row-total; each row sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_row",
+ )
+ # r1: 10/30, 20/30; r2: 30/70, 40/70
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(10 / 30)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(20 / 30)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(30 / 70)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(40 / 70)
+ assert result.sum(axis=1).tolist() == pytest.approx([1.0, 1.0])
+
+
+def test_pivot_show_values_as_percent_col() -> None:
+ """Each cell = cell / column-total; each column sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_col",
+ )
+ # c1 total=40: 10/40, 30/40; c2 total=60: 20/60, 40/60
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(10 / 40)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(30 / 40)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(20 / 60)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(40 / 60)
+ assert result.sum(axis=0).tolist() == pytest.approx([1.0, 1.0])
+
+
+def test_pivot_show_values_as_percent_total() -> None:
+ """Each cell = cell / grand-total; the whole frame sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_total",
+ )
+ # grand=100: each cell divided by 100
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(0.10)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(0.20)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(0.30)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(0.40)
+ assert result.values.sum() == pytest.approx(1.0)
+
+
+def test_pivot_show_values_as_preserves_nan_numerator() -> None:
+ """A NaN/NULL numerator stays NaN — matches the client-side #42810 guard
+ that a genuine SQL NULL should render blank, not "0.0%"."""
+ df = DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "v": [10, np.nan, 30, 40],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_row",
+ )
+ # The NaN numerator stays NaN — it did not become 0.0
+ assert pd.isna(result.loc["r1", ("v", "c2")])
+ # The other cells still divide correctly (r1 row total is now just 10)
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(1.0)
+
+
+def test_pivot_show_values_as_percent_total_zero_grand_total_yields_nan() -> None:
+ """Grand total of zero yields NaN cells rather than Infinity — matches the
+ client's ``if (acc === null) return null`` division-by-zero guard."""
+ df = DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "v": [0, 0, 0, 0],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_total",
+ )
+ # No cell should be Infinity or a real number; all should be NaN.
+ assert result.isna().values.all()
+
+
+def test_pivot_show_values_as_percent_row_multi_metric_keeps_metrics_separate() -> None:
+ """On a multi-metric pivot (``MultiIndex`` columns), per-row totals are
+ computed *within each metric*. Metric A's percentages must sum to 1.0
+ per row independent of metric B's values."""
+ df = DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "a": [10, 20, 30, 40],
+ "b": [1, 3, 5, 7],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates={"a": {"operator": "sum"}, "b": {"operator": "sum"}},
+ show_values_as="percent_row",
+ )
+ # Metric ``a``: row totals 30 and 70; each row of ``a`` sums to 1.
+ assert result["a"].sum(axis=1).tolist() == pytest.approx([1.0, 1.0])
+ # Metric ``b``: row totals 4 and 12; each row of ``b`` sums to 1.
+ assert result["b"].sum(axis=1).tolist() == pytest.approx([1.0, 1.0])
+ # Metric ``a`` percentages must not be contaminated by metric ``b`` values.
+ assert result.loc["r1", ("a", "c1")] == pytest.approx(10 / 30)
+ assert result.loc["r1", ("b", "c1")] == pytest.approx(1 / 4)
+
+
+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(
Review Comment:
Add regression tests for zero row/column denominators so percent-of-row/col produces NaN (not Infinity/0) when the rollup total is zero.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #42976:
URL: https://github.com/apache/superset/pull/42976#discussion_r3747577119
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -26,6 +27,90 @@
validate_column_args,
)
+_PERCENT_MODES = frozenset({"percent_row", "percent_col", "percent_total"})
+
+
+def _div_preserving_nan(numerator: DataFrame, denominator: Any, axis: int) -> DataFrame:
+ """Divide ``numerator`` by ``denominator``, preserving NaN numerators.
+
+ A genuine SQL NULL numerator must stay NaN (rendered blank) rather than
+ become ``0.0`` — matches the client-side #42810 semantics guarding
+ against measured "0.0%" values for values that should stay blank.
+ """
+ result = numerator.div(denominator, axis=axis)
+ return result.mask(numerator.isna(), other=float("nan"))
+
+
+def _apply_show_values_as(df: DataFrame, mode: str) -> DataFrame:
+ """Divide each metric cell by the appropriate rollup total.
+
+ Mirrors the client-side ``fractionOf`` semantic in
+ ``superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:739``:
+
+ - ``percent_row``: cell / row-total (denominator: sum across the columns axis)
+ - ``percent_col``: cell / column-total (denominator: sum across the rows axis)
+ - ``percent_total``: cell / grand-total (denominator: sum of the whole DataFrame)
+
+ On a multi-metric pivot (``MultiIndex`` columns) the totals are computed
+ *within each metric group* — never across metrics — so per-metric totals
+ stay separate, matching the client's ``metricAxis`` handling which never
+ conflates one metric's numerator with another metric's denominator.
+
+ A zero or NaN denominator produces NaN cells rather than
+ ``Infinity``/``NaN`` from division-by-zero, matching the client's
+ ``if (acc === null) return null`` guard.
+ """
+ is_multi_metric = isinstance(df.columns, pd.MultiIndex)
+
+ if mode == "percent_row":
+ if is_multi_metric:
+ return df.groupby(level=0, axis=1, group_keys=False).apply(
+ lambda g: _div_preserving_nan(
+ g,
+ g.sum(axis=PandasAxis.COLUMN, skipna=True).replace(0, float("nan")),
+ axis=PandasAxis.ROW,
+ )
+ )
Review Comment:
**Suggestion:** When `marginal_distributions` is enabled, this operates on a DataFrame that already contains the `All` row and/or column. The row denominator therefore includes the marginal column, and analogous code includes marginal rows or duplicated grand totals for the other modes, so ordinary cells no longer represent fractions of the requested data scope. Compute percentages from the non-margin cells or handle marginal cells according to the rollup semantics. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Pivot percentages are incorrect when margins are enabled.
- ⚠️ Row and column totals become percentage inputs.
- ⚠️ CSV/XLSX output can disagree with requested fractions.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dc49adc8aef747769531c280853198c2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=dc49adc8aef747769531c280853198c2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/pandas_postprocessing/pivot.py
**Line:** 67:73
**Comment:**
*Logic Error: When `marginal_distributions` is enabled, this operates on a DataFrame that already contains the `All` row and/or column. The row denominator therefore includes the marginal column, and analogous code includes marginal rows or duplicated grand totals for the other modes, so ordinary cells no longer represent fractions of the requested data scope. Compute percentages from the non-margin cells or handle marginal cells according to the rollup semantics.
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 fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42976&comment_hash=e1203d2aaea91a4b02ef9a80957e8b3fe4ba983a0dee18c5fa1ecbde2cbccf04&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42976&comment_hash=e1203d2aaea91a4b02ef9a80957e8b3fe4ba983a0dee18c5fa1ecbde2cbccf04&reaction=dislike'>👎</a>
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -26,6 +27,90 @@
validate_column_args,
)
+_PERCENT_MODES = frozenset({"percent_row", "percent_col", "percent_total"})
+
+
+def _div_preserving_nan(numerator: DataFrame, denominator: Any, axis: int) -> DataFrame:
+ """Divide ``numerator`` by ``denominator``, preserving NaN numerators.
+
+ A genuine SQL NULL numerator must stay NaN (rendered blank) rather than
+ become ``0.0`` — matches the client-side #42810 semantics guarding
+ against measured "0.0%" values for values that should stay blank.
+ """
+ result = numerator.div(denominator, axis=axis)
+ return result.mask(numerator.isna(), other=float("nan"))
+
+
+def _apply_show_values_as(df: DataFrame, mode: str) -> DataFrame:
+ """Divide each metric cell by the appropriate rollup total.
+
+ Mirrors the client-side ``fractionOf`` semantic in
+ ``superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:739``:
+
+ - ``percent_row``: cell / row-total (denominator: sum across the columns axis)
+ - ``percent_col``: cell / column-total (denominator: sum across the rows axis)
+ - ``percent_total``: cell / grand-total (denominator: sum of the whole DataFrame)
+
+ On a multi-metric pivot (``MultiIndex`` columns) the totals are computed
+ *within each metric group* — never across metrics — so per-metric totals
+ stay separate, matching the client's ``metricAxis`` handling which never
+ conflates one metric's numerator with another metric's denominator.
+
+ A zero or NaN denominator produces NaN cells rather than
+ ``Infinity``/``NaN`` from division-by-zero, matching the client's
+ ``if (acc === null) return null`` guard.
+ """
+ is_multi_metric = isinstance(df.columns, pd.MultiIndex)
+
+ if mode == "percent_row":
+ if is_multi_metric:
+ return df.groupby(level=0, axis=1, group_keys=False).apply(
+ lambda g: _div_preserving_nan(
+ g,
+ g.sum(axis=PandasAxis.COLUMN, skipna=True).replace(0, float("nan")),
+ axis=PandasAxis.ROW,
+ )
+ )
+ return _div_preserving_nan(
+ df,
+ df.sum(axis=PandasAxis.COLUMN, skipna=True).replace(0, float("nan")),
+ axis=PandasAxis.ROW,
+ )
+
+ if mode == "percent_col":
+ if is_multi_metric:
+ return df.groupby(level=0, axis=1, group_keys=False).apply(
+ lambda g: _div_preserving_nan(
+ g,
+ g.sum(axis=PandasAxis.ROW, skipna=True).replace(0, float("nan")),
+ axis=PandasAxis.COLUMN,
+ )
+ )
+ return _div_preserving_nan(
+ df,
+ df.sum(axis=PandasAxis.ROW, skipna=True).replace(0, float("nan")),
+ axis=PandasAxis.COLUMN,
+ )
+
+ if mode == "percent_total":
+ if is_multi_metric:
+
+ def _per_metric_total(g: DataFrame) -> DataFrame:
+ grand = g.sum(skipna=True).sum(skipna=True)
+ if pd.isna(grand) or grand == 0:
+ return g * float("nan")
+ return _div_preserving_nan(g, grand, axis=PandasAxis.ROW)
+
+ return df.groupby(level=0, axis=1, group_keys=False).apply(
+ _per_metric_total
+ )
+ grand = df.sum(skipna=True).sum(skipna=True)
+ if pd.isna(grand) or grand == 0:
+ return df * float("nan")
+ return _div_preserving_nan(df, grand, axis=PandasAxis.ROW)
Review Comment:
**Suggestion:** With no `columns` groupby and multiple metrics, the pivot has a flat column index and enters this branch, where the grand total sums all metric columns together. This violates the documented per-metric behavior and makes one metric's percentage depend on the magnitude of unrelated metrics. Compute the total independently for each metric even when the resulting columns are flat. [incorrect variable usage]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Percent-of-total values are wrong for flat multi-metric pivots.
- ⚠️ One metric's scale changes another metric's percentages.
- ⚠️ Metric-specific pivot exports report misleading proportions.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7fc45044872742d1aeb6def04546212d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7fc45044872742d1aeb6def04546212d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/pandas_postprocessing/pivot.py
**Line:** 107:110
**Comment:**
*Incorrect Variable Usage: With no `columns` groupby and multiple metrics, the pivot has a flat column index and enters this branch, where the grand total sums all metric columns together. This violates the documented per-metric behavior and makes one metric's percentage depend on the magnitude of unrelated metrics. Compute the total independently for each metric even when the resulting columns are flat.
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 fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42976&comment_hash=9389ffcedd24a0c434e7175c3003bdfad95c3cf71617ac9b49c1fbca363e3435&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42976&comment_hash=9389ffcedd24a0c434e7175c3003bdfad95c3cf71617ac9b49c1fbca363e3435&reaction=dislike'>👎</a>
##########
tests/unit_tests/pandas_postprocessing/test_pivot.py:
##########
@@ -446,3 +446,191 @@ def test_pivot_only_entirely_absent_metrics_are_restored():
assert ("metric_partial", "A") in df.columns
assert ("metric_partial", "B") not in df.columns
assert df[("metric_partial", "A")].iloc[0] == 1.0
+
+
+# --- show_values_as regression tests (#42809) --------------------------------
+#
+# ``show_values_as`` expresses each metric cell as a fraction of the row,
+# column, or grand total after pivoting. Mirrors the 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)
+# match the browser output. See #42809.
+#
+# Fixture: a tiny 3-column DataFrame that keeps row/col/grand totals easy
+# to eyeball. Two rows (``r1``, ``r2``), two columns (``c1``, ``c2``),
+# single metric ``v``. Grand total is 100 so every percent-of-total
+# assertion is trivially checkable.
+
+
+def _show_values_as_fixture() -> DataFrame:
+ """Long-format input that pivots to::
+
+ v
+ col c1 c2
+ row
+ r1 10 20
+ r2 30 40
+
+ row totals: r1=30, r2=70; col totals: c1=40, c2=60; grand=100.
+ """
+ return DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "v": [10, 20, 30, 40],
+ }
+ )
+
+
+def test_pivot_show_values_as_actual_is_noop() -> None:
+ """``show_values_as='actual'`` (and ``None``) leaves values unchanged."""
+ df = _show_values_as_fixture()
+ aggregates = {"v": {"operator": "sum"}}
+ baseline = pivot(df=df, index=["row"], columns=["col"], aggregates=aggregates)
+
+ for mode in (None, "actual"):
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates=aggregates,
+ show_values_as=mode,
+ )
+ pd.testing.assert_frame_equal(result, baseline)
+
+
+def test_pivot_show_values_as_percent_row() -> None:
+ """Each cell = cell / row-total; each row sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_row",
+ )
+ # r1: 10/30, 20/30; r2: 30/70, 40/70
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(10 / 30)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(20 / 30)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(30 / 70)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(40 / 70)
+ assert result.sum(axis=1).tolist() == pytest.approx([1.0, 1.0])
+
+
+def test_pivot_show_values_as_percent_col() -> None:
+ """Each cell = cell / column-total; each column sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_col",
+ )
+ # c1 total=40: 10/40, 30/40; c2 total=60: 20/60, 40/60
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(10 / 40)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(30 / 40)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(20 / 60)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(40 / 60)
+ assert result.sum(axis=0).tolist() == pytest.approx([1.0, 1.0])
+
+
+def test_pivot_show_values_as_percent_total() -> None:
+ """Each cell = cell / grand-total; the whole frame sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_total",
+ )
+ # grand=100: each cell divided by 100
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(0.10)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(0.20)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(0.30)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(0.40)
+ assert result.values.sum() == pytest.approx(1.0)
+
+
+def test_pivot_show_values_as_preserves_nan_numerator() -> None:
+ """A NaN/NULL numerator stays NaN — matches the client-side #42810 guard
+ that a genuine SQL NULL should render blank, not "0.0%"."""
+ df = DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "v": [10, np.nan, 30, 40],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_row",
+ )
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ The new regression test fails under the configured sum aggregator.
- ⚠️ NaN-preservation behavior remains unverified.
- ⚠️ CI cannot validate the intended NULL rendering contract.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=12c5f0bca1c241cabea33c65cc12b50f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=12c5f0bca1c241cabea33c65cc12b50f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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 fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42976&comment_hash=ec3260395c9094ad18144cd4408231bc48d24ad9901ad881e8b028e5b453dd6d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42976&comment_hash=ec3260395c9094ad18144cd4408231bc48d24ad9901ad881e8b028e5b453dd6d&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #42976:
URL: https://github.com/apache/superset/pull/42976#discussion_r3820179079
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -168,4 +297,7 @@ def pivot( # pylint: disable=too-many-arguments
# be silently dropped by stack's default dropna=True behavior.
df = df.stack(level=0, dropna=False).unstack()
+ if percent_mode is not None:
Review Comment:
When `combine_value_with_metric=True`, line 298 reshapes columns from `(metric, category)` to `(category, metric)` before this transform runs. `_apply_show_values_as` then treats each category as a metric and normalizes across different metrics, so percent-of-row/total values are wrong for combined pivot layouts. Could this transform run before the reshape, or select the actual metric level after it?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "Abdulrehman-PIAIC80387 (via GitHub)" <gi...@apache.org>.
Abdulrehman-PIAIC80387 commented on code in PR #42976:
URL: https://github.com/apache/superset/pull/42976#discussion_r3756162925
##########
tests/unit_tests/pandas_postprocessing/test_pivot.py:
##########
@@ -446,3 +446,191 @@ def test_pivot_only_entirely_absent_metrics_are_restored():
assert ("metric_partial", "A") in df.columns
assert ("metric_partial", "B") not in df.columns
assert df[("metric_partial", "A")].iloc[0] == 1.0
+
+
+# --- show_values_as regression tests (#42809) --------------------------------
+#
+# ``show_values_as`` expresses each metric cell as a fraction of the row,
+# column, or grand total after pivoting. Mirrors the 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)
+# match the browser output. See #42809.
+#
+# Fixture: a tiny 3-column DataFrame that keeps row/col/grand totals easy
+# to eyeball. Two rows (``r1``, ``r2``), two columns (``c1``, ``c2``),
+# single metric ``v``. Grand total is 100 so every percent-of-total
+# assertion is trivially checkable.
+
+
+def _show_values_as_fixture() -> DataFrame:
+ """Long-format input that pivots to::
+
+ v
+ col c1 c2
+ row
+ r1 10 20
+ r2 30 40
+
+ row totals: r1=30, r2=70; col totals: c1=40, c2=60; grand=100.
+ """
+ return DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "v": [10, 20, 30, 40],
+ }
+ )
+
+
+def test_pivot_show_values_as_actual_is_noop() -> None:
+ """``show_values_as='actual'`` (and ``None``) leaves values unchanged."""
+ df = _show_values_as_fixture()
+ aggregates = {"v": {"operator": "sum"}}
+ baseline = pivot(df=df, index=["row"], columns=["col"], aggregates=aggregates)
+
+ for mode in (None, "actual"):
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates=aggregates,
+ show_values_as=mode,
+ )
+ pd.testing.assert_frame_equal(result, baseline)
+
+
+def test_pivot_show_values_as_percent_row() -> None:
+ """Each cell = cell / row-total; each row sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_row",
+ )
+ # r1: 10/30, 20/30; r2: 30/70, 40/70
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(10 / 30)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(20 / 30)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(30 / 70)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(40 / 70)
+ assert result.sum(axis=1).tolist() == pytest.approx([1.0, 1.0])
+
+
+def test_pivot_show_values_as_percent_col() -> None:
+ """Each cell = cell / column-total; each column sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_col",
+ )
+ # c1 total=40: 10/40, 30/40; c2 total=60: 20/60, 40/60
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(10 / 40)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(30 / 40)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(20 / 60)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(40 / 60)
+ assert result.sum(axis=0).tolist() == pytest.approx([1.0, 1.0])
+
+
+def test_pivot_show_values_as_percent_total() -> None:
+ """Each cell = cell / grand-total; the whole frame sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_total",
+ )
+ # grand=100: each cell divided by 100
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(0.10)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(0.20)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(0.30)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(0.40)
+ assert result.values.sum() == pytest.approx(1.0)
+
+
+def test_pivot_show_values_as_preserves_nan_numerator() -> None:
+ """A NaN/NULL numerator stays NaN — matches the client-side #42810 guard
+ that a genuine SQL NULL should render blank, not "0.0%"."""
+ df = DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "v": [10, np.nan, 30, 40],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_row",
+ )
+ # The NaN numerator stays NaN — it did not become 0.0
+ assert pd.isna(result.loc["r1", ("v", "c2")])
+ # The other cells still divide correctly (r1 row total is now just 10)
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(1.0)
+
+
+def test_pivot_show_values_as_percent_total_zero_grand_total_yields_nan() -> None:
+ """Grand total of zero yields NaN cells rather than Infinity — matches the
+ client's ``if (acc === null) return null`` division-by-zero guard."""
+ df = DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "v": [0, 0, 0, 0],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_total",
+ )
+ # No cell should be Infinity or a real number; all should be NaN.
+ assert result.isna().values.all()
+
+
+def test_pivot_show_values_as_percent_row_multi_metric_keeps_metrics_separate() -> None:
+ """On a multi-metric pivot (``MultiIndex`` columns), per-row totals are
+ computed *within each metric*. Metric A's percentages must sum to 1.0
+ per row independent of metric B's values."""
+ df = DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "a": [10, 20, 30, 40],
+ "b": [1, 3, 5, 7],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates={"a": {"operator": "sum"}, "b": {"operator": "sum"}},
+ show_values_as="percent_row",
+ )
+ # Metric ``a``: row totals 30 and 70; each row of ``a`` sums to 1.
+ assert result["a"].sum(axis=1).tolist() == pytest.approx([1.0, 1.0])
+ # Metric ``b``: row totals 4 and 12; each row of ``b`` sums to 1.
+ assert result["b"].sum(axis=1).tolist() == pytest.approx([1.0, 1.0])
+ # Metric ``a`` percentages must not be contaminated by metric ``b`` values.
+ assert result.loc["r1", ("a", "c1")] == pytest.approx(10 / 30)
+ assert result.loc["r1", ("b", "c1")] == pytest.approx(1 / 4)
+
+
+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(
Review Comment:
Added in `80585a9678` — two new tests, one per axis:
- `test_pivot_show_values_as_percent_row_zero_row_total_yields_nan` — a row whose values sum to zero yields NaN cells in that row (rather than `Infinity`), and other rows still divide correctly.
- `test_pivot_show_values_as_percent_col_zero_col_total_yields_nan` — same guarantee for the columns axis.
The `.replace(0, float("nan"))` on the denominator series in `_apply_percent_transform_to_group` is what turns the zero divisor into NaN before the div, so `Series.div` produces NaN instead of Infinity.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42976:
URL: https://github.com/apache/superset/pull/42976#issuecomment-5252055294
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "Abdulrehman-PIAIC80387 (via GitHub)" <gi...@apache.org>.
Abdulrehman-PIAIC80387 commented on code in PR #42976:
URL: https://github.com/apache/superset/pull/42976#discussion_r3756122298
##########
tests/unit_tests/pandas_postprocessing/test_pivot.py:
##########
@@ -446,3 +446,191 @@ def test_pivot_only_entirely_absent_metrics_are_restored():
assert ("metric_partial", "A") in df.columns
assert ("metric_partial", "B") not in df.columns
assert df[("metric_partial", "A")].iloc[0] == 1.0
+
+
+# --- show_values_as regression tests (#42809) --------------------------------
+#
+# ``show_values_as`` expresses each metric cell as a fraction of the row,
+# column, or grand total after pivoting. Mirrors the 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)
+# match the browser output. See #42809.
+#
+# Fixture: a tiny 3-column DataFrame that keeps row/col/grand totals easy
+# to eyeball. Two rows (``r1``, ``r2``), two columns (``c1``, ``c2``),
+# single metric ``v``. Grand total is 100 so every percent-of-total
+# assertion is trivially checkable.
+
+
+def _show_values_as_fixture() -> DataFrame:
+ """Long-format input that pivots to::
+
+ v
+ col c1 c2
+ row
+ r1 10 20
+ r2 30 40
+
+ row totals: r1=30, r2=70; col totals: c1=40, c2=60; grand=100.
+ """
+ return DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "v": [10, 20, 30, 40],
+ }
+ )
+
+
+def test_pivot_show_values_as_actual_is_noop() -> None:
+ """``show_values_as='actual'`` (and ``None``) leaves values unchanged."""
+ df = _show_values_as_fixture()
+ aggregates = {"v": {"operator": "sum"}}
+ baseline = pivot(df=df, index=["row"], columns=["col"], aggregates=aggregates)
+
+ for mode in (None, "actual"):
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates=aggregates,
+ show_values_as=mode,
+ )
+ pd.testing.assert_frame_equal(result, baseline)
+
+
+def test_pivot_show_values_as_percent_row() -> None:
+ """Each cell = cell / row-total; each row sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_row",
+ )
+ # r1: 10/30, 20/30; r2: 30/70, 40/70
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(10 / 30)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(20 / 30)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(30 / 70)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(40 / 70)
+ assert result.sum(axis=1).tolist() == pytest.approx([1.0, 1.0])
+
+
+def test_pivot_show_values_as_percent_col() -> None:
+ """Each cell = cell / column-total; each column sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_col",
+ )
+ # c1 total=40: 10/40, 30/40; c2 total=60: 20/60, 40/60
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(10 / 40)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(30 / 40)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(20 / 60)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(40 / 60)
+ assert result.sum(axis=0).tolist() == pytest.approx([1.0, 1.0])
+
+
+def test_pivot_show_values_as_percent_total() -> None:
+ """Each cell = cell / grand-total; the whole frame sums to 1.0."""
+ result = pivot(
+ df=_show_values_as_fixture(),
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_total",
+ )
+ # grand=100: each cell divided by 100
+ assert result.loc["r1", ("v", "c1")] == pytest.approx(0.10)
+ assert result.loc["r1", ("v", "c2")] == pytest.approx(0.20)
+ assert result.loc["r2", ("v", "c1")] == pytest.approx(0.30)
+ assert result.loc["r2", ("v", "c2")] == pytest.approx(0.40)
+ assert result.values.sum() == pytest.approx(1.0)
+
+
+def test_pivot_show_values_as_preserves_nan_numerator() -> None:
+ """A NaN/NULL numerator stays NaN — matches the client-side #42810 guard
+ that a genuine SQL NULL should render blank, not "0.0%"."""
+ df = DataFrame(
+ {
+ "row": ["r1", "r1", "r2", "r2"],
+ "col": ["c1", "c2", "c1", "c2"],
+ "v": [10, np.nan, 30, 40],
+ }
+ )
+ result = pivot(
+ df=df,
+ index=["row"],
+ columns=["col"],
+ aggregates={"v": {"operator": "sum"}},
+ show_values_as="percent_row",
+ )
Review 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).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42976:
URL: https://github.com/apache/superset/pull/42976#issuecomment-5237299341
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42976:
URL: https://github.com/apache/superset/pull/42976#issuecomment-5450848529
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "Abdulrehman-PIAIC80387 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42976:
URL: https://github.com/apache/superset/pull/42976#issuecomment-5237343180
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42976?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `15.15152%` with `28 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.09%. Comparing base ([`3539c41`](https://app.codecov.io/gh/apache/superset/commit/3539c41dab2ce28f37fcaf1d7393a0546fd13dc4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ce172a1`](https://app.codecov.io/gh/apache/superset/commit/ce172a190a2869eba0949abcaeeb68366aab144d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/42976?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/utils/pandas\_postprocessing/pivot.py](https://app.codecov.io/gh/apache/superset/pull/42976?src=pr&el=tree&filepath=superset%2Futils%2Fpandas_postprocessing%2Fpivot.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvcGFuZGFzX3Bvc3Rwcm9jZXNzaW5nL3Bpdm90LnB5) | 15.15% | [28 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42976?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (3539c41) and HEAD (ce172a1). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (3539c41) | HEAD (ce172a1) |
>|------|------|------|
>|python|32|1|
>|presto|6|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #42976 +/- ##
==========================================
- Coverage 66.41% 57.09% -9.32%
==========================================
Files 2858 2858
Lines 161446 161478 +32
Branches 37190 37200 +10
==========================================
- Hits 107222 92194 -15028
- Misses 52187 68428 +16241
+ Partials 2037 856 -1181
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42976/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/42976/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/42976/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/42976/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/42976/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.16% <15.15%> (-0.02%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/42976/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.16% <15.15%> (-19.05%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/42976/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/42976/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42976?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #42976:
URL: https://github.com/apache/superset/pull/42976#discussion_r3873217081
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -26,6 +27,92 @@
validate_column_args,
)
+_PERCENT_MODES = frozenset({"percent_row", "percent_col", "percent_total"})
+
+
+def _div_preserving_nan(numerator: DataFrame, denominator: Any, axis: int) -> DataFrame:
+ """Divide ``numerator`` by ``denominator``, preserving NaN numerators.
+
+ A genuine SQL NULL numerator must stay NaN (rendered blank) rather than
+ become ``0.0`` — matches the client-side #42810 semantics guarding
+ against measured "0.0%" values for values that should stay blank.
+ """
+ result = numerator.div(denominator, axis=axis)
+ return result.mask(numerator.isna(), other=float("nan"))
+
+
+def _apply_percent_transform_to_group(g: DataFrame, mode: str) -> DataFrame:
+ """Apply a percent-of-{row,col,total} transform to a single-metric block.
+
+ Called both for the whole DataFrame when it holds one metric, and
+ per-metric-group for MultiIndex / flat-multi-metric pivots. A zero or
+ NaN denominator produces NaN cells rather than ``Infinity``/``NaN``
+ from division-by-zero, matching the client's ``if (acc === null)
+ return null`` guard in ``fractionOf``.
+ """
+ if mode == "percent_row":
+ row_totals = g.sum(axis=PandasAxis.COLUMN, skipna=True).replace(0, float("nan"))
Review Comment:
Summing the already-aggregated cells does not produce the row rollup for non-additive metrics. For example, category means `[5, 10]` from groups `[0, 10]` and `[10]` are normalized as 1/3 and 2/3 here, while the chart's DB rollup uses mean `20/3`; exports will therefore disagree for AVG and similar metrics. Can this use the requested rollup values or reject unsupported aggregations?
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -26,6 +27,92 @@
validate_column_args,
)
+_PERCENT_MODES = frozenset({"percent_row", "percent_col", "percent_total"})
+
+
+def _div_preserving_nan(numerator: DataFrame, denominator: Any, axis: int) -> DataFrame:
+ """Divide ``numerator`` by ``denominator``, preserving NaN numerators.
+
+ A genuine SQL NULL numerator must stay NaN (rendered blank) rather than
+ become ``0.0`` — matches the client-side #42810 semantics guarding
+ against measured "0.0%" values for values that should stay blank.
+ """
+ result = numerator.div(denominator, axis=axis)
+ return result.mask(numerator.isna(), other=float("nan"))
Review Comment:
A present SQL NULL aggregated with `sum` has already become `0.0` by the time this mask runs, so this returns a measured 0% instead of the blank the new contract promises. Could the transform retain nullness before aggregation or avoid claiming NULL preservation for this supported path?
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -26,6 +27,92 @@
validate_column_args,
)
+_PERCENT_MODES = frozenset({"percent_row", "percent_col", "percent_total"})
+
+
+def _div_preserving_nan(numerator: DataFrame, denominator: Any, axis: int) -> DataFrame:
+ """Divide ``numerator`` by ``denominator``, preserving NaN numerators.
+
+ A genuine SQL NULL numerator must stay NaN (rendered blank) rather than
+ become ``0.0`` — matches the client-side #42810 semantics guarding
+ against measured "0.0%" values for values that should stay blank.
+ """
+ result = numerator.div(denominator, axis=axis)
+ return result.mask(numerator.isna(), other=float("nan"))
+
+
+def _apply_percent_transform_to_group(g: DataFrame, mode: str) -> DataFrame:
+ """Apply a percent-of-{row,col,total} transform to a single-metric block.
+
+ Called both for the whole DataFrame when it holds one metric, and
+ per-metric-group for MultiIndex / flat-multi-metric pivots. A zero or
+ NaN denominator produces NaN cells rather than ``Infinity``/``NaN``
+ from division-by-zero, matching the client's ``if (acc === null)
+ return null`` guard in ``fractionOf``.
+ """
+ if mode == "percent_row":
+ row_totals = g.sum(axis=PandasAxis.COLUMN, skipna=True).replace(0, float("nan"))
+ return _div_preserving_nan(g, row_totals, axis=PandasAxis.ROW)
+ if mode == "percent_col":
+ col_totals = g.sum(axis=PandasAxis.ROW, skipna=True).replace(0, float("nan"))
+ return _div_preserving_nan(g, col_totals, axis=PandasAxis.COLUMN)
+ # percent_total
+ grand = g.sum(skipna=True).sum(skipna=True)
+ if pd.isna(grand) or grand == 0:
+ return g * float("nan")
+ return _div_preserving_nan(g, grand, axis=PandasAxis.ROW)
+
+
+def _apply_show_values_as(df: DataFrame, mode: str) -> DataFrame:
+ """Divide each metric cell by the appropriate rollup total.
+
+ Mirrors the client-side ``fractionOf`` semantic in
+ ``plugin-chart-pivot-table/src/react-pivottable/utilities.ts:739``:
+
+ - ``percent_row``: cell / row-total (sum across the columns axis)
+ - ``percent_col``: cell / column-total (sum across the rows axis)
+ - ``percent_total``: cell / grand-total (sum of the metric block)
+
+ Per-metric isolation — the totals are computed *within each metric* so
+ one metric's numerator is never divided by another metric's total,
+ matching the client's ``metricAxis`` handling. This applies to both
+ shapes ``pivot_table`` can produce:
+
+ - **MultiIndex columns** (level 0 = metric): iterate the level-0
+ groups explicitly. Explicit iteration avoids the deprecated
+ ``df.groupby(level=0, axis=1)`` pattern (removed in pandas 3.x).
+ - **Flat columns with >1 column** (multi-metric pivot with no
+ ``columns`` groupby — each column IS a metric): treat each column
+ as its own single-column metric block.
+ - **Flat columns with 1 column** (single-metric pivot with no
+ ``columns`` groupby): the whole block is one metric.
+ """
+ is_multi_metric_wide = isinstance(df.columns, pd.MultiIndex)
+ is_flat_multi_metric = not is_multi_metric_wide and df.shape[1] > 1
+
+ if is_multi_metric_wide:
+ # Iterate level-0 groups explicitly (pandas-3-safe).
+ metrics = df.columns.get_level_values(0).unique()
+ parts = []
+ for metric in metrics:
+ block = df.xs(metric, axis=PandasAxis.COLUMN, level=0, drop_level=False)
+ parts.append(_apply_percent_transform_to_group(block, mode))
+ # ``concat`` along columns preserves the MultiIndex; reorder to
+ # match the original column layout deterministically.
+ combined = pd.concat(parts, axis=PandasAxis.COLUMN)
Review Comment:
An empty result with a column grouping has an empty `MultiIndex`, leaving `parts` empty and making `pd.concat` raise `ValueError: No objects to concatenate`; the same pivot without this option returns an empty frame. Could this return the empty frame unchanged (with a zero-row regression 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "Abdulrehman-PIAIC80387 (via GitHub)" <gi...@apache.org>.
Abdulrehman-PIAIC80387 commented on code in PR #42976:
URL: https://github.com/apache/superset/pull/42976#discussion_r3820266611
##########
superset/utils/pandas_postprocessing/pivot.py:
##########
@@ -168,4 +297,7 @@ def pivot( # pylint: disable=too-many-arguments
# be silently dropped by stack's default dropna=True behavior.
df = df.stack(level=0, dropna=False).unstack()
+ if percent_mode is not None:
Review Comment:
Confirmed empirically — you're right. Fixed in 5290ebe.
**Reproduction** (2 metrics × 2 categories × 2 rows, `show_values_as="percent_row"`, `combine_value_with_metric=True`):
Before the reshape, columns are `(metric, category)` — `[('a', 'c1'), ('a', 'c2'), ('b', 'c1'), ('b', 'c2')]`.
After `stack(level=0).unstack()`, they become `(category, metric)` — `[('c1', 'a'), ('c1', 'b'), ('c2', 'a'), ('c2', 'b')]`.
`_apply_show_values_as` iterates `df.columns.get_level_values(0).unique()` and treats each as a metric. When the reshape runs first, that iteration walks categories, so `a` gets divided by `a+b` sums:
| order | metric a row sum (should be 1.0) | metric b row sum |
|---|---|---|
| reshape → percent (current) | **1.78** ❌ | **0.22** ❌ |
| percent → reshape (fix) | 1.00 ✓ | 1.00 ✓ |
**Fix:** swap the two blocks — apply the percent transform on the natural `(metric, category)` shape first, then let the reshape run on already-normalized values. Regression test added at `test_pivot_show_values_as_with_combine_value_with_metric_preserves_per_metric` which fails on the pre-fix code and passes on the fix.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(postprocessing): teach pivot() to compute percent-of-row/col/total (#42809) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #42976:
URL: https://github.com/apache/superset/pull/42976
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org