kokhlo commented on PR #43421:
URL: https://github.com/apache/superset/pull/43421#issuecomment-5396486271
Thanks for picking this up — the `ORIGINAL` sentinel approach with
`toTotalsAggregate()` is the right shape, and the coverage for both plugins is
thorough. Two things I'd flag from reading the diff against current `master`:
**1. Raw-mode summary columns in the plain Table plugin may still need
attention.**
The AG Grid plugin's `rawSummaryAggregate` fallback (`ORIGINAL` → `SUM` for
raw-mode summary columns) is handled, but the plain Table plugin's summary path
in raw mode goes through a different route — historically the Table summary in
raw mode summarized `allColumns` (or `query_columns`) rather than metrics, and
I don't see that path touched in this diff. If the raw-mode summary in
plugin-chart-table also derives its aggregates through the same coercion logic,
charts in raw mode saved without `totals_aggregate` would still get coerced.
Worth a quick check that raw-mode `show_totals` in plugin-chart-table is either
unaffected or handled.
**2. Migration semantics for charts saved with an explicit
`totals_aggregate: 'SUM'`.**
The `toTotalsAggregate()` narrowing treats an explicit `'SUM'` as SUM —
correct. But charts saved *after* #43027 (between 2026-08-13 and this fix) will
carry `totals_aggregate: 'SUM'` in form data even when the user never touched
the control (the control defaulted to SUM and was `clearable: false`, so any
chart re-saved in that window carries the default baked in). For those charts
the summary row keeps overriding to SUM — the exact broken behaviour the issue
describes — and only manually switching the control to "Each metric's own"
fixes them. That's arguably correct-by-default going forward (the stored value
is what the user sees in the UI), but it means the regression isn't fully
self-healing for the mid-window cohort. A heads-up in the PR description (or a
follow-up migration) would save maintainers some confused issue comments later.
Minor: the regression-test comment in
`plugin-chart-table/test/buildQuery.test.ts` ("Overriding this to SUM sums the
counted column...") is good context — the same comment exists verbatim in the
AG Grid test file; consider keeping just one and letting the other reference
it, or leave as-is if duplication is preferred for test locality.
The rest checks out from my reading: `getTotalsMetrics` returning the input
array by reference on `ORIGINAL` (no unnecessary clones), shared util exported
from chart-controls, control descriptions rewritten to match new semantics, and
both `buildQuery.ts` call sites narrowed through `toTotalsAggregate()`
consistently.
--
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]
