Repository navigation
fix(pivot-table): blank mixed-metric totals in exports and reports - #45311
Conversation
… or pick one _collapsed_metric picked the last configured metric for any rollup spanning 2+ distinct metrics on the collapsed 'Total' pseudo-dimension, so the report/export path could show a specific, plausible-looking percentage for a cell that mixes e.g. MAX(sales) with MEDIAN(msrp). apache#44657 already changed the interactive chart to blank that cell, since combining metrics that share no unit has no single meaningful value; this brings the export/report path in line with it. - _collapsed_metric now returns None on a mixed present, instead of reversed(metrics)'s tie-break. - pivot_df's collapse() propagates that as a genuinely blank total (was: falling back to summing across every metric). - _reduce's sum uses min_count=1 so a rollup of nothing but blanks stays blank, rather than pandas defaulting an all-NaN sum to 0. Fixes apache#44725.
_apply_rollup_totals and _rollup_denominators each defined the same metric_of closure; replace both with a module-level _rollup_metric.
…finish-pivot-mixed-totals
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45311 +/- ##
===========================================
+ Coverage 65.15% 83.65% +18.50%
===========================================
Files 3053 3053
Lines 200638 200639 +1
Branches 46820 46820
===========================================
+ Hits 130717 167837 +37120
+ Misses 66981 29561 -37420
- Partials 2940 3241 +301
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rebenitez1802
left a comment
There was a problem hiding this comment.
Request changes: blanking mixed-metric totals is the right call and matches #44657's browser rule, but the NaN blanks turn integers into floats across the whole CSV, and the change in output needs an UPDATING.md entry before this leaves draft.
🟡 Medium — CSV exports turn every integer into a float
The blank Total column is float NaN (client_processing.py:672, again at :730). When the column-totals row is added (subtotal.to_frame().T, :719-723), every metric column is converted to float64. With 2+ integer metrics and both totals on, CSV rows change from SUM(a),CA,1,2,3 on master to SUM(a),CA,1.0,2.0,3.0. superset/utils/csv.py writes plain df.to_csv, so downstream readers see the floats, and integer sums above 2^53 lose precision. The 8.06797e+07 snapshot change in test_pivot_df_no_cols_no_rows_two_metrics is a sign of this, and the markdown snapshots hide it because tabulate prints 31290.0 as 31290. Fix: keep each column's type when building the totals row (e.g. subtotal.astype(object).to_frame().T.infer_objects()), or restore the original integer types on columns with no NaN. Add a test on the actual CSV text.
🟡 Medium — Export output changes with no UPDATING.md entry
_collapsed_metric (:160) blanks any total that combines different metric labels, even when every metric is a SUM. SUM(online_rev) + SUM(store_rev) used to export a total and now exports a blank. Scheduled CSV reports that feed the Total column into other systems will get empty strings without warning, under a header that still says "Total (Sum)". #44657's UPDATING.md change doesn't cover this either. Fix: add an UPDATING.md entry for blank mixed-metric totals in CSV/Excel exports and scheduled reports.
🟡 Medium — Must be rebased and re-checked after #44657
#44657 changes the same lines: pivot_v2_aggfunc_map[aggfunc](...) at :676 / :716 becomes summarize_frame(...). It also lets aggfunc be Median, Count and so on in pivot_table_v2. The two branches merge cleanly today, but the 16-case matrix assumes Actual Values means Sum. That assumption breaks once #44657's aggregation setting returns. Keep this PR draft as planned, and after #44657 lands, re-run the matrix with at least one non-Sum aggregation.
🟢 Low — min_count=1 only applies in percent modes
The all-blank-stays-blank rule lives in _reduce (:217), which only the percent modes use. Actual Values still sums with pd.Series.sum, so a single metric whose CA row is all NULL exports 0.0, while "% of total" exports blank and the browser shows blank. Either use the same rule in Actual Values, or narrow the PR description, which says all-empty rollups stay blank.
🟢 Low — Subtotals under "Combine metrics" also go blank, untested
With combineMetric=True, every per-group Subtotal column (e.g. ('boy','Subtotal')) is blank in all four modes. This probably matches the browser, but the matrix never enables "Combine metrics" or "Transpose pivot", and the description only mentions Total cells. Fix: add those cases to test_pivot_table_v2_blanks_mixed_metric_totals, plus a single-metric control case.
🟢 Low — Docstrings describe browser behaviour master doesn't have yet
The _apply_show_values_as docstring (:399-403) still says a mixed total "resolves to a single metric the way the renderer does", but it now blanks. The _collapsed_metric docstring (:160-172) and the new test docstring say the renderer already blanks; that only becomes true once #44657 merges. It's also worth a one-line comment saying the cross-metric denominator at :428-441 is harmless because the numerator is blank.
🟢 Low — Redundant check and per-cell set rebuild
In elif percent_mode and source is not None (:673), source is not None is always true at that point. _rollup_metric also rebuilds set(metrics) for every cell, even though the description says the set is precomputed. Fix: drop the redundant condition, and pass the precomputed set into _rollup_metric.
Two issues on master that this PR doesn't cause, each worth its own issue: Count, Count Unique Values, List Unique Values and the "as Fraction of" aggregations already crash with row totals on, and "Sum as Fraction of Total" raises an AttributeError at df.sum().sum().
mikebridge
left a comment
There was a problem hiding this comment.
Thanks for picking this up and finishing #44730, @fitzee. The mixed-metric blanking looks right to me, and it matches what #44657 does in the browser. I tried metrics on columns and on rows, Combine metrics on and off, transposed, subtotals, the grand-total corner, all four Show values as modes, and data with and without GROUPING SETS. Every cell that mixes metrics came out blank. Single-metric totals didn't change, and neither did totals for any one metric.
There's one thing I'd fix before this merges:
-
CSV exports now print integer metrics as floats when a pivot has two or more metrics and both totals are on. The new all-blank Total column makes the inserted total row a float Series, so
pd.concatin the column-totals pass turns every column into floats, leaf cells included. CSV/XLSX run withapply_number_format=False, soUK,20,20,40,6,10,16,56turns intoUK,20.0,20.0,40.0,6.0,10.0,16.0,. That's the same thing behind the8.06797e+07snapshot changes. Dropping the mixed columns before the row reduction and reindexing the new row fixed it for me locally, and all 374 chart tests still pass:subtotal_values = df.iloc[slice_, :].drop(columns=mixed_metric_totals) ... df = pd.concat([df[: slice_.stop], subtotal.to_frame().T.reindex(columns=df.columns), df[slice_.stop :]])
It also makes the
df[column] = np.nanpass afterwards unnecessary. Ato_csv()or dtype assertion for the two-metric, both-totals case would pin it.
Smaller things, take them or leave them:
- The
_apply_show_values_asdocstring still says a total that collapses the metric axis "resolves to a single metric the way the renderer does". Thecross_metric_reducercomment ("it sums") is out of date in the same way. - The 16-case matrix never turns on
combineMetric, which is where the per-group Subtotal columns become the mixed slots. I checked those by hand and they're blank, but it'd be good to add it to the parametrize. - On merge order: against #44657's current head, this merges cleanly and keeps
summarize_frame, and the merged test file passes. #44657 still needs a rebase, though, so it's worth checking again once that lands. Agreed that this stays in draft until then.
What I ran: the test file at the head (129 passed) and tests/unit_tests/charts/ (374 passed, 2 xfailed). The new tests fail against master's client_processing.py (27 failed). The red unit-tests (current) job was cancelled at the 55-minute timeout, not a test failure, so it probably just needs a re-run.
(Review drafted with Claude (AI) on behalf of @mikebridge.)
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Reviewed both sets of feedback against the latest branch. The existing 63f63d0 commit fixes integer CSV precision (including values above 2^53), adds the UPDATING.md entry, covers Sum/Average and combined/transposed layouts, corrects the stale denominator comments, and narrows the all-empty-rollup claim to percentage modes. I verified those changes locally. Pushed c7f29d2 to finish the remaining cleanup: Validation: 433 passed, 2 xfailed in the full charts unit suite, and all changed-file pre-commit checks pass (including mypy, Ruff and Pylint). The post-#44657 integration recheck remains pending because that PR is still open. This stays draft; after it lands, we still need to update against master and rerun the non-Sum cases plus hosted export/report checks. |
mikebridge
left a comment
There was a problem hiding this comment.
Thanks @fitzee, the follow-ups look good. Since the last round I checked each of @rebenitez1802's points and mine against c7f29d2.
- Integer precision: fixed. Dropping the mixed columns before the row reduction and reindexing the inserted row keeps integer columns intact. I ran a dense pivot through all 16 combinations of layout, Combine metrics, transpose and subtotals.
2**53 + 1and the totals built on it came out exact, with no.0anywhere. Outside the blank mixed slots, a head-vs-master CSV diff is identical. - Done: the docstrings, the denominator comment, the redundant
source is not None, the shared metric set and the combined/transposed subtotal tests. min_countwording: the description now limits the all-empty rule to the percent modes, which matches the code.
A few small things for the rebase:
- UPDATING.md: the entry is a
### Pivot Table mixed-metric export totalsheading at the top of## Next. Everything else in that section is a bullet, so the semantic NULL filter and containment-cache bullets below it would end up under the pivot heading. A- **Pivot Table mixed-metric export totals.** …bullet would fix that and also clear the only conflict with master. - Average matrix after #44657: I merged this locally onto #44657's current head (ae61bd9). The code merges cleanly, but 12 of the new
Average+ percent cases fail withKeyError: 'Total (Sum)'. With #44657,Averageis a result aggregation that takes precedence over Show values as, so the label becomesTotal (Average). The mixed slot is still blank in every case I tried, so it's only the expectation at thetotal = total_label() if mode else …line that needs to change. _reducedocstring: it still uses a mixed-metric slot as its example of an all-blank reduction, but mixed slots don't reach_reduceany more.- Precision claim in the description: "Integer values retain their CSV spelling and precision" only holds for dense, all-integer pivots. Sparse pivots and int + float metric pairs still print
30.0in the rows layout. That's the same on master, so it isn't something this PR introduced; it just needs narrower wording.
Agreed this stays in draft until #44657 lands. After that, a rebase with those two changes should be all it needs from me.
What I ran: the test file at head (188 passed), tests/unit_tests/charts/ (433 passed, 2 xfailed), the new precision test against the previous head's code (8 failed, as expected), and the merged-with-#44657 charts suite (444 passed, 12 failed as described).
|
@mikebridge Addressed the four items in 39f624d:
#44657 has merged, so I merged current master into this reviewed branch without rewriting its history and reran the combined chart suite: 457 passed, 2 xfailed. Changed-file pre-commit checks pass, including mypy, Ruff and Pylint. Only the intended three files differ from current master. CI on the new head is pending. Hosted CSV/Excel and scheduled-report verification remains outstanding. The PR's existing ready-for-review state is unchanged. |
| source is None and aggfunc != CURRENCY_CONTEXT_AGGREGATION | ||
| ) | ||
| if mixed_metrics: | ||
| subtotal = pd.Series(np.nan, index=block.index) |
There was a problem hiding this comment.
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: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Api mismatch
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| 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 |
There was a problem hiding this comment.
Suggestion: 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.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Type error
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 fixThere was a problem hiding this comment.
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.
CodeAnt PR Risk: Low Risk
Assessed commit: |
| """ | ||
| method = getattr(data, reducer) | ||
| return method(axis=axis) if axis is not None else method() | ||
| kwargs = {"min_count": 1} if reducer == "sum" else {} |
There was a problem hiding this comment.
min_count=1 applies to every _reduce sum, but the docstring says it is for percentage-mode rollups only, so any caller using _reduce outside percent mode now returns NaN instead of 0 for all-empty input.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
df.astype(object).T leaves object-dtype columns after the transpose when mixed totals exist, so numeric formatting or to_excel downstream may treat them differently than on the non-mixed path.
There was a problem hiding this comment.
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).
| raise | ||
|
|
||
| subtotal_values = df.iloc[slice_, :] | ||
| subtotal_values = df.iloc[slice_, :].drop(columns=mixed_metric_totals) |
There was a problem hiding this comment.
drop(columns=mixed_metric_totals) raises KeyError if a name in the list is not in df.columns, and duplicate subtotal names across groups are not guarded.
There was a problem hiding this comment.
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.
|
Fixed the CI failure in 4f9a8cb after merging current master. The failing test was Validation: 640 passed, 2 xfailed across the native tool surface, chart previews, chart processing, and Excel utility suites; all changed-file pre-commit checks passed. The new CI run is pending. |
Refresh CI and Codecov checks.
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve: every round-1 finding is addressed and the blanking now matches what #44657 shipped. One non-blocking regression for operators with a custom CSV_EXPORT decimal/float_format is noted inline as a quick follow-up; the rest are nits.
🟡 Medium — Metrics-on-rows CSV ignores CSV_EXPORT decimal / float_format (inline on client_processing.py:785, non-blocking)
Default deployments are unaffected (CSV_EXPORT = {"encoding": "utf-8-sig"}), and values are correct; only the separator/format changes. Details, repro and a tested suggestion are in the inline comment.
🟢 Low — UPDATING.md entry misses a few output changes
UPDATING.md:27-34 says "scheduled CSV/text reports", but scheduled XLSX reports go through the same post-processing and blank too. It also doesn't mention two other visible changes: metrics-on-rows CSVs now write integers without .0 across all cells, and in percent modes a subtotal whose leaves are all NULL goes from 0.0% to blank, single-metric pivots included. Fix: write "scheduled CSV/Excel/text reports" and add one sentence on each change.
🟢 Low — Leftover "proposed" and time-relative wording
The collapse docstring (client_processing.py:627) still says "the browser change proposed in #44657", which has merged. The test docstring at test_client_processing.py:1922 says "its current, blanked behavior", against the repo's no-time-relative-comments rule. Fix: drop "proposed", and change "its current, blanked behavior" to "its blanked behavior".
🟢 Low — native_tool_inventory.json refresh duplicates #45511
The refresh in 4f9a8cb fixes a real break on master (#44558 changed the MCP schemas without refreshing the inventory), and it is line-for-line identical to open #45511, so the two will merge cleanly in either order. Fix: drop this commit if #45511 lands first, to keep the PR scoped to pivot exports.
Checked and fine this round: the replies to the min_count=1, drop(columns=...) and XLSX-precision comments hold up. All _reduce callers run only in percent modes, and about 9,000 configurations ran with no new exceptions versus master. The excel.py change gives identical cell output to master across about 50 frame types, except that big integers next to blanks are now exact. Large integers survive exactly through the CSV path in both layouts. Mixed totals blank for every result aggregation restored by #44657, matching the browser's mixed-metric tracker.
| 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 |
There was a problem hiding this comment.
🟡 Medium — Metrics-on-rows CSV ignores CSV_EXPORT decimal / float_format (non-blocking, quick follow-up)
DataFrame.to_csv applies decimal and float_format only to float columns. After this astype(object), every output column is object, so every float cell in the export, not only the totals, is written with .. That undoes #38616 for this layout. It triggers with metricsLayout: ROWS, 2+ metrics, column totals on, and float values (AVG metrics, Average/Median aggregation, or any "Show values as" percent mode), for both CSV downloads and scheduled CSV reports. The COLUMNS layout, single-metric pivots, Excel and JSON are unaffected, as are deployments on the default CSV_EXPORT.
Repro with CSV_EXPORT = {"sep": ";", "decimal": ","}, two AVG metrics, both totals on:
- master:
AVG(a);UK;3,5;3,5 - this branch:
AVG(a);UK;3.5;3.5
With float_format="%.2f", master writes 3.50 and this branch writes 3.5.
Casting only when an integer column actually needs exact values fixes the float case. I ran this change locally: tests/unit_tests/charts/ plus excel_tests.py give 479 passed, 2 xfailed, including the new large-integer CSV/XLSX tests and a CSV_EXPORT decimal repro.
| df = df.astype(object).T if mixed_metric_totals else df.T | |
| exact_ints = bool(mixed_metric_totals) and any( | |
| pd.api.types.is_integer_dtype(dtype) for dtype in df.dtypes | |
| ) | |
| df = df.astype(object).T if exact_ints else df.T |
Worth adding a CSV_EXPORT sep/decimal test for ROWS layout with column totals. A ROWS pivot that mixes integer and float metrics would still lose decimal; converting columns to nullable Int64/Float64 after the transpose would close that too, but a blanket convert_dtypes breaks existing tests, so that can be a separate follow-up.
mikebridge
left a comment
There was a problem hiding this comment.
Thanks @fitzee, the merge looks clean. Taking #44657 in fixes the test labels, and the UPDATING entry now sits as a bullet under Next. I re-ran the test file at 39f624d (204 passed, plus the rest of tests/unit_tests/charts/) and went through @msyavuz's three points. I see 044e2a5 has since reworded the _reduce docstring, added the invariant comment at the subtotal drop and handled exact integers in XLSX, thanks. I haven't re-run tests on that commit.
:783 (df.astype(object).T): real, but narrow, and still open at e4456b2. Once the mixed-total columns turn into object dtype, pandas stops applying decimal and float_format to the floats in them. So an operator with CSV_EXPORT = {"sep": ";", "decimal": ","} gets 6.75 instead of 6,75, but only for a mixed-metric pivot with metrics on rows and totals enabled. Master, the COLUMNS layout and single-metric pivots all still honour it, and the default config isn't affected. We already test this config for pivots in test_apply_client_processing_pivot_table_v2_custom_sep_decimal, so a mixed ROWS case there makes a good red test. One fix that keeps your 2^53 work intact: after the object transpose, convert back to numeric only the columns that contain a non-integral float, e.g.
for position in range(len(df.columns)):
column = df.iloc[:, position]
if column.map(lambda v: isinstance(v, float) and pd.notna(v) and not v.is_integer()).any():
df.isetitem(position, pd.to_numeric(column))That gave 6,75 / 17,25 with decimal=",", and 9007199254740993 stayed exact in the integer column. convert_dtypes() and infer_objects() both look tempting here, but they send the integers back through float64.
:216 and :708: agreed with your changes. All four _reduce callers are percent-mode, so min_count=1 doesn't flip any path from 0 to blank, and every name in mixed_metric_totals was inserted just above the drop. I fuzzed about 200 configs (including dimension values like Subtotal and Total (Sum), two-level columns and both layouts) and saw no failures that master doesn't also have.
One small thing while you're in there: the collapse docstring still says "the browser change proposed in #44657". It's merged now.
Happy to approve once the :783 fix and a test are in.
The master PNG-preview change grew the shared preview schema but left its recorded inventory stale. Regenerate the three affected tool entries, matching the refresh in apache#45311, so apache#45498 can exercise its unit-test gate without inherited inventory drift.
SUMMARY
Blank export/report totals and subtotals that combine distinct metrics, matching the browser behavior merged in #44657. Continues #44730 and fixes #44725.
The browser dependency has merged. This branch now includes current master and has been rechecked with the restored result-aggregation behavior.
Mixed-metric columns are excluded from totals-row reductions. Dense pivots containing only integer metrics retain their CSV spelling and precision, including with metrics on rows, Combine metrics and Transpose pivot. Sparse pivots and mixed integer/float metrics can still export floating-point values, as on master; this PR does not change that existing behavior. UPDATING.md documents the changed output and its impact on downstream consumers.
The all-empty
min_count=1behavior applies to percentage-mode rollups. Single-metric all-NULL totals in Actual Values and broader result-aggregation parity remain outside this PR.TESTING INSTRUCTIONS
The chart and Excel utility unit suites pass: 475 passed, 2 xfailed. Coverage includes Sum/Average, all four Show values as modes, both metric orientations, GROUPING SETS, combined/transposed layouts, single-metric controls and exact large-integer CSV and XLSX output. Five new XLSX regression cases fail before the Excel conversion fix and pass after it; oversized integers remain exact text and ordinary values remain numeric cells.
Changed-file pre-commit checks pass, including mypy, Ruff and Pylint. The shared rollup metric set is reused without copying or mutation, and combined/transposed subtotal coverage includes both metric layouts.
Hosted CSV/Excel and scheduled-report verification remains outstanding. The combined chart suite includes Average as a non-Sum aggregation and uses its Total (Average) labels even when Show values as is set.
ADDITIONAL INFORMATION
No database migration or feature flag. The existing single-MAX additive-total issue remains outside scope.