feat(table): allow choosing Sum or Average for the "Show summary" totals row - #43027
Conversation
dosubot
Bot
added
change:frontend
![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 #5a2352
Actionable Suggestions - 2
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/getTotalsMetrics.ts - 1
- Missing unit tests · Line 36-43
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts - 1
- Missing unit tests for new utility · Line 25-25
Additional Suggestions - 3
-
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx - 1
-
Missing renderTrigger on new control · Line 489-489The `totals_aggregate` control is missing `renderTrigger: true`, which is inconsistent with the equivalent control in `plugin-chart-ag-grid-table` (line 503). Without it, chart re-renders after changing the aggregate selection are deferred until the user moves focus away, degrading UX.
-
-
superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx - 1
-
Missing control panel unit tests · Line 498-522The new `totals_aggregate` control is added but `controlPanel.test.tsx` lacks coverage for it. Existing tests in `buildQuery.test.ts` verify the build-time behavior, but the control-panel visibility and default value should be exercised in unit tests as well.
-
-
tests/integration_tests/non_additive_totals_tests.py - 1
-
Missing docstring on new test · Line 205-205Add a docstring to `test_avg_totals_aggregate_matches_sum_over_count` explaining this test validates that an AVG override in the totals row produces a true row-level average (SUM/COUNT over all rows) rather than the metric's native SUM aggregation, consistent with the pattern used by `test_backend_computes_percent_column_for_summary_query` in the same file.
-
Review Details
-
Files reviewed - 9 · Commit Range:
79cec2e..79cec2e- superset-frontend/packages/superset-ui-chart-controls/src/utils/getTotalsMetrics.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts
- superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts
- superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts
- tests/integration_tests/non_additive_totals_tests.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
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- 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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43027 +/- ##
=======================================
Coverage 66.64% 66.64%
=======================================
Files 2866 2867 +1
Lines 162896 162908 +12
Branches 37525 37534 +9
=======================================
+ Hits 108568 108577 +9
- Misses 52203 52206 +3
Partials 2125 2125
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:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #e141faActionable Suggestions - 0Review 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 |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a user-facing control to choose how the Table and AG Grid Table “Show summary” totals row aggregates metrics (Sum vs Average), implemented by rewriting only Simple (adhoc) metrics for the separate totals query.
Changes:
- Introduces a new
totals_aggregatecontrol (Sum/Average) for both table plugins. - Adds shared
getTotalsMetricsutility in@superset-ui/chart-controlsand uses it when constructing totals queries. - Expands unit + integration test coverage to validate correct AVG totals behavior and pass-through for SQL/saved metrics.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration_tests/non_additive_totals_tests.py | Adds an end-to-end guard asserting AVG totals equals SUM/COUNT at row-level. |
| superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts | Adds tests for default SUM totals and AVG override behavior. |
| superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx | Adds “Summary aggregation” dropdown (visible when Show summary is enabled in agg mode). |
| superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts | Uses shared getTotalsMetrics to rewrite totals-query metrics. |
| superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts | Adds tests for aggregate-mode and raw-mode totals aggregation override. |
| superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx | Adds “Summary aggregation” dropdown for AG Grid table. |
| superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts | Applies totals_aggregate to both aggregate-mode totals and raw-mode summary columns. |
| superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts | Re-exports new getTotalsMetrics utility. |
| superset-frontend/packages/superset-ui-chart-controls/src/utils/getTotalsMetrics.ts | Implements shared logic to override Simple (adhoc) metric aggregates for totals queries. |
| superset-frontend/packages/superset-ui-chart-controls/src/utils/getTotalsMetrics.test.ts | Adds unit tests for getTotalsMetrics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #a12d31Actionable Suggestions - 0Review 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 |
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
LGTM. Backward compat is clean (default SUM reproduces prior behavior) and the totals query has no GROUP BY so AVG is a true row-level SUM/COUNT rather than re-aggregation. One nit worth considering: with AVG selected, the override is applied to every simple metric, so a COUNT/MIN/MAX metric's totals cell also becomes AVG(col), which can read as surprising — might be worth only overriding SUM-aggregated metrics, or documenting that all simple metrics are affected.
sha174n
added
the
merge-if-green
|
@sha174n thanks for the review! Same tradeoff a bot flagged elsewhere on this PR, and I landed on the same answer: guarding it properly needs column-type info |
…als row The Table and AG Grid Table chart's "Show summary" (show_totals) row always reused each metric's own aggregation, so a metric already aggregated as AVG/rate/score had no meaningful way to summarize -- a user would need a second, differently-aggregated metric hidden/relabeled just for the totals row, which doesn't work at all in Aggregate mode where metrics are already pre-aggregated. Add a `totals_aggregate` control (Sum/Average, default Sum) that overrides just the totals query's Simple (adhoc) metrics via a new shared `getTotalsMetrics` utility. The totals query has no GROUP BY, so the database evaluates the metric fresh over all rows -- swapping the aggregate for that query alone is a correct, independent computation, not a re-aggregation of already-aggregated per-row values. Fixes apache#43021 Known limitations, called out in the PR description: - Median is out of scope (needs a new backend aggregate function; no universal SQL MEDIAN, Postgres needs PERCENTILE_CONT, MySQL <8 lacks it) -- filed as a follow-up. - Custom-SQL and saved (string) metrics keep their own native aggregate in the totals row; there's no safe way to rewrite arbitrary SQL. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Clamp totals_aggregate to a known-safe value before it reaches getTotalsMetrics/adhoc metric aggregate, simplify the ag-grid-table totals-metrics branching into one variable, and guard the AVG-vs-SUM integration test against a single-row fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e633e5f to
eae7a1e
Compare
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
Today the "Show summary" (
show_totals) row in the Table chart and AG Grid Table chart always uses whatever aggregation the underlying metric already specifies (e.g. aSUM(x)metric totals as a sum). A user displaying anAVG/rate/score metric per row has no way to get a meaningful summary — they'd have to add a second, differently-aggregated metric and hide/relabel it, which doesn't work at all for Aggregate-mode metrics that are already pre-aggregated.This adds a
totals_aggregatecontrol (Sum / Average, default Sum, only visible when "Show summary" is on) that lets the user pick the totals row's aggregation independently of each metric's own aggregation. It's implemented via a new sharedgetTotalsMetricsutility (@superset-ui/chart-controls) that clones each Simple (adhoc) metric with itsaggregateswapped, used by both plugins' totals query construction.This is safe because the "Show summary" row is produced by a separate query with no
GROUP BY(columns: []) — the database evaluates each metric fresh over all rows, so swapping the aggregate for just that query is a correct, independent computation, not a re-aggregation of already-aggregated per-row values (the class of bug SIP-216 fixed for Pivot Table subtotals viaGROUPING SETS).No backend changes are required:
show_totalshas always been a frontend-only form-data key, andChartDataAdhocMetricSchemaalready acceptsAVGfor any Simple metric on any query object.Known limitations (out of scope for this PR):
MEDIAN— Postgres needsPERCENTILE_CONT(0.5) WITHIN GROUP (...), MySQL <8 lacks it entirely — so it needs new backend aggregate-function support. Will be filed as a follow-up issue linked from here.Fixes #43021
Related: #41463 / #41184 (SIP-216 — same underlying "totals row aggregation" concern, but for making totals correct for whatever aggregation a metric already uses, on Pivot Table only)
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — this PR was implemented and verified via automated tests only (see Testing instructions); no interactive browser session was available to capture screenshots.
TESTING INSTRUCTIONS
SUM-aggregated Simple metric, group by a dimension.AVG()query) — not the sum, and not a naive average of the displayed per-row values.rawSummaryColumns).Automated coverage added:
plugin-chart-table/test/buildQuery.test.ts— newTotals Aggregationdescribe block (default SUM, AVG override, SQL/saved metrics pass through unchanged).plugin-chart-ag-grid-table/test/buildQuery.test.ts— new cases for aggregate-mode and raw-mode totals aggregate override.tests/integration_tests/non_additive_totals_tests.py— newTestTableTotalsAggregateOverride, an end-to-end guard (against the realbirth_namesfixture) that an AVG-override totals query returns a trueSUM / COUNTrow-level average, not the metric's own SUM.All frontend unit tests for the two plugin packages pass locally (456/456),
tsc --buildis clean, andpre-commit runis green on the changed files. The new backend integration test could not be run in this environment (pre-existing local test-DB migration issue unrelated to this change — reproduced with pre-existing tests in the same file) and should be verified in CI.ADDITIONAL INFORMATION