amaannawab923 commented on code in PR #43718:
URL: https://github.com/apache/superset/pull/43718#discussion_r3896174013
##########
superset/charts/client_processing.py:
##########
@@ -87,8 +167,23 @@ def pivot_df( # pylint: disable=too-many-locals,
too-many-arguments, too-many-s
show_columns_total: bool = False,
apply_metrics_on_rows: bool = False,
metric_name_aggfunc: Optional[str] = None,
+ show_values_as: Optional[str] = None,
) -> pd.DataFrame:
+ percent_mode = (
+ show_values_as if show_values_as in SHOW_VALUES_AS_PERCENT_MODES else
None
+ )
+ if percent_mode:
+ # The chart ignores `aggregateFunction` post-SIP-216: cells arrive
+ # pre-aggregated from the database and totals are plain rollups of
them.
+ # Match that here so the totals and the percent denominators cannot
+ # disagree -- otherwise a total stops dividing by itself and the Total
+ # row/column reads something other than 100%.
+ aggfunc = "Sum"
Review Comment:
this fixes the totals-divide-by-themselves problem, but it looks like it
pulls against the min/max reducer thread above.
`additiveReducerFor` in `plugin/utilities.ts:63` maps SIMPLE MIN/MAX metrics
to min/max rollups and only falls back to sum for everything else. so pinning
`aggfunc = "Sum"` here keeps totals self consistent, but puts a MAX metric back
out of step with the chart: a row of [6, 10] divides by 16 and reads 37.5 /
62.5, where the chart rolls up to 10 and shows 60 / 100.
carrying the per metric reducer into `_apply_show_values_as` rather than
forcing Sum would satisfy both, since a total still divides by itself when the
reducer is min or max. probably worth settling that direction before the next
pass, the two threads want opposite things as they stand.
##########
tests/unit_tests/charts/test_client_processing.py:
##########
@@ -1804,6 +1805,235 @@ def test_pivot_df_complex_null_values():
)
+# --- `showValuesAs` percent modes (#42809) -----------------------------------
+#
+# Exports and scheduled reports render server-side, so they have to reproduce
+# the client's `fractionOf` aggregator: each cell over its row, column, or
grand
+# total, computed per metric, with totals dividing by their own rollup rather
+# than summing the fractions around them.
+
+SHOW_VALUES_AS_OPTIONS: dict[str, Any] = {
+ "rows": ["nation"],
+ "columns": ["gender"],
+ "metrics": ["SUM(num)"],
+ "aggfunc": "Sum",
+ "transpose_pivot": False,
+ "combine_metrics": False,
Review Comment:
`combine_metrics` is pinned False for all the show_values_as cases, so the
`metric_level = df.columns.nlevels - 1` branch in `_apply_show_values_as` never
runs. the `combine_metrics=True` cases higher up the file belong to other tests.
the description calls out combineMetric handling and metrics_on_rows has its
own case, so probably worth one with the flag flipped. the two metric setup in
`keeps_metrics_separate` would cover it as is.
--
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]
