sadpandajoe commented on code in PR #44679:
URL: https://github.com/apache/superset/pull/44679#discussion_r4286367781
##########
superset/charts/client_processing.py:
##########
@@ -479,6 +479,133 @@ def _apply_show_values_as( # pylint:
disable=too-many-arguments
return numeric / denominator.replace(0, np.nan)
+def _reduce_result_values(values: pd.Series, aggregate: str) -> Any:
+ """Reduce original metric results without aggregating intermediate
summaries."""
+ if values.empty:
+ return None
+ name = aggregate.split(" as Fraction of ")[0]
+ if name == "Count":
+ return len(values)
+ if name == "Count Unique Values":
+ return values.nunique(dropna=False)
+ if name == "List Unique Values":
+ return ", ".join(sorted({str(value) for value in values}))
+ if name in {"First", "Last"}:
+ present = values.dropna()
+ return present.iloc[0 if name == "First" else -1] if len(present) else
None
+ numeric = pd.to_numeric(values, errors="coerce").dropna()
+ if name == "Sum":
+ text = values.dropna()
+ return text.iloc[-1] if numeric.empty and len(text) else numeric.sum()
+ if numeric.empty:
+ return None
+ if name == "Sample Variance":
+ return numeric.var(ddof=1) if len(numeric) > 1 else 0
+ if name == "Sample Standard Deviation":
+ return numeric.std(ddof=1) if len(numeric) > 1 else 0
+ return {
+ "Average": numeric.mean,
+ "Median": numeric.median,
+ "Minimum": numeric.min,
+ "Maximum": numeric.max,
+ }[name]()
+
+
+def _group_result_records(
+ source: pd.DataFrame, dimensions: list[str]
+) -> dict[tuple[str, ...], pd.DataFrame]:
+ """Index contributing records by the dimension prefix of a summary
scope."""
+ if not dimensions:
+ return {(): source}
+ keys = [source[dim].fillna("SUPERSET_PANDAS_NAN").astype(str) for dim in
dimensions]
Review Comment:
The group keys come from `Series.astype(str)` but the lookup keys in
`_rollup_key` use scalar `str()`. For a midnight datetime dimension these
differ (`"2026-01-01"` vs `"2026-01-01 00:00:00"`), so no records match. With
dates `[2026-01-01, 2026-01-02]` and sales `[10, 20]` in an Excel export with
`Sum`, both leaf cells come out blank while the grand total still shows 30.
Could both sides build keys the same way?
##########
superset/charts/client_processing.py:
##########
@@ -479,6 +479,133 @@ def _apply_show_values_as( # pylint:
disable=too-many-arguments
return numeric / denominator.replace(0, np.nan)
+def _reduce_result_values(values: pd.Series, aggregate: str) -> Any:
+ """Reduce original metric results without aggregating intermediate
summaries."""
+ if values.empty:
+ return None
+ name = aggregate.split(" as Fraction of ")[0]
+ if name == "Count":
+ return len(values)
+ if name == "Count Unique Values":
+ return values.nunique(dropna=False)
+ if name == "List Unique Values":
+ return ", ".join(sorted({str(value) for value in values}))
+ if name in {"First", "Last"}:
+ present = values.dropna()
+ return present.iloc[0 if name == "First" else -1] if len(present) else
None
+ numeric = pd.to_numeric(values, errors="coerce").dropna()
+ if name == "Sum":
+ text = values.dropna()
+ return text.iloc[-1] if numeric.empty and len(text) else numeric.sum()
+ if numeric.empty:
+ return None
+ if name == "Sample Variance":
+ return numeric.var(ddof=1) if len(numeric) > 1 else 0
+ if name == "Sample Standard Deviation":
+ return numeric.std(ddof=1) if len(numeric) > 1 else 0
+ return {
+ "Average": numeric.mean,
+ "Median": numeric.median,
+ "Minimum": numeric.min,
+ "Maximum": numeric.max,
+ }[name]()
+
+
+def _group_result_records(
+ source: pd.DataFrame, dimensions: list[str]
+) -> dict[tuple[str, ...], pd.DataFrame]:
+ """Index contributing records by the dimension prefix of a summary
scope."""
+ if not dimensions:
+ return {(): source}
+ keys = [source[dim].fillna("SUPERSET_PANDAS_NAN").astype(str) for dim in
dimensions]
+ return {
+ key if isinstance(key, tuple) else (key,): group
+ for key, group in source.groupby(keys, sort=False, dropna=False)
+ }
+
+
+def _apply_result_aggregation(
+ frame: pd.DataFrame,
+ source: pd.DataFrame,
+ rows: list[str],
+ columns: list[str],
+ metrics: list[str],
+ metric_level: int,
+ row_depths: dict[Any, int],
+ column_depths: dict[Any, int],
+ aggregate: str,
+ show_values_as: Optional[str],
+ metrics_on_rows: bool,
+) -> pd.DataFrame:
+ """Calculate every scope from its original contributing query results."""
+
+ if not set(metrics).issubset(source.columns):
+ return frame
+
+ grouped_sources: dict[tuple[int, int], dict[tuple[str, ...],
pd.DataFrame]] = {}
+
+ def contributing(
+ row: Any,
+ column: Any,
+ row_depth: int,
+ column_depth: int,
+ selected_metrics: list[str],
+ ) -> pd.Series:
+ level = (row_depth, column_depth)
+ if level not in grouped_sources:
+ grouped_sources[level] = _group_result_records(
+ source, rows[:row_depth] + columns[:column_depth]
+ )
+ key = _rollup_key(row, row_depth, metric_level, is_column=False) +
_rollup_key(
+ column,
+ column_depth,
+ metric_level,
+ is_column=True,
+ )
+ selected = grouped_sources[level].get(key, source.iloc[:0])
+ # Row-major order matches the browser's metric unpivoting.
+ return pd.Series(selected[selected_metrics].to_numpy().ravel(),
dtype=object)
+
+ old_fraction = aggregate.partition(" as Fraction of ")[2]
+ fraction = {
+ "Total": "percent_total",
+ "Rows": "percent_row",
+ "Columns": "percent_col",
+ }.get(old_fraction) or show_values_as
+ for ri, row in enumerate(frame.index):
+ for ci, column in enumerate(frame.columns):
+ rd = row_depths.get(row, len(rows))
+ cd = column_depths.get(column, len(columns))
+ metric = _metric_of_column(column, metric_level)
+ selected_metrics = (
+ [metric] if metric in metrics else list(dict.fromkeys(metrics))
+ )
+ # Match the browser's mixed-metric guard: an axis/corner that
+ # combines different metrics has no meaningful single summary.
+ if len(selected_metrics) != 1:
+ frame.iloc[ri, ci] = None
+ continue
+ value = _reduce_result_values(
+ contributing(row, column, rd, cd, selected_metrics), aggregate
+ )
+ if fraction in SHOW_VALUES_AS_PERCENT_MODES:
+ dr, dc = rd, cd
+ if fraction == "percent_total":
+ dr, dc = 0, 0
+ elif (fraction == "percent_row") != metrics_on_rows:
+ dc = 0
+ else:
+ dr = 0
+ denominator = _reduce_result_values(
Review Comment:
The denominator is re-reduced for every cell, and for a `... as Fraction of
Total` choice that is a reduction over the whole source each time. A 100x100
grid built from 10,000 records visits about 100M observations (roughly 20s here
versus 0.2s on the non-result path), which could hit scheduled-report and
download timeouts. Can the reduced value be cached per (scope, metric)?
##########
tests/unit_tests/charts/test_client_processing.py:
##########
@@ -4634,3 +4634,220 @@ def
test_apply_client_processing_csv_format_pivot_table_multiple_rows():
("London", "Consumer"),
("London", "Corporate"),
}
+
+
[email protected](
+ "aggregate,expected",
+ [("Median", 14.5), ("Average", 32.5), ("Sum", 130), ("Count", 4)],
+)
+def test_result_aggregation_uses_original_values_for_corner(aggregate,
expected):
+ """The grand summary must not be a median/mean of intermediate
summaries."""
+ data = pd.DataFrame(
+ {
+ "region": ["A", "A", "A", "B"],
+ "city": ["a", "b", "c", "d"],
+ "period": ["x", "y", "z", "x"],
+ "sales": [1, 9, 20, 100],
+ }
+ )
+ result = pivot_table_v2(
+ data,
+ {
+ "groupbyRows": ["region", "city"],
+ "groupbyColumns": ["period"],
+ "metrics": ["sales"],
+ "aggregateFunction": aggregate,
+ "rowTotals": True,
+ "colTotals": True,
+ },
+ apply_number_format=False,
+ )
+ label = f"Total ({aggregate})"
+ assert result.loc[(label, ""), (label, "")] == expected
+
+
+def test_result_median_export_ignores_nulls():
+ """Null metrics are missing observations, not zero-valued observations."""
+ result = pivot_table_v2(
+ pd.DataFrame(
+ {
+ "group": ["a", "b", "c", "d"],
+ "sales": [None, 0, 10, 20],
+ }
+ ),
+ {
+ "groupbyRows": ["group"],
+ "metrics": ["sales"],
+ "aggregateFunction": "Median",
+ "colTotals": True,
+ },
+ apply_number_format=False,
+ )
+ assert result.loc[("Total (Median)",), ("sales",)] == 10
+
+
[email protected](
+ "aggregate,expected", [("Metric", 19), ("Average", 15), ("Median", 15)]
+)
+def test_result_mode_discards_stored_database_rollups(
+ aggregate: str, expected: float
+) -> None:
+ """A stored rollup query must not double-count its precomputed
summaries."""
+ result = pivot_table_v2(
+ grouping_sets_df(),
+ {
+ "groupbyRows": ["nation"],
+ "groupbyColumns": ["gender"],
+ "metrics": ["AVG(num)"],
+ "aggregateFunction": aggregate,
+ "rowTotals": True,
+ "colTotals": True,
+ },
+ apply_number_format=False,
+ )
+ assert result.iloc[-1, -1] == expected
+
+
[email protected]("layout", ["ROWS", "COLUMNS"])
[email protected]("transpose", [True, False])
+def test_result_median_layouts_blank_mixed_metric_corner(
+ layout: str, transpose: bool
+) -> None:
+ """Mixed metric corners stay blank regardless of metric layout and
transpose."""
+ result = pivot_table_v2(
+ pd.DataFrame(
+ {
+ "region": ["A", "B"],
+ "period": ["x", "y"],
+ "sales": [10, 100],
+ "profit": [20, 200],
+ }
+ ),
+ {
+ "groupbyRows": ["region"],
+ "groupbyColumns": ["period"],
+ "metrics": ["sales", "profit"],
+ "aggregateFunction": "Median",
+ "rowTotals": True,
+ "colTotals": True,
+ "combineMetric": True,
+ "metricsLayout": layout,
+ "transposePivot": transpose,
+ },
+ apply_number_format=False,
+ )
+ assert pd.isna(result.iloc[-1, -1])
+
+
[email protected](
+ "grand,subtotals", [(True, False), (False, True), (True, True)]
+)
+def test_result_summary_visibility(grand: bool, subtotals: bool) -> None:
+ """A subtotal can be visible without a grand total, and vice versa."""
+ result = pivot_table_v2(
+ pd.DataFrame(
+ {
+ "region": ["A", "A", "B"],
+ "city": ["a", "b", "c"],
+ "sales": [10, 20, 100],
+ }
+ ),
+ {
+ "groupbyRows": ["region", "city"],
+ "metrics": ["sales"],
+ "aggregateFunction": "Average",
+ "colTotals": grand,
+ "rowSubTotals": subtotals,
+ },
+ apply_number_format=False,
+ )
+ assert (("Total (Average)", "") in result.index) == grand
+ assert (("A", "Subtotal") in result.index) == subtotals
+ if subtotals:
+ assert result.loc[("A", "Subtotal"), ("sales",)] == 15
+
+
[email protected]("layout", ["ROWS", "COLUMNS"])
[email protected]("combine", [False, True])
[email protected]("kind", ["Sum", "Count"])
[email protected](
+ "scope,sum_expected,count_expected",
+ [("Total", 0.1, 0.25), ("Rows", 1 / 3, 0.5), ("Columns", 0.25, 0.5)],
+)
+def test_result_fractions_keep_each_metric_denominator(
+ layout: str,
+ combine: bool,
+ kind: str,
+ scope: str,
+ sum_expected: float,
+ count_expected: float,
+) -> None:
+ """A second metric must not dilute another metric's exported fractions."""
+ result = pivot_table_v2(
+ pd.DataFrame(
+ {
+ "region": ["A", "A", "B", "B"],
+ "period": ["Q1", "Q2", "Q1", "Q2"],
+ "sales": [10, 20, 30, 40],
+ "profit": [100, 200, 300, 400],
+ }
+ ),
+ {
+ "groupbyRows": ["region"],
+ "groupbyColumns": ["period"],
+ "metrics": ["sales", "profit"],
+ "aggregateFunction": f"{kind} as Fraction of {scope}",
+ "rowTotals": True,
+ "colTotals": True,
+ "combineMetric": combine,
+ "metricsLayout": layout,
+ },
+ apply_number_format=False,
+ )
+ row: tuple[str, ...]
+ column: tuple[str, ...]
+ for metric in ("sales", "profit"):
+ if layout == "ROWS":
+ row = ("A", metric) if combine else (metric, "A")
+ column = ("Q1",)
+ else:
+ row = ("A",)
+ column = ("Q1", metric) if combine else (metric, "Q1")
+ expected = sum_expected if kind == "Sum" else count_expected
+ assert result.loc[row, column] == pytest.approx(expected)
+ assert pd.isna(result.iloc[-1, -1])
+
+
[email protected](
+ "aggregate,expected",
+ [
+ ("Sum", 16),
+ ("Average", 4),
+ ("Median", 3),
+ ("Sample Variance", 12),
Review Comment:
Every leaf in this fixture has exactly one observation, but only the
four-observation grand total is asserted. Removing the singleton handling in
`_reduce_result_values` would turn every populated leaf's variance/deviation
into NaN while these cases still pass. Could you assert that the `Sample
Variance` and `Sample Standard Deviation` leaf cells equal 0?
##########
docs/docs/using-superset/pivot-result-exports.md:
##########
@@ -0,0 +1,56 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+# Pivot result aggregation exports
+
+This backend follow-up to #44660 extracts the result reducers and export tests
+from #44647. It does not restore frontend controls or add another migration.
+The frontend restoration must ship with this work before result selections can
+be expected to agree across browser and tabular export paths.
+
+Each cell, subtotal and grand summary is reduced from its original contributing
+grouped metric values. Query filters and limits have already been applied.
+Shared totals combining different metrics are blank; a fraction's denominator
+uses only its own metric. Transposition and metric placement do not change that
+rule. Stored GROUPING SETS data is split into leaves before result aggregation,
+so stored database rollups are not counted as additional observations.
+
+Explicit `Metric`, missing and unrecognised selections retain metric-definition
+behavior. Applying database rollups in Actual Values mode overlaps with #44631;
+once that merges, retain its behavior behind the non-result-mode guard. Its
+regression fixtures using `aggregateFunction: "Average"` need to use `Metric`
+when they intend database totals: Average becomes an explicit result reducer.
+
+## Integration decisions still open
Review Comment:
This page is picked up by the Using Superset sidebar, but most of it
(extraction from #44647, pending merges, a frontend revision hash, open
integration decisions) is PR coordination rather than something an analyst can
act on. Should that live in the PR description, with this page limited to the
supported behavior and how to configure the export?
##########
superset/charts/client_processing.py:
##########
@@ -752,6 +912,30 @@ def collapse(block: pd.DataFrame) -> tuple[pd.DataFrame,
str]:
else None,
)
+ if result_aggregation:
+ df = df.loc[
+ [
+ key
+ for key in df.index
+ if key not in row_prefix_depth
+ or (
+ render_column_total
+ if row_prefix_depth[key] == 0
+ else show_row_subtotals is not False
+ )
+ ],
+ [
+ key
+ for key in df.columns
+ if key not in column_prefix_depth
Review Comment:
With `combineMetric` off, the `(sales, Subtotal)` column has dimension depth
0, so this check classifies it as a grand total and follows `rowTotals` instead
of `colSubTotals`. For columns `period` with metrics `sales, profit`,
`colSubTotals=true` and `rowTotals=false`, the requested metric subtotals (30
and 300 for records `(A,x,10,100)`, `(A,y,20,200)`) disappear, and flipping the
two toggles shows them even though subtotals are off. Should depth 0 be
combined with the metric level before choosing the toggle?
##########
tests/unit_tests/charts/test_client_processing.py:
##########
@@ -4634,3 +4634,220 @@ def
test_apply_client_processing_csv_format_pivot_table_multiple_rows():
("London", "Consumer"),
("London", "Corporate"),
}
+
+
[email protected](
+ "aggregate,expected",
+ [("Median", 14.5), ("Average", 32.5), ("Sum", 130), ("Count", 4)],
+)
+def test_result_aggregation_uses_original_values_for_corner(aggregate,
expected):
+ """The grand summary must not be a median/mean of intermediate
summaries."""
+ data = pd.DataFrame(
+ {
+ "region": ["A", "A", "A", "B"],
+ "city": ["a", "b", "c", "d"],
+ "period": ["x", "y", "z", "x"],
+ "sales": [1, 9, 20, 100],
+ }
+ )
+ result = pivot_table_v2(
+ data,
+ {
+ "groupbyRows": ["region", "city"],
+ "groupbyColumns": ["period"],
+ "metrics": ["sales"],
+ "aggregateFunction": aggregate,
+ "rowTotals": True,
+ "colTotals": True,
+ },
+ apply_number_format=False,
+ )
+ label = f"Total ({aggregate})"
+ assert result.loc[(label, ""), (label, "")] == expected
+
+
+def test_result_median_export_ignores_nulls():
+ """Null metrics are missing observations, not zero-valued observations."""
+ result = pivot_table_v2(
+ pd.DataFrame(
+ {
+ "group": ["a", "b", "c", "d"],
+ "sales": [None, 0, 10, 20],
+ }
+ ),
+ {
+ "groupbyRows": ["group"],
+ "metrics": ["sales"],
+ "aggregateFunction": "Median",
+ "colTotals": True,
+ },
+ apply_number_format=False,
+ )
+ assert result.loc[("Total (Median)",), ("sales",)] == 10
+
+
[email protected](
+ "aggregate,expected", [("Metric", 19), ("Average", 15), ("Median", 15)]
+)
+def test_result_mode_discards_stored_database_rollups(
+ aggregate: str, expected: float
+) -> None:
+ """A stored rollup query must not double-count its precomputed
summaries."""
+ result = pivot_table_v2(
+ grouping_sets_df(),
+ {
+ "groupbyRows": ["nation"],
+ "groupbyColumns": ["gender"],
+ "metrics": ["AVG(num)"],
+ "aggregateFunction": aggregate,
+ "rowTotals": True,
+ "colTotals": True,
+ },
+ apply_number_format=False,
+ )
+ assert result.iloc[-1, -1] == expected
+
+
[email protected]("layout", ["ROWS", "COLUMNS"])
[email protected]("transpose", [True, False])
+def test_result_median_layouts_blank_mixed_metric_corner(
+ layout: str, transpose: bool
+) -> None:
+ """Mixed metric corners stay blank regardless of metric layout and
transpose."""
+ result = pivot_table_v2(
+ pd.DataFrame(
+ {
+ "region": ["A", "B"],
+ "period": ["x", "y"],
+ "sales": [10, 100],
+ "profit": [20, 200],
+ }
+ ),
+ {
+ "groupbyRows": ["region"],
+ "groupbyColumns": ["period"],
+ "metrics": ["sales", "profit"],
+ "aggregateFunction": "Median",
+ "rowTotals": True,
+ "colTotals": True,
+ "combineMetric": True,
+ "metricsLayout": layout,
+ "transposePivot": transpose,
+ },
+ apply_number_format=False,
+ )
+ assert pd.isna(result.iloc[-1, -1])
+
+
[email protected](
+ "grand,subtotals", [(True, False), (False, True), (True, True)]
+)
+def test_result_summary_visibility(grand: bool, subtotals: bool) -> None:
+ """A subtotal can be visible without a grand total, and vice versa."""
+ result = pivot_table_v2(
+ pd.DataFrame(
+ {
+ "region": ["A", "A", "B"],
+ "city": ["a", "b", "c"],
+ "sales": [10, 20, 100],
+ }
+ ),
+ {
+ "groupbyRows": ["region", "city"],
+ "metrics": ["sales"],
+ "aggregateFunction": "Average",
+ "colTotals": grand,
+ "rowSubTotals": subtotals,
+ },
+ apply_number_format=False,
+ )
+ assert (("Total (Average)", "") in result.index) == grand
+ assert (("A", "Subtotal") in result.index) == subtotals
+ if subtotals:
+ assert result.loc[("A", "Subtotal"), ("sales",)] == 15
+
+
[email protected]("layout", ["ROWS", "COLUMNS"])
[email protected]("combine", [False, True])
[email protected]("kind", ["Sum", "Count"])
[email protected](
+ "scope,sum_expected,count_expected",
+ [("Total", 0.1, 0.25), ("Rows", 1 / 3, 0.5), ("Columns", 0.25, 0.5)],
+)
+def test_result_fractions_keep_each_metric_denominator(
Review Comment:
Every fraction test here uses `apply_number_format=False`, and the existing
formatting tests go through `showValuesAs`, so the new `" as Fraction of "`
conditions in `pivot_table_v2` and `apply_client_processing` are never
exercised. Dropping them would leave these tests green while legacy fractions
export as plain numbers/currency. Could you add a case for `Sum as Fraction of
Total` without `showValuesAs` asserting `"10.0%"` for a 0.1 ratio (including
with a currency override), plus an XLSX assertion of value 0.1 with number
format `0.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: [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]
