fix(sql): validate Custom SQL metric has an aggregate under GROUP BY#42199
fix(sql): validate Custom SQL metric has an aggregate under GROUP BY#42199DamianPendrak wants to merge 2 commits into
Conversation
A Custom SQL metric that is not an aggregate (e.g. `GREATEST(confirmed, predicted)`) produced an invalid grouped query such as `SELECT lon, lat, GREATEST(...) ... GROUP BY lon, lat`, which every database rejects. This is most visible on Deck.gl charts (e.g. a Screen Grid's "Weight"), where the spatial columns are forced into the GROUP BY. Detect the case in `get_sqla_query` -- the single choke point shared by both the modern chart-data API and the legacy viz.py/deck_multi paths -- and raise a clear `QueryObjectValidationError` telling the user to wrap the expression in an aggregate (e.g. `MAX(...)`), instead of letting a cryptic database error (or nondeterministic results on permissive MySQL) through. Custom SQL metrics own their own aggregation, so we surface guidance rather than guessing one. Adds `has_aggregate()` in `superset/sql/parse.py` using sqlglot: windowed aggregates are excluded (they don't collapse rows), and it fails open on parse errors and unknown/anonymous functions so a valid query is never wrongly blocked. Fixes apache#38913 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dosubot
Bot
added
explore:error
Code Review Agent Run #a1f309Actionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42199 +/- ##
==========================================
- Coverage 65.08% 65.08% -0.01%
==========================================
Files 2758 2758
Lines 155229 155244 +15
Branches 35572 35577 +5
==========================================
+ Hits 101026 101035 +9
- Misses 52288 52292 +4
- Partials 1915 1917 +2
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| """ | ||
| ``has_aggregate`` should detect top-level (non-windowed) aggregates and | ||
| fail open (return True) when the expression can't be parsed or uses a | ||
| function sqlglot can't model (a possible unknown aggregate). |
There was a problem hiding this comment.
Suggestion: The test documentation says the helper should detect top-level aggregates, but the parametrized cases include a non-top-level aggregate in a subquery marked as valid; this contradiction makes the contract ambiguous and can mislead future changes. Align the docstring or the test case so they describe the same behavior. [docstring mismatch]
Severity Level: Minor 🧹
- ⚠️ Potential minor confusion for future maintainers reading tests.
- ⚠️ Suggestion targets doc clarity; no runtime effect.
- ⚠️ Existing behavior already explicitly encoded by parametrized cases.Steps of Reproduction ✅
1. Open `tests/unit_tests/sql/parse_tests.py` and locate the docstring for
`test_has_aggregate` at lines 4421-4424, which states the helper should detect top-level
(non-windowed) aggregates and fail open on unparseable or unknown-function expressions.
2. Compare this description with the parametrized test cases at lines 4407-4418,
specifically the case `("a - (SELECT AVG(b) FROM t)", True)` at line 4417.
3. Note that the docstring does not explicitly discuss subquery aggregates, while the test
case treats an aggregate inside a subquery as acceptable (returning `True`), extending the
described fail-open behavior.
4. This minor ambiguity is confined to test documentation; the tests pass and there is no
conflicting behavior in the code-under-test shown in this PR, so the issue is editorial
rather than a functional bug.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/sql/parse_tests.py
**Line:** 4421:4424
**Comment:**
*Docstring Mismatch: The test documentation says the helper should detect top-level aggregates, but the parametrized cases include a non-top-level aggregate in a subquery marked as valid; this contradiction makes the contract ambiguous and can mislead future changes. Align the docstring or the test case so they describe the same behavior.
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 fixThere was a problem hiding this comment.
Pull request overview
This PR addresses a SQL correctness bug in grouped queries (notably Deck.gl Screen Grid “Weight” metrics) by validating that Custom SQL adhoc metrics include an aggregate whenever the query will produce a GROUP BY, and raising a clear QueryObjectValidationError instead of letting an invalid query reach the database.
Changes:
- Add
has_aggregate()SQL parser helper to detect aggregates while attempting to ignore window-only aggregates. - Add a guard in
SqlaTable.get_sqla_query()to reject non-aggregate Custom SQL metrics when aGROUP BYis present (including timeseries buckets), with an actionable error message. - Add unit tests covering the new validation behavior and aggregate detection.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
superset/sql/parse.py |
Adds has_aggregate() helper to detect aggregates in Custom SQL expressions. |
superset/models/helpers.py |
Adds validation to block non-aggregate Custom SQL metrics when the query is grouped. |
tests/unit_tests/sql/parse_tests.py |
Adds unit tests for has_aggregate() behavior. |
tests/unit_tests/models/helpers_test.py |
Adds unit tests ensuring grouped queries reject non-aggregate Custom SQL metrics and allow valid cases. |
Addresses the automated code-review comments on apache#42199: - `has_aggregate` ignored any aggregate with a window *ancestor*, so a real aggregate nested inside a windowed one (`SUM(SUM(x)) OVER ()`) was misclassified as non-aggregate and would have wrongly rejected a valid grouped query. It now ignores only an aggregate that is itself directly windowed (immediate parent is `exp.Window`). - Add a `SUM(SUM(x)) OVER ()` test case. - Align the `test_has_aggregate` docstring with its parametrized cases (drop the "top-level" wording that contradicted the subquery case). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #81a380Actionable 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 |
SUMMARY
Fixes #38913.
A Deck.gl Screen Grid chart whose Weight is a Custom SQL expression that
is not an aggregate - e.g.
GREATEST(confirmed, predicted)- produces a groupedquery that selects an expression which is neither grouped nor aggregated:
Rather than silently auto-wrapping in a guessed aggregate - which would invent semantics and risk
returning wrong numbers - this raises a clear, actionable validation error:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After:
TESTING INSTRUCTIONS
From the issue:
ADDITIONAL INFORMATION