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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
