Repository navigation
fix(pivot-table): blank mixed-metric totals in exports and reports #45311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e9ab69e
a35d176
b6ec96f
af3628b
486e597
a9b0aaf
63f63d0
c7f29d2
39f624d
044e2a5
9240830
4f9a8cb
e4456b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -157,23 +157,20 @@ def get_metric_rollup_reducers( | |||||||||||
| return reducers | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def _collapsed_metric(present: list[Any], metrics: list[str]) -> Any: | ||||||||||||
| def _collapsed_metric(present: list[Any] | set[Any]) -> Any: | ||||||||||||
| """ | ||||||||||||
| The metric a rollup spanning `present` stands for. | ||||||||||||
|
|
||||||||||||
| A total that collapses the metric axis is undefined in the renderer, which | ||||||||||||
| resolves it to the last metric pushed into the shared slot (see the | ||||||||||||
| ``metricAxis`` handling in ``react-pivottable/utilities.ts``). Mirror that | ||||||||||||
| by taking the last metric in the configured order, so exported percentages | ||||||||||||
| match the chart rather than summing metrics that share no unit. | ||||||||||||
| The metric a rollup spanning `present` stands for, or `None` when `present` | ||||||||||||
| spans more than one distinct metric. | ||||||||||||
|
|
||||||||||||
| A total that collapses the metric axis has no single meaningful value once | ||||||||||||
| it mixes distinct metrics -- e.g. a "Total" cell combining `MAX(sales)` | ||||||||||||
| with `MEDIAN(msrp)`. The browser change in #44657 blanks that case instead | ||||||||||||
| of picking one arbitrarily (see ``metricAxis`` handling in | ||||||||||||
| ``react-pivottable/utilities.ts``); mirror it here so exported/scheduled | ||||||||||||
| values agree with what the chart shows. | ||||||||||||
| """ | ||||||||||||
| distinct = set(present) | ||||||||||||
| if len(distinct) == 1: | ||||||||||||
| return distinct.pop() | ||||||||||||
| for metric in reversed(metrics): | ||||||||||||
| if metric in distinct: | ||||||||||||
| return metric | ||||||||||||
| return None | ||||||||||||
| distinct = present if isinstance(present, set) else set(present) | ||||||||||||
| return next(iter(distinct)) if len(distinct) == 1 else None | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def _broadcast(total: pd.Series, block: pd.DataFrame, axis: int) -> pd.DataFrame: | ||||||||||||
|
|
@@ -194,14 +191,30 @@ def _metric_of_column(column: Any, metric_level: int) -> Any: | |||||||||||
| return column[metric_level] if isinstance(column, tuple) else column | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def _rollup_metric(column: Any, metric_level: int, metrics: set[str]) -> Any: | ||||||||||||
| """The metric to read from a rollup record for a cell in ``column``. | ||||||||||||
|
|
||||||||||||
| A column carrying a total label instead of a metric name collapses the metric | ||||||||||||
| axis; see ``_collapsed_metric``. | ||||||||||||
| """ | ||||||||||||
| name = _metric_of_column(column, metric_level) | ||||||||||||
| return name if name in metrics else _collapsed_metric(metrics) | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def _reduce( | ||||||||||||
| data: Union[pd.DataFrame, pd.Series], | ||||||||||||
| reducer: str, | ||||||||||||
| axis: Optional[int] = None, | ||||||||||||
| ) -> Any: | ||||||||||||
| """Apply a rollup reducer (``sum``/``min``/``max``), skipping empty cells.""" | ||||||||||||
| """Apply a rollup reducer (``sum``/``min``/``max``), skipping empty cells. | ||||||||||||
|
|
||||||||||||
| A rollup with no non-null observations stays blank: | ||||||||||||
| ``sum`` otherwise defaults an all-NaN input to 0, treating missing values | ||||||||||||
| as a measured zero. | ||||||||||||
| """ | ||||||||||||
| method = getattr(data, reducer) | ||||||||||||
| return method(axis=axis) if axis is not None else method() | ||||||||||||
| kwargs = {"min_count": 1} if reducer == "sum" else {} | ||||||||||||
| return method(axis=axis, **kwargs) if axis is not None else method(**kwargs) | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def _rollup_index( | ||||||||||||
|
|
@@ -271,12 +284,6 @@ def _apply_rollup_totals( # pylint: disable=too-many-arguments,too-many-locals | |||||||||||
| keyed = _rollup_index(rollup_levels) | ||||||||||||
| metric_names = set(metrics) | ||||||||||||
|
|
||||||||||||
| def metric_of(column: Any) -> Any: | ||||||||||||
| name = _metric_of_column(column, metric_level) | ||||||||||||
| return ( | ||||||||||||
| name if name in metric_names else _collapsed_metric(list(metrics), metrics) | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| def lookup(row: Any, column: Any) -> tuple[bool, Any]: | ||||||||||||
| row_depth = row_prefix_depth.get(row, len(rows)) | ||||||||||||
| column_depth = column_prefix_depth.get(column, len(columns)) | ||||||||||||
|
|
@@ -287,7 +294,7 @@ def lookup(row: Any, column: Any) -> tuple[bool, Any]: | |||||||||||
| record = keyed(grouped).get(key) | ||||||||||||
| if record is None: | ||||||||||||
| return False, None | ||||||||||||
| return True, record.get(metric_of(column)) | ||||||||||||
| return True, record.get(_rollup_metric(column, metric_level, metric_names)) | ||||||||||||
|
|
||||||||||||
| # Index positionally: a tuple label on a MultiIndex is ambiguous to `.loc`. | ||||||||||||
| for column_position, column in enumerate(df.columns): | ||||||||||||
|
|
@@ -331,12 +338,6 @@ def _rollup_denominators( # pylint: disable=too-many-arguments,too-many-locals | |||||||||||
| keyed = _rollup_index(rollup_levels) | ||||||||||||
| metric_names = set(metrics) | ||||||||||||
|
|
||||||||||||
| def metric_of(column: Any) -> Any: | ||||||||||||
| name = _metric_of_column(column, metric_level) | ||||||||||||
| return ( | ||||||||||||
| name if name in metric_names else _collapsed_metric(list(metrics), metrics) | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| def denominator(row: Any, column: Any) -> tuple[bool, Any]: | ||||||||||||
| if mode == ShowValuesAs.PERCENT_OF_TOTAL: | ||||||||||||
| grouped: list[str] = [] | ||||||||||||
|
|
@@ -356,7 +357,7 @@ def denominator(row: Any, column: Any) -> tuple[bool, Any]: | |||||||||||
| record = keyed(grouped).get(key) | ||||||||||||
| if record is None: | ||||||||||||
| return False, None | ||||||||||||
| return True, record.get(metric_of(column)) | ||||||||||||
| return True, record.get(_rollup_metric(column, metric_level, metric_names)) | ||||||||||||
|
|
||||||||||||
| resolved = [[denominator(row, column) for column in df.columns] for row in df.index] | ||||||||||||
| values = pd.DataFrame( | ||||||||||||
|
|
@@ -397,8 +398,8 @@ def _apply_show_values_as( # pylint: disable=too-many-arguments | |||||||||||
| - A total is rolled up within a single metric, so a cell is never divided by | ||||||||||||
| a total that mixes in another metric, and each metric uses its own | ||||||||||||
| reducer -- a MIN/MAX metric divides by the row's minimum/maximum rather | ||||||||||||
| than its sum. A total that collapses the metric axis resolves to a single | ||||||||||||
| metric the way the renderer does; see ``_collapsed_metric``. | ||||||||||||
| than its sum. A total that combines distinct metrics stays blank; see | ||||||||||||
| ``_collapsed_metric``. | ||||||||||||
|
|
||||||||||||
| A zero denominator yields NaN (blank) rather than infinity, matching | ||||||||||||
| ``pandas_postprocessing.pivot``'s ``show_values_as``. | ||||||||||||
|
|
@@ -418,6 +419,8 @@ def _apply_show_values_as( # pylint: disable=too-many-arguments | |||||||||||
| derived = pd.DataFrame(np.nan, index=numeric.index, columns=numeric.columns) | ||||||||||||
| for metric in dict.fromkeys(metric_of_column): | ||||||||||||
| selection = np.array([column == metric for column in metric_of_column]) | ||||||||||||
| # Mixed-metric numerators are blank, so the fallback cross-metric | ||||||||||||
| # denominator cannot produce a displayed value. | ||||||||||||
| denominator_selection = ( | ||||||||||||
| selection if metric is not None else np.ones(len(selection), dtype=bool) | ||||||||||||
| ) | ||||||||||||
|
|
@@ -431,8 +434,7 @@ def _apply_show_values_as( # pylint: disable=too-many-arguments | |||||||||||
| metric_of_column, denominator_selection, strict=True | ||||||||||||
| ) | ||||||||||||
| if keep and column_metric is not None | ||||||||||||
| ], | ||||||||||||
| metrics, | ||||||||||||
| ] | ||||||||||||
| ) | ||||||||||||
| reducer = reducers.get(str(denominator_metric), DEFAULT_ROLLUP_REDUCER) | ||||||||||||
| if denominator_metric is not None: | ||||||||||||
|
|
@@ -606,27 +608,37 @@ def pivot_df( # pylint: disable=too-many-locals, too-many-arguments, too-many-s | |||||||||||
| # still divides by itself: a MAX metric's row total is the row's maximum, | ||||||||||||
| # and dividing that maximum by itself reads 100%. | ||||||||||||
| totals_metric_level = df.columns.nlevels - 1 if combine_metrics else 0 | ||||||||||||
| # A column carrying a total label rather than a metric name rolls up | ||||||||||||
| # everything; with no metric to resolve it to, it sums. | ||||||||||||
| # Single-metric totals retain their reducer. Mixed-metric totals are | ||||||||||||
| # blanked before reduction; the fallback cannot make them meaningful. | ||||||||||||
| cross_metric_reducer = ( | ||||||||||||
| reducers.get(metrics[0], DEFAULT_ROLLUP_REDUCER) | ||||||||||||
| if len(set(metrics)) == 1 | ||||||||||||
| else DEFAULT_ROLLUP_REDUCER | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| def collapse(block: pd.DataFrame) -> tuple[pd.DataFrame, str]: | ||||||||||||
| """Narrow a total's source columns to one metric, and pick its reducer.""" | ||||||||||||
| metric_names = set(metrics) | ||||||||||||
| metric_names = set(metrics) | ||||||||||||
|
|
||||||||||||
| def collapse(block: pd.DataFrame) -> tuple[Optional[pd.DataFrame], str]: | ||||||||||||
| """Narrow a total's source columns to one metric, and pick its reducer. | ||||||||||||
|
|
||||||||||||
| Returns `(None, reducer)` when `block` spans more than one distinct | ||||||||||||
| metric: such a total has no single meaningful value, so the caller | ||||||||||||
| blanks it instead of summing across metrics that share no unit | ||||||||||||
| (matches the browser change proposed in #44657). | ||||||||||||
| """ | ||||||||||||
| present = [ | ||||||||||||
| _metric_of_column(column, totals_metric_level) for column in block.columns | ||||||||||||
| ] | ||||||||||||
| known = [metric for metric in present if metric in metric_names] | ||||||||||||
| metric = _collapsed_metric(known, metrics) if known else None | ||||||||||||
| if metric is None: | ||||||||||||
| if not known: | ||||||||||||
| return block, cross_metric_reducer | ||||||||||||
| metric = _collapsed_metric(known) | ||||||||||||
| if metric is None: | ||||||||||||
| return None, DEFAULT_ROLLUP_REDUCER | ||||||||||||
| keep = [column == metric for column in present] | ||||||||||||
| return block.loc[:, keep], reducers.get(str(metric), DEFAULT_ROLLUP_REDUCER) | ||||||||||||
|
|
||||||||||||
| mixed_metric_totals: list[Any] = [] | ||||||||||||
| if show_rows_total: | ||||||||||||
| # add subtotal for each group and overall total; we start from the | ||||||||||||
| # overall group, and iterate deeper into subgroups | ||||||||||||
|
|
@@ -654,8 +666,13 @@ def collapse(block: pd.DataFrame) -> tuple[pd.DataFrame, str]: | |||||||||||
| # to a number. The row totals below already coerce; do the | ||||||||||||
| # same here so a total means the same thing on both axes. | ||||||||||||
| block = block.apply(pd.to_numeric, errors="coerce") | ||||||||||||
| if percent_mode: | ||||||||||||
| source, reducer = collapse(block) | ||||||||||||
| source, reducer = collapse(block) | ||||||||||||
| mixed_metrics = ( | ||||||||||||
| source is None and aggfunc != CURRENCY_CONTEXT_AGGREGATION | ||||||||||||
| ) | ||||||||||||
| if mixed_metrics: | ||||||||||||
| subtotal = pd.Series(np.nan, index=block.index) | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Until the paired browser change lands, the chart still displays the last metric in shared total slots while exports blank them, so their results disagree. Assessment: 🟠 Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** superset/charts/client_processing.py
**Line:** 675:675
**Comment:**
*Api Mismatch: Until the paired browser change lands, the chart still displays the last metric in shared total slots while exports blank them, so their results disagree.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||
| elif percent_mode: | ||||||||||||
| subtotal = _reduce(source, reducer, axis=1) | ||||||||||||
| else: | ||||||||||||
| subtotal = summarize_frame(aggfunc, block, axis=1) | ||||||||||||
|
|
@@ -665,6 +682,8 @@ def collapse(block: pd.DataFrame) -> tuple[pd.DataFrame, str]: | |||||||||||
| # insert column after subgroup | ||||||||||||
| df.insert(int(slice_.stop), subtotal_name, subtotal) | ||||||||||||
| inserted_columns.append(subtotal_name) | ||||||||||||
| if mixed_metrics: | ||||||||||||
| mixed_metric_totals.append(subtotal_name) | ||||||||||||
| column_prefix_depth[subtotal_name] = ( | ||||||||||||
| level if combine_metrics else max(0, level - 1) | ||||||||||||
| ) | ||||||||||||
|
|
@@ -686,7 +705,9 @@ def collapse(block: pd.DataFrame) -> tuple[pd.DataFrame, str]: | |||||||||||
| ) | ||||||||||||
| raise | ||||||||||||
|
|
||||||||||||
| subtotal_values = df.iloc[slice_, :] | ||||||||||||
| # These labels were inserted above; no columns are removed | ||||||||||||
| # before this point. Keep strict lookup to catch broken invariants. | ||||||||||||
| subtotal_values = df.iloc[slice_, :].drop(columns=mixed_metric_totals) | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked the lifecycle and added an invariant comment in 044e2a5. mixed_metric_totals is populated only after a successful df.insert, and no columns are removed before this drop; row concatenation explicitly reindexes to df.columns. Subtotal labels include the complete group prefix, so different groups do not share labels; df.insert also rejects an actual duplicate before it can enter this list. Keeping strict drop is useful here: silently ignoring a missing label would hide an invariant violation. The combined/transposed subtotal tests exercise both metric layouts. |
||||||||||||
| if aggfunc != CURRENCY_CONTEXT_AGGREGATION: | ||||||||||||
| subtotal_values = subtotal_values.apply( | ||||||||||||
| pd.to_numeric, errors="coerce" | ||||||||||||
|
|
@@ -702,7 +723,11 @@ def collapse(block: pd.DataFrame) -> tuple[pd.DataFrame, str]: | |||||||||||
| subtotal.name = tuple([*subgroup, total, *([""] * depth)]) # noqa: C409 | ||||||||||||
| # insert row after subgroup | ||||||||||||
| df = pd.concat( | ||||||||||||
| [df[: slice_.stop], subtotal.to_frame().T, df[slice_.stop :]] | ||||||||||||
| [ | ||||||||||||
| df[: slice_.stop], | ||||||||||||
| subtotal.to_frame().T.reindex(columns=df.columns), | ||||||||||||
| df[slice_.stop :], | ||||||||||||
| ] | ||||||||||||
| ) | ||||||||||||
| inserted_rows.append(subtotal.name) | ||||||||||||
| row_prefix_depth[subtotal.name] = level | ||||||||||||
|
|
@@ -752,13 +777,13 @@ def collapse(block: pd.DataFrame) -> tuple[pd.DataFrame, str]: | |||||||||||
| else None, | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| # if we want to apply the metrics on the rows we need to pivot the | ||||||||||||
| # dataframe back | ||||||||||||
| # Replace placeholders before transposing, while column dtypes are intact. | ||||||||||||
| df.replace("SUPERSET_PANDAS_NAN", np.nan, inplace=True) | ||||||||||||
| if apply_metrics_on_rows: | ||||||||||||
| df = df.T | ||||||||||||
| # Mixed blank columns must not coerce integer cells through float64 | ||||||||||||
| # during transposition (which would round integers above 2**53). | ||||||||||||
| df = df.astype(object).T if mixed_metric_totals else df.T | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: For metrics-on-rows mixed totals, XLSX processing re-infers these object columns as numeric; blank cells make Assessment: 🟠 Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** superset/charts/client_processing.py
**Line:** 784:784
**Comment:**
*Type Error: For metrics-on-rows mixed totals, XLSX processing re-infers these object columns as numeric; blank cells make `pd.to_numeric` round integers above 2**53.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 044e2a5. Confirmed that both pd.to_numeric and scalar mapping over nullable integers can coerce through float64. Excel preparation now uses nullable numeric conversion followed by object conversion before mapping oversized values to text. test_pivot_mixed_totals_preserve_integer_xlsx exercises the full XLSX path with metrics on rows across Combine metrics/Transpose combinations, and test_numeric_column_with_blanks_preserves_large_integers checks the utility directly. All five cases fail before the fix and pass after it; the workbook retains exact large integers as text, ordinary values as numeric cells, and blank cells as blank.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The object dtype is intentional here: transposing mixed blank columns with integer columns otherwise rounds integers above 2**53. Pivot formatting uses format_column per metric rather than skipping object columns. Your downstream Excel concern was valid: fixed in 044e2a5 by preserving nullable integer values through Excel type preparation. test_pivot_mixed_totals_preserve_integer_xlsx checks all four Combine metrics/Transpose combinations through the full export path, including exact text for large integers and numeric workbook cells for small values. The chart and Excel unit suites pass (475 passed, 2 xfailed).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium — Metrics-on-rows CSV ignores
Repro with
With Casting only when an integer column actually needs exact values fixes the float case. I ran this change locally:
Suggested change
Worth adding a |
||||||||||||
|
|
||||||||||||
| # replace the remaining temporary placeholder string for np.nan after pivoting | ||||||||||||
| df.replace("SUPERSET_PANDAS_NAN", np.nan, inplace=True) | ||||||||||||
| df.rename( | ||||||||||||
| index={"SUPERSET_PANDAS_NAN": np.nan}, | ||||||||||||
| columns={"SUPERSET_PANDAS_NAN": np.nan}, | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
min_count=1applies to every_reducesum, but the docstring says it is for percentage-mode rollups only, so any caller using_reduceoutside percent mode now returns NaN instead of 0 for all-empty input.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified the helper docstring in 044e2a5: every all-empty reduction stays blank. I checked all four call sites: two are inside _apply_show_values_as, and the other two are explicitly guarded by percent_mode. There are no non-percentage callers today, so this does not change Actual Values aggregation. test_pivot_rollup_reducer_preserves_blank_and_zero covers the blank-versus-measured-zero distinction for sum/min/max.