Visitar URL original
fix(pivot-table): requery when totals and subtotals options change by sadpandajoe · Pull Request #45306 · apache/superset · GitHub
Skip to content

fix(pivot-table): requery when totals and subtotals options change - #45306

Merged
rusackas merged 8 commits into
masterfrom
fix-pivot-table-totals-requery
Oct 27, 2026
Merged

rusackas merged 8 commits into
masterfrom
fix-pivot-table-totals-requery

Conversation

@sadpandajoe

@sadpandajoe sadpandajoe commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

SUMMARY

With non-additive metrics (for example AVG), the Pivot Table gets its totals and subtotals from rollup levels returned by the chart query (grouping_sets, see SIP-216). Which levels are requested is derived from the Show row summaries / Show rows subtotal / Show column summaries / Show columns subtotal options. Two problems followed from that:

  1. The options did not requery. All four controls were renderTrigger, so toggling one re-rendered the stored result without a new query. The table drew Total rows/columns and the group collapse toggles, but the total cells were empty and no Subtotal lines appeared until a requery was forced. In that state, collapsing a group hid the whole group instead of collapsing it to its subtotal row.
  2. Row subtotals depended on "Show column summaries". With metrics applied on rows, the renderer places the metric pseudo-dimension first in the row hierarchy, so the fully collapsed row level is drawn as each metric group's "Subtotal" line. buildGroupbyCombinations only requested that level for "Show column summaries", so with only "Show rows subtotal" the generated query was identical to the no-options query (same cache entry) and no subtotal rows were ever fetched. The mirror case is "Show columns subtotal" with metrics applied on columns, which was gated only on "Show row summaries".

ROOT CAUSE

CHANGES

  • controlPanel.tsx: remove renderTrigger from rowTotals, rowSubTotals, colTotals and colSubTotals, and also from metricsLayout ("Apply metrics on"), transposePivot and combineMetric. Which axis the metrics land on, which dimensions form rows and columns, and whether metrics are combined all decide which rollup level the totals/subtotals options need, so changing them with an option already checked had the same stale-level problem. Toggling any of them now marks the chart as needing an update, and Run fetches the levels it needs (same treatment as showValuesAs). Charts whose metrics are all additive, or that use a result aggregation, also need Run after a change now.
  • utilities.ts: also request the collapsed metric-axis level for the metric axis's own subtotal option. It is skipped when metrics are combined (combineMetric), because no per-metric subtotal line exists there and a percent mode would otherwise carry the extra level through the combined-metrics filter and break rendering.
  • form_data_query_context.py: _pivot_grouping_sets is the declared Python mirror of the frontend level builder (used when queries are rebuilt from form data, e.g. MCP chart tools and the export fallback); it now applies the same metric-axis subtotal rule so it matches the chart.
  • Tests: updated the "row subtotals on, everything else off" pruning expectation and added tests for the control flags (all seven level-selecting controls), the requested levels with and without transposePivot, the generated query, the combined-metrics percent cases (levels unchanged, chart renders), and a parametrized Python test of _pivot_grouping_sets using the same territory/quarter cases as the plugin tests. Run against the previous code, the control, level, query and Python level tests fail.

BEFORE/AFTER SCREENSHOTS

Before After
Before — Pivot Table, column summaries checked: toggling Transpose pivot, then Run After — Pivot Table, column summaries checked: toggling Transpose pivot, then Run

Before video: https://gist.githubusercontent.com/sadpandajoe/a7524beba653265a119d951b09036f81/raw/ae1a30051a0ee0c23225b0dff5296c2021fce145/before.mp4

Before — Pivot Table, column summaries checked: toggling Transpose pivot, then Run recording

After video: https://gist.githubusercontent.com/sadpandajoe/a7524beba653265a119d951b09036f81/raw/ae1a30051a0ee0c23225b0dff5296c2021fce145/after.mp4

After — Pivot Table, column summaries checked: toggling Transpose pivot, then Run recording

Before: with column summaries checked, toggling "Transpose pivot" (or "Apply metrics on") re-rendered instantly from the stored result (⚡), leaving blank Total or Subtotal cells until a requery was forced; "Show rows subtotal" alone never showed Subtotal lines.

After: each of these controls marks the chart as needing an update; after Run the Total/Subtotal cells are filled in, "Show rows subtotal" alone shows a Subtotal line per row group, and collapsing a group leaves its subtotal row.

The recordings above were captured at an earlier commit of this branch (before combineMetric and the Python mirror were added); they cover the totals/subtotals options, "Apply metrics on" and "Transpose pivot". Browser evidence for the latest commits will be added once an environment is available.

The recordings walk through each case: rows subtotal alone, column summaries, all four options with a collapsed group, the metrics-on-columns mirror, switching "Apply metrics on", toggling "Transpose pivot", and the adjacent checks ("Show values as", combined metrics with a percent display mode, an additive-only chart, and the Median result aggregation).

