codeant-ai-for-open-source[bot] commented on code in PR #43027: URL: https://github.com/apache/superset/pull/43027#discussion_r3761241524
########## superset-frontend/packages/superset-ui-chart-controls/src/utils/getTotalsMetrics.ts: ########## @@ -0,0 +1,43 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { isAdhocMetricSimple, QueryFormMetric } from '@superset-ui/core'; + +export type TotalsAggregate = 'SUM' | 'AVG'; + +/** + * Build the metrics for a chart's "Show summary" totals query, overriding + * each Simple (adhoc) metric's aggregate function with the user-chosen + * totals aggregate. The totals query has no GROUP BY, so the database + * evaluates each metric fresh over all rows -- swapping the aggregate here + * is a correct, independent computation, not a re-aggregation of + * already-aggregated per-row values. + * + * Custom-SQL metrics and saved (string) metrics pass through unchanged: + * there is no safe way to rewrite an arbitrary SQL expression's aggregate + * function without parsing it, so the totals row keeps their own native + * aggregate for those. + */ +export function getTotalsMetrics( + metrics: QueryFormMetric[], + aggregate: TotalsAggregate, +): QueryFormMetric[] { + return metrics.map(metric => + isAdhocMetricSimple(metric) ? { ...metric, aggregate } : metric, + ); Review Comment: **Suggestion:** Applying `AVG` to every SIMPLE metric can make an otherwise valid chart query fail. SIMPLE metrics may aggregate string columns with `MAX` or `MIN`, and selecting Average rewrites those into `AVG(string_column)`, which is rejected by databases such as PostgreSQL. Restrict the override to compatible numeric columns/metrics or preserve the original aggregate when Average is not valid. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Summary queries fail for charts containing valid text-compatible SIMPLE metrics. - ⚠️ Table and AG Grid charts can lose totals when Average is selected. - ⚠️ Mixed numeric and text metric charts are especially affected. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bde379aa2ec3481bbe2941ede90ad1d7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bde379aa2ec3481bbe2941ede90ad1d7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/packages/superset-ui-chart-controls/src/utils/getTotalsMetrics.ts **Line:** 40:42 **Comment:** *Type Error: Applying `AVG` to every SIMPLE metric can make an otherwise valid chart query fail. SIMPLE metrics may aggregate string columns with `MAX` or `MIN`, and selecting Average rewrites those into `AVG(string_column)`, which is rejected by databases such as PostgreSQL. Restrict the override to compatible numeric columns/metrics or preserve the original aggregate when Average is not valid. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43027&comment_hash=2447b29e845afed29fbb775095e77b0bfb757e1c7509d1f8dcedaa8c48312963&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43027&comment_hash=2447b29e845afed29fbb775095e77b0bfb757e1c7509d1f8dcedaa8c48312963&reaction=dislike'>👎</a> ########## tests/integration_tests/non_additive_totals_tests.py: ########## @@ -189,6 +189,48 @@ 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] Review Comment: **Suggestion:** This test does not exercise the new totals override: it sends `avg_metric` with `aggregate: "AVG"` directly in the query and never sets `show_totals`, `totals_aggregate`, or invokes the frontend query builder. It would pass even if `getTotalsMetrics` and both chart build-query integrations were completely absent, so it cannot detect a regression in the feature it claims to cover. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ The named integration test cannot detect regressions in the new override wiring. - ⚠️ Backend AVG behavior may remain green while Table totals construction breaks. - ⚠️ The integration test provides no end-to-end coverage for the user-facing control. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=035ae4f887cf42ceba4de9253dd28957&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=035ae4f887cf42ceba4de9253dd28957&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/integration_tests/non_additive_totals_tests.py **Line:** 220:224 **Comment:** *Incomplete Implementation: This test does not exercise the new totals override: it sends `avg_metric` with `aggregate: "AVG"` directly in the query and never sets `show_totals`, `totals_aggregate`, or invokes the frontend query builder. It would pass even if `getTotalsMetrics` and both chart build-query integrations were completely absent, so it cannot detect a regression in the feature it claims to cover. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43027&comment_hash=42eafb08807168d11bebbed142fe3ec27d5824a229a8abd141a7fcf95b5c4df2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43027&comment_hash=42eafb08807168d11bebbed142fe3ec27d5824a229a8abd141a7fcf95b5c4df2&reaction=dislike'>👎</a> -- 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]
