Dmitry-Kucher opened a new pull request, #43421:
URL: https://github.com/apache/superset/pull/43421
### SUMMARY
Fixes #43420.
#43027 added a Sum/Average control for the Table and AG Grid Table "Show
summary" row, and applied it to every Simple (adhoc) metric unconditionally.
The control defaults to `SUM` and is `clearable: false`, so there is no way to
keep a metric's own aggregation — and a chart saved before that PR, which has
no `totals_aggregate` in its form data, is coerced into `SUM` too.
That silently changes what the summary row means:
- `COUNT_DISTINCT(col)` becomes `SUM(col)`. If `col` is not numeric the
database refuses the query outright — on Postgres with a `uuid` column,
`function sum(uuid) does not exist`. If it is numeric, the row shows the sum of
the ids and nothing signals that anything is wrong.
- The same applies to `COUNT`, `MIN` and `MAX`.
Only the summary query is affected (`queries[1]`, `columns: []`); the main
query stays correct, so a partly-broken chart is easy to miss.
#43027's rationale — that swapping the aggregate is safe because the summary
query has no `GROUP BY`, so each metric is evaluated fresh over all rows —
holds arithmetically. What it does not account for is that the swap assumes the
metric's column is summable, and that replacing the aggregate preserves the
user's intent. Neither holds for the counting aggregates.
**This PR** adds `ORIGINAL` as a third choice and makes it the default:
- `getTotalsMetrics(metrics, 'ORIGINAL')` returns the metrics untouched.
- A new `toTotalsAggregate()` narrows the raw form-data value in one place,
so anything that is not an explicit `SUM`/`AVG` — including charts saved before
the control existed — resolves to `ORIGINAL` instead of falling into `SUM`.
- Both control panels gain an "Each metric's own" option, which becomes the
default.
Sum and Average remain available exactly as designed in #43027, just as a
deliberate choice rather than something applied to every chart on upgrade.
Existing charts go back to the behaviour they had before that PR.
AG Grid's raw-mode summary columns are built from column names and have no
aggregate of their own to preserve, so they keep summing; `ORIGINAL` maps to
`SUM` on that path only.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before, a Table chart with `COUNT_DISTINCT` over a uuid column and "Show
summary" enabled:
```
Ошибка базы данных
Error: function sum(uuid) does not exist
LINE 1: SELECT sum(contract_id) AS "Кол-во", SUM(contract_amount/100... ^
HINT: No function matches the given name and argument types.
```
After, the chart renders and the summary row shows the distinct count.
### TESTING INSTRUCTIONS
Manual:
1. Table chart, Aggregate mode, one dimension, one metric
`COUNT_DISTINCT(<uuid column>)`.
2. Enable **Show summary**. On `master` the chart fails with `function
sum(uuid) does not exist`; with this change it renders, and **Summary
aggregation** shows "Each metric's own".
3. Switch **Summary aggregation** to Sum or Average — the override still
applies, as added in #43027.
4. Repeat with an AG Grid Table chart, in both Aggregate and Raw mode. Raw
mode still sums its summary columns.
Automated:
```
npx jest
packages/superset-ui-chart-controls/src/utils/getTotalsMetrics.test.ts \
plugins/plugin-chart-table/test/buildQuery.test.ts \
plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts
```
127 tests pass. New coverage: `ORIGINAL` leaves every metric type untouched;
`toTotalsAggregate` maps `SUM`/`AVG` through and everything else (including
`undefined`) to `ORIGINAL`; and both plugins keep `COUNT_DISTINCT` in the
summary query by default.
Two existing tests asserted the behaviour this PR changes — `defaults the
totals query metric aggregate to SUM` and `defaults aggregate-mode totals to
SUM for a simple metric` — and are updated to assert the metric's own aggregate
is kept.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #43420
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
Introduced by #43027 (`f7a2f0ec50`). No follow-up commit has touched
`getTotalsMetrics.ts` or the totals block in either plugin's `buildQuery.ts`,
so `master` at `f2610e9dca` still has it.
cc @rusackas as the author of #43027 — happy to take this a different way if
you would rather the override stay on by default and only be skipped for the
counting aggregates; I went with an explicit default because that is also what
makes the upgrade non-breaking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
