Repository navigation
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44730 +/- ##
==========================================
- Coverage 83.49% 82.60% -0.90%
==========================================
Files 3047 3018 -29
Lines 199523 195691 -3832
Branches 46554 44858 -1696
==========================================
- Hits 166597 161648 -4949
- Misses 29699 30819 +1120
+ Partials 3227 3224 -3
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:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #68b9b7
Actionable Suggestions - 1
-
superset/charts/client_processing.py - 1
- Duplicated metric_of closures · Line 338-338
Additional Suggestions - 1
-
tests/unit_tests/charts/test_client_processing.py - 1
-
duplicated test setup · Line 1915-1934The new test duplicates the setup and blanking assertion of `test_pivot_df_show_values_as_keeps_metrics_separate` (same `MAX(num)` df, same 2-metric pivot, same `pd.isna` on the mixed-metric total). It only adds `percent_total` mode and the grand-total-row cell. A shared helper or a merge would keep the two from diverging and masking a regression.
-
Review Details
-
Files reviewed - 2 · Commit Range:
e9ab69e..e9ab69e- superset/charts/client_processing.py
- tests/unit_tests/charts/test_client_processing.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
_apply_rollup_totals and _rollup_denominators each defined the same metric_of closure; replace both with a module-level _rollup_metric.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #99890bActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Review notes, full context in #44725 (comment). I checked this diff against Must fix before merge
Tests
Minor
Verdict: Needs changes. Items 1–3. |
|
Updated the branch. That takes care of |
|
@rusackas hey, sorry.... currently have a lot on my plate. Please go ahead, u guys can have it for sure. |
|
Continued this in #45311 after the author's handoff, preserving the original commits. I cannot push to this fork from my account. The continuation updates to current master, fixes Actual Values without GROUPING SETS (including the grand-total corner), restores set-based metric lookup, and covers both layouts across all four value modes and both data paths. Charts unit tests: 374 passed, 2 xfailed; changed-file pre-commit checks pass. The two missing Actual Values cases fail against the unfinished implementation. #45311 explicitly depends on #44657 and remains draft until that browser change lands. This PR has been left open for maintainers to decide how to consolidate the work. |
A total that collapses the metric axis -- the right-hand Total column, or the bottom Total row with metrics on rows, grand-total corner included -- mixes every configured metric, so it has no single meaningful value. The percent modes already left it blank; in Actual Values mode the additive path (every metric SUM/COUNT/MIN/MAX, so no GROUPING SETS requested) still reduced the leaf cells across metrics, exporting e.g. 14 / 26 / 40 for a SUM + MAX pivot where the chart shows nothing (apache#44657). Run every row total through `collapse()` and blank it when the block spans more than one metric, then re-blank those columns after the column totals pass, which would otherwise sum a column of blanks to 0. The currency context pass keeps its unions. Both rollup consumers now take the metric names as a set built once per call instead of a list lookup per cell. Snapshots for two-metric pivots change only in the mixed Total/Subtotal cells. The regression test covers Actual Values and all three percent modes, both metric layouts, with and without database rollups. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CodeAnt PR Risk: Low Risk
Assessed commit: |
github-actions
Bot
added
the
requires:rebase
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Requesting changes because this PR is a duplicate: the same pivot-table total fix already landed in #45311 (merged). Please close this PR rather than rebasing it; if you believe something in it is not covered by the merged change, open a new PR for that remainder only.
Fixes #44725.
Depends on #44657. Hold this until the browser fix lands: on
mastertoday the chart still fills the mixed-metric Total cells on the GROUPING SETS path, so merging this first would make exports blank cells that Explore still shows. Once #44657 is in, both paths agree._collapsed_metricpicked the last configured metric for any rollup spanning 2+ distinct metrics on the collapsed "Total" pseudo-dimension, so the report/export path (scheduled report, CSV/Excel export) could show a specific, plausible-looking value for a cell that actually mixes e.g.MAX(sales)withMEDIAN(msrp). #44657 changes the interactive chart to blank that cell instead, since combining metrics that share no unit has no single meaningful value. This brings the export/report path in line with it.Changes:
_collapsed_metricnow returnsNonewhen its input spans more than one distinct metric, instead of tie-breaking viareversed(metrics).pivot_df'scollapse()propagates that as a genuinely blank total (previously fell back to summing across every metric when the picked metric wasNone).collapse(), in Actual Values mode as in the percent modes. The additive path (all metrics SUM/COUNT/MIN/MAX, so no GROUPING SETS requested) used to reduce the leaf cells across metrics there, exporting e.g. 14 / 26 / 40 for a SUM + MAX pivot. The blanked columns are re-blanked after the column-totals pass, which would otherwise sum a column of blanks to 0 in the grand-total corner. The currency-context pass keeps its unions._reduce'ssumnow passesmin_count=1so a rollup of nothing but blanks stays blank, rather than pandas defaulting an all-NaN sum to 0. This matchessynthesizeAdditiveLevelsin the chart, which returnsnullfor an all-empty group, and is a small user-visible change on its own: a single-metric total over all-NULL cells now exports blank instead of 0._apply_rollup_totals,_rollup_denominators) take the metric names as a set built once per call instead of a list lookup per cell.Tests:
test_pivot_table_v2_blanks_every_mixed_metric_totalcovers Actual Values and all three percent modes, both metric layouts (COLUMNSandROWS, where the mixed cells are the bottom Total row), with and without database rollups; the two Actual Values additive cases fail without the fix.test_pivot_df_show_values_as_blanks_mixed_metric_grand_totalcovers the grand-total corner from the issue's repro, andtest_pivot_df_show_values_as_keeps_metrics_separatewas updated (its old assertion documented the exact pre-#44657 behavior). Existing two-metric snapshots change only in the mixed Total/Subtotal cells.tests/unit_tests/charts/: 373 passed, 2 xfailed.Not in scope (noted in review, separate issue): on the additive path in Actual Values mode a single
MAXmetric's total is still the sum of the maxima rather than the maximum.