fitzee commented on code in PR #44679:
URL: https://github.com/apache/superset/pull/44679#discussion_r4300440068
##########
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:
Fixed in 71d3b975becafbea80ac302955712879d26b4c7a. Reduced values are cached
per dimension scope and metric. The regression counts observations reduced,
rather than relying on timing: numerator and denominator work totals two passes
over the source for each fraction mode.
Regression test:
`tests/unit_tests/charts/test_client_processing.py::test_result_fraction_reduces_each_scope_once`.
The new regression fails against the previous implementation and passes with
this commit. Full chart suite: 434 passed, 2 expected failures.
Siblings checked:
- `superset/charts/client_processing.py` — fixed in
71d3b975becafbea80ac302955712879d26b4c7a: Total, Rows and Columns fraction
denominators.
##########
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:
Fixed in 71d3b975becafbea80ac302955712879d26b4c7a. The visibility check now
distinguishes a metric subtotal from a grand total even when dimension depth is
zero.
Regression test:
`tests/unit_tests/charts/test_client_processing.py::test_result_metric_subtotal_visibility`.
The new regression fails against the previous implementation and passes with
this commit. Full chart suite: 434 passed, 2 expected failures.
Siblings checked:
- `superset/charts/client_processing.py` — fixed in
71d3b975becafbea80ac302955712879d26b4c7a: metrics on rows and columns, both
toggle directions; currency-context layout.
##########
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:
Added `test_result_export_fraction_formatting` in
71d3b975becafbea80ac302955712879d26b4c7a. It covers Sum as Fraction of Total
without showValuesAs, with and without a currency override: formatted output is
"10.0%", and the XLSX cell is numeric 0.1 with number format "0.0%". It also
covers plain Sum with showValuesAs set, so the export cannot accidentally
format the result as a percentage.
CSV uses the existing raw numeric export contract (0.1); the test checks
that separately. The full chart suite passes (434 tests, 2 expected failures).
Siblings checked:
- `superset/charts/client_processing.py` — not affected: existing formatted
pivot results, raw CSV and numeric XLSX with and without currency overrides
behavior has regression coverage in this commit.
##########
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:
Added `test_result_single_observation_statistic` in
71d3b975becafbea80ac302955712879d26b4c7a. Both Sample Variance and Sample
Standard Deviation explicitly assert zero for each singleton leaf,
independently of grand-summary assertions. The full chart suite passes (434
tests, 2 expected failures).
Siblings checked:
- `superset/charts/client_processing.py` — not affected: existing both
Sample Variance and Sample Standard Deviation singleton leaves behavior has
regression coverage in this commit.
##########
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:
Rewrote the page in 71d3b975becafbea80ac302955712879d26b4c7a around
analyst-facing behavior: selecting a result aggregation, subtotal visibility,
fraction denominators, export formatting and missing values. Removed extraction
history, PR references, revision hashes and pending integration decisions from
the user documentation. Those coordination details and remaining checks live in
the PR description.
--
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]