TESTING INSTRUCTIONS

  1. Create a Pivot Table on a dataset with a numeric column, for example Vehicle Sales: Rows territory, Columns quarter, Metrics SUM(sales) and AVG(sales), "Apply metrics on" Rows. Run it.
  2. Check Show rows subtotal only. The chart is marked as needing an update; click Run. Each territory group now shows a Subtotal line.
  3. Uncheck it, check Show column summaries only, and Run. The Total row is filled in.
  4. Check all four options and Run; totals and subtotals are shown, and collapsing a group leaves its subtotal row.
  5. Switch "Apply metrics on" to Columns and repeat with Show columns subtotal and Show row summaries.
  6. With metrics on Columns and only Show rows subtotal checked, Run, then switch "Apply metrics on" to Rows. The chart is marked as needing an update; click Run and the Subtotal lines appear. Likewise, with only the column summaries option checked, toggle Transpose pivot and click Run: the Total row is filled in.
  7. With metrics on Rows, only Show rows subtotal checked and Combine metrics checked, Run, then uncheck Combine metrics: the chart is marked as needing an update and Run adds the per-metric Subtotal lines.
  8. cd superset-frontend && npx jest plugins/plugin-chart-pivot-table; pytest tests/unit_tests/mcp_service/chart/test_native_chart_regressions.py

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov

codecov Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.13%. Comparing base (3b70018) to head (b1b0701).
⚠️ Report is 58 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #45306      +/-   ##
==========================================
+ Coverage   84.12%   84.13%   +0.01%     
==========================================
  Files        3056     3056              
  Lines      201463   201472       +9     
  Branches    47105    47108       +3     
==========================================
+ Hits       169481   169518      +37     
+ Misses      28735    28709      -26     
+ Partials     3247     3245       -2     
Flag Coverage Δ
hive 34.87% <0.00%> (-0.01%) ⬇️
javascript 80.20% <100.00%> (+0.02%) ⬆️
mysql 52.83% <0.00%> (-0.01%) ⬇️
postgres 52.83% <0.00%> (-0.01%) ⬇️
presto 36.60% <0.00%> (-0.01%) ⬇️
python 87.36% <100.00%> (+<0.01%) ⬆️
sqlite 52.59% <0.00%> (-0.01%) ⬇️
unit 81.76% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sadpandajoe
sadpandajoe marked this pull request as ready for review October 14, 2026 22:58
@codeant-ai-for-open-source

codeant-ai-for-open-source Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

CodeAnt PR Risk: Low Risk

  • The PR appears safe to merge; the affected controls now trigger fresh queries for the rollup levels they select.
  • Tests cover control requery behavior and verify matching rollup levels in the frontend and backend.

Assessed commit: b4fcba798754

@netlify

netlify Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit b1b0701
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6ada8b279f7b716971220694
😎 Deploy Preview https://deploy-preview-45306--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions github-actions Bot added the requires:rebase Requires rebasing on top of current master label Oct 20, 2026

@rusackas rusackas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really thorough root-cause writeup, and the fix for the four toggles themselves looks right. CodeAnt's metricsLayout/transposePivot point checks out though: both are still renderTrigger: true in controlPanel.tsx, and by this PR's own root-cause analysis the renderer's collapsed-level requirement depends on which axis the metric pseudo-dimension lands on, which is exactly what metricsLayout controls. Toggling "Apply metrics on" (or transposePivot) with a totals/subtotals option already checked would hit the same stale-rollup-level bug this PR fixes for the four direct controls, just via a different trigger.

Worth dropping renderTrigger from those two as well, or saying explicitly why they're out of scope if there's a reason I'm missing. Also needs a rebase, it's showing conflicts with master right now. The two failing CI checks (test-storybook, playwright-tests) both died on infra-level apt/package issues (digest-mismatch), not this diff.

Totals and subtotals for non-additive metrics are database rollup levels
chosen from the Options toggles, but those toggles are renderTrigger
controls, so changing them re-renders the stored result without
requerying. Separately, with the metric pseudo-dimension on the metric
axis, the fully collapsed level doubles as the per-metric group subtotal,
yet buildGroupbyCombinations only requests it for the opposite total.

Add failing tests pinning both: the four toggles must not be
renderTrigger, "Show rows/columns subtotal" alone must request the
per-metric subtotal level, and it must change the generated query.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Totals and subtotals for non-additive metrics are rollup levels fetched
by the chart query, so the "Show rows/columns total" and "Show
rows/columns subtotal" options must trigger a real query instead of
re-rendering the stored result, which lacked the new levels (empty total
cells, no Subtotal lines, and collapsing a group hid it entirely).
Drop renderTrigger from the four controls.

