Copilot commented on code in PR #43027:
URL: https://github.com/apache/superset/pull/43027#discussion_r3760265732
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -694,6 +695,7 @@ export const buildQueryUncached:
BuildQuery<TableChartFormData> = (
formData.show_totals &&
queryMode === QueryMode.Aggregate,
);
+ const totalsAggregate = formData.totals_aggregate ?? 'SUM';
Review Comment:
Similar to the table plugin, `totals_aggregate` should be normalized to a
known-safe value before being used as an adhoc metric aggregate (both in
raw-mode `rawSummaryColumns` and aggregate-mode `getTotalsMetrics`). Suggest
constraining to `'AVG' | 'SUM'` at runtime (e.g., treat any unexpected value as
`'SUM'`) to avoid emitting invalid query objects.
##########
superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:
##########
@@ -350,6 +351,7 @@ export const buildQuery: BuildQuery<TableChartFormData> = (
extraQueries.push({
...queryObject,
columns: [],
+ metrics: getTotalsMetrics(metrics, formData.totals_aggregate ?? 'SUM'),
Review Comment:
`formData.totals_aggregate` is ultimately user-controlled persisted state;
if it ever contains an unexpected value (e.g., from older/hand-edited form
data), it will be forwarded into adhoc metric `aggregate` and could generate
invalid SQL or backend validation errors. Consider sanitizing to the supported
set (e.g., map anything other than 'AVG' to 'SUM') before calling
`getTotalsMetrics`.
##########
tests/integration_tests/non_additive_totals_tests.py:
##########
@@ -189,6 +189,44 @@ def
test_backend_computes_percent_column_for_summary_query(self):
assert totals_df["sum__num_pct"].iloc[0] == pytest.approx(1.0)
[email protected]("load_birth_names_dashboard_with_slices")
+class TestTableTotalsAggregateOverride(SupersetTestCase):
+ """
+ #43021: "Show summary" lets a user choose the totals row's aggregation
+ (Sum or Average) independently of each metric's own aggregation. The
+ frontend (``getTotalsMetrics``) implements this by cloning a SIMPLE
+ metric with its ``aggregate`` swapped for just the totals query. Since
+ that query has no GROUP BY, the database evaluates the swapped
+ aggregate fresh over every row -- this guard pins that an AVG override
+ is a true row-level average (SUM / COUNT over all rows), not the
+ metric's own SUM aggregation and not a naive average of per-group sums.
+ """
+
+ def test_avg_totals_aggregate_matches_sum_over_count(self):
+ self.login("admin")
+
+ sum_metric = {
+ "expressionType": "SIMPLE",
+ "column": {"column_name": "num"},
+ "aggregate": "SUM",
+ "label": "sum__num",
+ }
+ count_metric = {
+ "expressionType": "SIMPLE",
+ "column": {"column_name": "num"},
+ "aggregate": "COUNT",
+ "label": "count__num",
+ }
+ avg_metric = {**sum_metric, "aggregate": "AVG", "label": "avg__num"}
+
+ total_sum = _result_df(_base_payload(sum_metric,
[]))["sum__num"].iloc[0]
+ total_count = _result_df(_base_payload(count_metric,
[]))["count__num"].iloc[0]
+ avg_total = _result_df(_base_payload(avg_metric,
[]))["avg__num"].iloc[0]
+
+ assert avg_total == pytest.approx(total_sum / total_count)
+ assert avg_total != pytest.approx(total_sum)
Review Comment:
The second assertion (`avg_total != approx(total_sum)`) is only guaranteed
if `total_count != 1`. To prevent the test from becoming flaky if the
fixture/filtering changes over time, consider asserting an invariant like
`total_count > 1` (or selecting filters/columns that guarantee multiple rows)
before asserting `avg_total != total_sum`.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -728,14 +730,18 @@ export const buildQueryUncached:
BuildQuery<TableChartFormData> = (
extraQueries.push({
...queryObject,
columns: [],
- ...(rawSummaryColumns.length > 0 && {
- metrics: rawSummaryColumns.map(columnName => ({
- expressionType: 'SIMPLE' as const,
- aggregate: 'SUM' as const,
- column: { column_name: columnName },
- label: columnName,
- })),
- }),
+ ...(rawSummaryColumns.length > 0
+ ? {
+ metrics: rawSummaryColumns.map(columnName => ({
+ expressionType: 'SIMPLE' as const,
+ aggregate: totalsAggregate,
+ column: { column_name: columnName },
+ label: columnName,
+ })),
+ }
+ : showAggregateTotals
+ ? { metrics: getTotalsMetrics(metrics ?? [], totalsAggregate) }
+ : {}),
Review Comment:
This nested conditional spread is hard to read and increases the chance of
subtle branching mistakes as the totals logic evolves. Consider computing a
single `totalsMetrics` variable earlier (based on `rawSummaryColumns` vs
`showAggregateTotals`) and then conditionally attaching `metrics` once, which
will make the query construction and future edits clearer.
--
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]
