geido commented on code in PR #43718:
URL: https://github.com/apache/superset/pull/43718#discussion_r3895547029
##########
superset/charts/client_processing.py:
##########
@@ -75,6 +76,80 @@ def get_column_key(label: tuple[str, ...], metrics:
list[str]) -> tuple[Any, ...
return tuple(parts)
+def _apply_show_values_as( # pylint: disable=too-many-arguments
+ df: pd.DataFrame,
+ mode: str,
+ axis: dict[str, int],
+ metrics: list[str],
+ combine_metrics: bool,
+ inserted_rows: list[Any],
+ inserted_columns: list[Any],
+) -> pd.DataFrame:
+ """
+ Express each cell as a fraction of its row, column, or grand total.
+
+ Mirrors the client's ``fractionOf`` aggregator in
+ ``plugin-chart-pivot-table/src/react-pivottable/utilities.ts``. Two details
+ it inherits from there:
+
+ - Denominators are summed over leaf cells only. Totals and subtotals
+ inserted into the frame are numerators like any other cell -- a "% of
+ row" grand total row reads ``column total / grand total``, not the sum of
+ the fractions above it.
+ - A total is summed within a single metric, so a cell is never divided by a
+ total that mixes in another metric. Cross-metric totals (whose metric
+ level holds a total label rather than a metric name) divide by the
+ denominator spanning every metric.
+
+ A zero denominator yields NaN (blank) rather than infinity, matching
+ ``pandas_postprocessing.pivot``'s ``show_values_as``.
+ """
+ numeric = df.apply(pd.to_numeric, errors="coerce").astype(float)
+ is_multi_index = isinstance(df.columns, pd.MultiIndex)
+ # `combine_metrics` has already moved the metric to the lowest column
level.
+ metric_level = df.columns.nlevels - 1 if combine_metrics and
is_multi_index else 0
+ metric_names = set(metrics)
+ metric_of_column = [
+ key if key in metric_names else None
+ for key in df.columns.get_level_values(metric_level)
+ ]
+ leaf_rows = ~df.index.isin(inserted_rows)
+ leaf_columns = ~df.columns.isin(inserted_columns)
+
+ result = numeric.copy()
+ for metric in dict.fromkeys(metric_of_column):
+ selection = np.array([column == metric for column in metric_of_column])
+ denominator_selection = (
+ selection if metric is not None else np.ones(len(selection),
dtype=bool)
+ )
+ block = numeric.loc[:, selection]
+ if mode == ShowValuesAs.PERCENT_OF_TOTAL:
+ leaf = numeric.loc[leaf_rows, leaf_columns & denominator_selection]
+ # Sum through pandas, not numpy: a sparse pivot leaves NaN in cells
+ # whose group had no rows, and numpy would propagate that to the
+ # grand total, blanking every cell.
+ grand_total = leaf.sum().sum()
+ fraction = block / (
+ np.nan if pd.isna(grand_total) or grand_total == 0 else
grand_total
+ )
+ else:
+ summed, divided = (
+ (axis["rows"], axis["columns"])
+ if mode == ShowValuesAs.PERCENT_OF_COLUMN
+ else (axis["columns"], axis["rows"])
+ )
+ # The metric lives on the column axis, so only a sum taken along
+ # that axis has to stay within one metric.
+ leaf = (
+ numeric.loc[:, leaf_columns & denominator_selection]
+ if summed == 1
+ else numeric.loc[leaf_rows, :]
+ )
+ fraction = block.div(leaf.sum(axis=summed).replace(0, np.nan),
axis=divided)
Review Comment:
**Non-additive `aggfunc` leaves the exported table internally inconsistent.**
The denominators here are always `.sum()`, but the totals inserted above are
computed with `pivot_v2_aggfunc_map[aggfunc]`, where `aggfunc` comes from
`form_data.get("aggregateFunction", "Sum")`. When those two disagree, a total
no longer divides by itself and the Total row/column stops reading 100%.
`aggfunc="Average"` on this PR's own 2x2 fixture:
```
SUM(num) Total (Average)
gender boy girl Subtotal
UK 0.500 0.500 0.5 0.5
US 0.250 0.750 0.5 0.5
Total (Average) 0.375 0.625 0.5 0.5
```
The Total column reads 50%: each row divides by `sum(10, 30) = 40` while the
displayed total is `mean(10, 30) = 20`. The chart renders 100% here, since
`fractionOf` divides by the rollup aggregator's own value, so a total always
divides by itself regardless of the aggregate.
This looks reachable rather than theoretical. #42761's migration docstring
notes SIP-216 deliberately left `aggregateFunction` on saved charts ("Saved
charts that set `aggregateFunction` will ignore it"), and only the three `Sum
as Fraction of ...` values get migrated. A pre-SIP-216 chart still carrying
`Average`, `Median`, `Minimum`, `Maximum`, or `Count Unique Values`, on which
someone then picks a "Show values as" percent mode, lands exactly here.
Worth noting that `pandas_postprocessing/pivot.py` already guards this case
via `_ADDITIVE_OPERATORS` and raises `InvalidPostProcessingError` -- this
implementation has no equivalent, which is the kind of drift the shared
`ShowValuesAs` enum in the second commit is meant to prevent. Either mirror
that guard, or compute the denominator with the same
`pivot_v2_aggfunc_map[aggfunc]` the totals use so the two can't diverge. Given
that the client ignores `aggregateFunction` entirely post-SIP-216, forcing
sum-consistency here would also be defensible.
_Verified by running `pivot_df` directly against the fixture from this PR's
tests._
--
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]
