EnxDev commented on code in PR #43718:
URL: https://github.com/apache/superset/pull/43718#discussion_r3895693463
##########
superset/charts/client_processing.py:
##########
@@ -541,10 +654,22 @@ def pivot_table_v2(
"show_rows_total": bool(form_data.get("rowTotals")),
"show_columns_total": bool(form_data.get("colTotals")),
"apply_metrics_on_rows": form_data.get("metricsLayout") == "ROWS",
+ "show_values_as": percent_mode,
}
pivoted = pivot_df(df, **pivot_options)
if apply_number_format:
+ if percent_mode:
+ # A ratio has no currency and ignores per-metric value formats, the
+ # same way the client skips `formattedAggregators` while a fraction
+ # is active.
+ return apply_pivot_number_formats(
+ pivoted,
+ form_data,
+ detected_currency,
+ datasource,
+ force_number_format=PERCENT_3_POINT,
Review Comment:
The pivot renderer uses `usFmtPct` with one decimal, so the chart shows
`12.5%`, while this makes text reports show `12.500%`. Could we use the
one-decimal equivalent here so report tables match the chart?
##########
superset/charts/client_processing.py:
##########
@@ -541,10 +654,22 @@ def pivot_table_v2(
"show_rows_total": bool(form_data.get("rowTotals")),
"show_columns_total": bool(form_data.get("colTotals")),
"apply_metrics_on_rows": form_data.get("metricsLayout") == "ROWS",
+ "show_values_as": percent_mode,
Review Comment:
Could we split GROUPING SETS results before passing them to `pivot_df`? For
saved, adhoc, and other non-additive metrics, `buildQuery` includes denominator
rollups in this same frame. Those rows are treated as leaves here, so values
`10`, `20`, plus grand total `30` produce a denominator of `60` and an extra
blank pivot row. A test with `__superset_grouping` markers would catch this.
##########
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:
This still assumes every metric rolls up by sum. SIP-216 uses min/max
reducers for SIMPLE `MIN`/`MAX` metrics, so a MAX row `[6, 10]` should divide
by `10` (`60%`, `100%`); this divides by `16` (`37.5%`, `62.5%`). Could we
carry the metric's rollup reducer into this transform?
--
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]