Also request the level the renderer draws as each metric group's
Subtotal line. With the metric pseudo-dimension on an axis, the fully
collapsed level on that axis is that line, but it was only requested by
the opposite axis's total, so "Show rows subtotal" alone (metrics on
rows) or "Show columns subtotal" alone (metrics on columns) produced the
same query as no options. The new clauses are skipped when metrics are
combined, where no such line exists and a percent mode would otherwise
let the extra level through and make the renderer throw.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…metrics

The subtotal tests covered transposePivot only with combined metrics.
Add generator-level cases for non-combined metrics with transposePivot:
the metric axis follows metricsLayout in display coordinates (display
rows are groupbyColumns, display columns are groupbyRows), so the
collapsed level is requested only for the matching subtotal toggle and
the opposite toggle adds no level.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The axis the metrics land on ("Apply metrics on") and "Transpose pivot"
decide which dimensions form the rollup levels and which collapsed level
the totals/subtotals options need. As renderTrigger controls they
re-rendered the stored result, so changing either with a totals or
subtotals option already checked used levels the earlier query never
fetched (empty total cells, missing Subtotal lines) for non-additive
metrics. Drop renderTrigger from both so changing them requeries.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@sadpandajoe
sadpandajoe force-pushed the fix-pivot-table-totals-requery branch from b435fc5 to 20d2343 Compare October 20, 2026 23:06
@github-actions github-actions Bot removed the requires:rebase Requires rebasing on top of current master label Oct 21, 2026

@aminghadersohi aminghadersohi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two places that track this builder's level contract did not move with it: the Python rebuild in form_data_query_context.py, and combineMetric's renderTrigger.

@rusackas

Copy link
Copy Markdown
Member

The metricsLayout/transposePivot gap I flagged is fixed now, both have renderTrigger dropped and the tests cover them.

Leaving this to @aminghadersohi's review though, since they just caught two more spots in the same family (combineMetric and the form_data_query_context.py rebuild).

"Combine metrics" decides whether a per-metric subtotal level exists and
which metric-axis rollup levels are requested, so it changes the levels
the totals/subtotals options need. As a renderTrigger control it
re-rendered the stored result, so unchecking it with a subtotals option
already checked used a level the earlier query never fetched. Drop
renderTrigger so changing it requeries.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The form-data query builder mirrors the Pivot Table plugin's rollup-level
selection, but still gated the fully collapsed level on the metric axis
only by the opposite axis's total. The plugin now also requests that
level for the metric axis's own subtotal toggle (rowSubTotals with
metrics on rows, colSubTotals with metrics on columns), because the
renderer draws it as each metric group's Subtotal line; combined metrics
draw no such line and are excluded. Native pivot queries rebuilt from
form data therefore lacked the level for "Show rows/columns subtotal"
alone.

Port the same rule and add tests that mirror the plugin's cases
(territory x quarter, SUM and AVG), including transposePivot, the
opposite-toggle controls and combined metrics with a percent mode.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@sadpandajoe
sadpandajoe marked this pull request as draft October 21, 2026 22:11
@sadpandajoe sadpandajoe added the 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR label Oct 21, 2026
@github-actions github-actions Bot added 🎪 21013ae 🚦 building Environment 21013ae status: building 🎪 21013ae 📅 2026-10-21T23-16 Environment 21013ae created at 2026-10-21T23-16 🎪 21013ae 🤡 sadpandajoe and removed 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR labels Oct 21, 2026
@github-actions github-actions Bot added 🎪 21013ae 🚦 failed and removed 🎪 21013ae 📅 2026-10-21T23-16 Environment 21013ae created at 2026-10-21T23-16 labels Oct 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🎪 Showtime is building environment on GHA for b4fcba7

@github-actions github-actions Bot added 🎪 b4fcba7 🚦 deploying Environment b4fcba7 status: deploying 🎪 b4fcba7 🚦 failed and removed 🎪 b4fcba7 🚦 building Environment b4fcba7 status: building 🎪 b4fcba7 🚦 deploying Environment b4fcba7 status: deploying labels Oct 22, 2026
@sadpandajoe

Copy link
Copy Markdown
Member Author

Thanks for the careful review. Done: renderTrigger is removed from metricsLayout and transposePivot in 20d2343, and from combineMetric in d9c6268 (another review asked for that one too); the control test now covers all seven level-selecting controls. The branch was rebased onto master after your review and has since been brought up to date, and the checks you mentioned were infra failures: the required checks are green on the current head.

@sadpandajoe
sadpandajoe marked this pull request as ready for review October 22, 2026 21:38

@aminghadersohi aminghadersohi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_pivot_grouping_sets now applies the plugin's metric-axis subtotal rule, and combineMetric requeries when toggled, so both builders request the same rollup levels.

@rusackas
rusackas merged commit 037abb7 into master Oct 27, 2026
85 of 86 checks passed
@rusackas
rusackas deleted the fix-pivot-table-totals-requery branch October 27, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants