You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@superset.apache.org by "DamianPendrak (via GitHub)" <gi...@apache.org> on 2026/07/18 12:06:32 UTC

[PR] fix(sql): validate Custom SQL metric has an aggregate under GROUP BY [superset]

DamianPendrak opened a new pull request, #42199:
URL: https://github.com/apache/superset/pull/42199

   ### 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 grouped
   query that selects an expression which is neither grouped nor aggregated:
   
   ```sql
   SELECT longitude, latitude, GREATEST(confirmed, predicted) AS "..."
   FROM incidents
   WHERE longitude IS NOT NULL AND latitude IS NOT NULL
   GROUP BY longitude, latitude ...
   -- ERROR: column "confirmed" must appear in the GROUP BY clause
   --        or be used in an aggregate function
   ```
   
   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:
   ```
   The Custom SQL metric "GREATEST(confirmed, predicted)" is not an aggregate and
   can't be combined with a GROUP BY. Wrap it in an aggregate function, e.g.
   MAX(GREATEST(confirmed, predicted)).
   ```
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   After:
   <img width="1906" height="1019" alt="Screenshot 2026-07-18 at 13 21 14" src="https://github.com/user-attachments/assets/024f8804-b6b1-469c-b718-c06e373d85e0" />
   
   ### TESTING INSTRUCTIONS
   From the issue:
   1. Go to Charts → Create Chart
   2. Select dataset (e.g., incidents)
   3. Choose Deck.gl Screen Grid
   4. Choose longitude & latitude
   5. Weight - Custom SQL -> GREATEST(confirmed, predicted)
   6. Update chart
   
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue: [#38913](https://github.com/DamianPendrak/superset/issues/38913)
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(sql): validate Custom SQL metric has an aggregate under GROUP BY [superset]

Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.

netlify[bot] commented on PR #42199:
URL: https://github.com/apache/superset/pull/42199#issuecomment-5011191412

   ### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
   
   
   |  Name | Link |
   |:-:|------------------------|
   |<span aria-hidden="true">🔨</span> Latest commit | 00cb26fe54eeb96fa51688854484db893fa4a068 |
   |<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a5b6c489af4a50008d4f39b |
   |<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-42199--superset-docs-preview.netlify.app](https://deploy-preview-42199--superset-docs-preview.netlify.app) |
   |<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTQyMTk5LS1zdXBlcnNldC1kb2NzLXByZXZpZXcubmV0bGlmeS5hcHAifQ.aTu_vIUMCoopIK12TBfrzXkkP4_VYQqSl-0UtUoF7A4)<br /><br />_Use your smartphone camera to open QR code link._</details> |
   |<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctLCoAgFADAu7x1kqGGdoOO8fwWZJoZFNHdczebeSGXFHOFCaADXXA3S7Nf755JNTBirrOmSM5jI9HVshqCIRQXsLoWrMtbembbyohCj4ZLhZ6joJRKyz1TGr4f) |
   ---
   <!-- [superset-docs-preview Preview](https://deploy-preview-42199--superset-docs-preview.netlify.app) -->
   _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(sql): validate Custom SQL metric has an aggregate under GROUP BY [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.

bito-code-review[bot] commented on PR #42199:
URL: https://github.com/apache/superset/pull/42199#issuecomment-5011186750

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(sql): validate Custom SQL metric has an aggregate under GROUP BY [superset]

Posted by "DamianPendrak (via GitHub)" <gi...@apache.org>.

DamianPendrak commented on code in PR #42199:
URL: https://github.com/apache/superset/pull/42199#discussion_r3608402783


##########
superset/models/helpers.py:
##########
@@ -3787,6 +3787,36 @@ def get_sqla_query(  # pylint: disable=too-many-arguments,too-many-locals,too-ma
                 if time_filter_column is not None:
                     time_filters.append(time_filter_column)
 
+        # Gate on `groupby_all_columns` rather than the raw dimensions: it is the
+        # real GROUP BY signal and also captures the timeseries time bucket. A
+        # non-aggregate Custom SQL metric under a GROUP BY is invalid SQL, so
+        # raise a clear error instead of a raw database one (#38913). Templated
+        # expressions may render to an aggregate at runtime, so leave them alone.
+        if groupby_all_columns:
+            for metric in metrics:
+                if (
+                    utils.is_adhoc_metric(metric)
+                    and metric.get("expressionType")
+                    == utils.AdhocMetricExpressionType.SQL
+                ):
+                    expression = metric.get("sqlExpression")
+                    if (
+                        isinstance(expression, str)
+                        and "{{" not in expression
+                        and "{%" not in expression
+                        and not has_aggregate(expression, self.database.backend)

Review Comment:
   `has_aggregate()` does **not** return `False` for `my_avg(value_column)`. `my_avg` isn't a sqlglot built-in, so it parses to `exp.Anonymous`, and the helper returns `True` (fail-open) on exactly that case - the branch is present in the reviewed commit (`superset/sql/parse.py`)
   
   I don't think a change is needed here



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(sql): validate Custom SQL metric has an aggregate under GROUP BY [superset]

Posted by "DamianPendrak (via GitHub)" <gi...@apache.org>.

DamianPendrak merged PR #42199:
URL: https://github.com/apache/superset/pull/42199


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(sql): validate Custom SQL metric has an aggregate under GROUP BY [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.

bito-code-review[bot] commented on PR #42199:
URL: https://github.com/apache/superset/pull/42199#issuecomment-5011834279

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
   
   <details>
   <summary>
   Bito Usage Guide
   </summary>
   
   **Commands**
         
   Type the following command in the pull request comment and save the comment.
         
   - `/review` - Manually triggers a full AI review.
   
   - `/pause` - Pauses automatic reviews on this pull request.
   - `/resume` - Resumes automatic reviews.
   - `/resolve` - Marks all Bito-posted review comments as resolved.
   - `/abort` - Cancels all in-progress reviews.
   
         
   Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
         
   **Configuration**
   
   This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
         
   **Documentation & Help**
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
   
   </details>
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(sql): validate Custom SQL metric has an aggregate under GROUP BY [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.

codecov[bot] commented on PR #42199:
URL: https://github.com/apache/superset/pull/42199#issuecomment-5011196806

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42199?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `18.75000%` with `13 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 56.19%. Comparing base ([`cce77b4`](https://app.codecov.io/gh/apache/superset/commit/cce77b42f64d5cd835927ab55346494bc54a38ff?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`00cb26f`](https://app.codecov.io/gh/apache/superset/commit/00cb26fe54eeb96fa51688854484db893fa4a068?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 7 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/42199?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [superset/sql/parse.py](https://app.codecov.io/gh/apache/superset/pull/42199?src=pr&el=tree&filepath=superset%2Fsql%2Fparse.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc3FsL3BhcnNlLnB5) | 11.11% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [superset/models/helpers.py](https://app.codecov.io/gh/apache/superset/pull/42199?src=pr&el=tree&filepath=superset%2Fmodels%2Fhelpers.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | 28.57% | [3 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/42199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   > :exclamation:  There is a different number of reports uploaded between BASE (cce77b4) and HEAD (00cb26f). Click for more details.
   > 
   > <details><summary>HEAD has 4 uploads less than BASE</summary>
   >
   >| Flag | BASE (cce77b4) | HEAD (00cb26f) |
   >|------|------|------|
   >|python|6|2|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #42199      +/-   ##
   ==========================================
   - Coverage   65.08%   56.19%   -8.89%     
   ==========================================
     Files        2758     2758              
     Lines      155229   155244      +15     
     Branches    35572    35577       +5     
   ==========================================
   - Hits       101026    87243   -13783     
   - Misses      52288    67170   +14882     
   + Partials     1915      831    -1084     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/42199/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/42199/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.93% <12.50%> (-0.01%)` | :arrow_down: |
   | [mysql](https://app.codecov.io/gh/apache/superset/pull/42199/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [postgres](https://app.codecov.io/gh/apache/superset/pull/42199/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/42199/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.89% <18.75%> (-0.01%)` | :arrow_down: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/42199/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.95% <18.75%> (-18.16%)` | :arrow_down: |
   | [sqlite](https://app.codecov.io/gh/apache/superset/pull/42199/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/42199/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42199?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(sql): validate Custom SQL metric has an aggregate under GROUP BY [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.

codeant-ai-for-open-source[bot] commented on code in PR #42199:
URL: https://github.com/apache/superset/pull/42199#discussion_r3608334623


##########
tests/unit_tests/sql/parse_tests.py:
##########
@@ -4399,3 +4400,27 @@ def test_backtick_fallback_logs_warning(caplog: pytest.LogCaptureFixture) -> Non
         record.levelname == "WARNING" and "MySQL dialect" in record.getMessage()
         for record in caplog.records
     )
+
+
+@pytest.mark.parametrize(
+    "expression,expected",
+    [
+        ("GREATEST(confirmed, predicted)", False),
+        ("MAX(GREATEST(a, b))", True),
+        ("SUM(x)", True),
+        ("COUNT(*)", True),
+        ("SUM(x) OVER (PARTITION BY y)", False),
+        ("ROW_NUMBER() OVER ()", False),
+        ("a + b", False),
+        (")(", True),
+        ("MY_CUSTOM_AGG(x)", True),
+        ("a - (SELECT AVG(b) FROM t)", True),
+    ],
+)
+def test_has_aggregate(expression: str, expected: bool) -> None:
+    """
+    ``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).

Review 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]
   
   <details>
   <summary><b>Severity Level:</b> Minor 🧹</summary>
   
   ```mdx
   - ⚠️ Potential minor confusion for future maintainers reading tests.
   - ⚠️ Suggestion targets doc clarity; no runtime effect.
   - ⚠️ Existing behavior already explicitly encoded by parametrized cases.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   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.
   ```
   </details>
   
   [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=68377d5f3e464dbf95545a58b5b48577&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=68377d5f3e464dbf95545a58b5b48577&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   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 fix
   ```
   </details>
   <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42199&comment_hash=85d395130f402693e01818c28871f285b8843d870923c38b4be47a8aa1cbe008&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42199&comment_hash=85d395130f402693e01818c28871f285b8843d870923c38b4be47a8aa1cbe008&reaction=dislike'>👎</a>



##########
superset/models/helpers.py:
##########
@@ -3787,6 +3787,36 @@ def get_sqla_query(  # pylint: disable=too-many-arguments,too-many-locals,too-ma
                 if time_filter_column is not None:
                     time_filters.append(time_filter_column)
 
+        # Gate on `groupby_all_columns` rather than the raw dimensions: it is the
+        # real GROUP BY signal and also captures the timeseries time bucket. A
+        # non-aggregate Custom SQL metric under a GROUP BY is invalid SQL, so
+        # raise a clear error instead of a raw database one (#38913). Templated
+        # expressions may render to an aggregate at runtime, so leave them alone.
+        if groupby_all_columns:
+            for metric in metrics:
+                if (
+                    utils.is_adhoc_metric(metric)
+                    and metric.get("expressionType")
+                    == utils.AdhocMetricExpressionType.SQL
+                ):
+                    expression = metric.get("sqlExpression")
+                    if (
+                        isinstance(expression, str)
+                        and "{{" not in expression
+                        and "{%" not in expression
+                        and not has_aggregate(expression, self.database.backend)

Review Comment:
   **Suggestion:** This check will reject valid metrics that use database-specific or user-defined aggregate functions when the SQL parser does not classify them as aggregates. In those cases the query is valid, but this validation raises a form error prematurely. Consider only hard-failing when aggregate detection is certain, or augmenting detection with engine-spec aggregate allowlists. [logic error]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ❌ Custom aggregate metrics rejected despite being valid SQL.
   - ⚠️ Advanced database-specific analytics blocked through Explore UI.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Configure a SQLAlchemy datasource using a database that exposes a custom aggregate
   function, for example `my_avg()`, and register it in Superset (dataset definition lives in
   `superset/connectors/sqla/models.py` via `SqlaTable`, which ultimately calls
   `get_sqla_query()` in `superset/models/helpers.py`).
   
   2. In the Explore UI, create or edit a chart backed by that dataset so that it has at
   least one group-by dimension (ensuring `groupby_all_columns` is truthy inside
   `get_sqla_query()` at `superset/models/helpers.py:3790-3796`).
   
   3. Add an adhoc metric of type SQL (the metric object flowing into `metrics` at
   `helpers.get_sqla_query()` around lines 3795-3803) with `sqlExpression` set to
   `my_avg(value_column)` and no Jinja templating (so the checks at lines 3804-3806 pass).
   
   4. When the chart is run, `get_sqla_query()` executes the new guard: the metric passes the
   adhoc/SQL/Jinja checks, then `has_aggregate("my_avg(value_column)",
   self.database.backend)` (imported from `superset/sql/parse.py` at line 109) returns False
   because the parser only knows built-in aggregates; the condition at line 3807 becomes true
   and `QueryObjectValidationError` is raised at lines 3809-3818, blocking a query that the
   underlying database would accept and correctly aggregate.
   ```
   </details>
   
   [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0c20bf8b38cd4711abd05e82970ae0c9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0c20bf8b38cd4711abd05e82970ae0c9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/models/helpers.py
   **Line:** 3807:3807
   **Comment:**
   	*Logic Error: This check will reject valid metrics that use database-specific or user-defined aggregate functions when the SQL parser does not classify them as aggregates. In those cases the query is valid, but this validation raises a form error prematurely. Consider only hard-failing when aggregate detection is certain, or augmenting detection with engine-spec aggregate allowlists.
   
   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%2F42199&comment_hash=26beb44110ae441f38f915593ed34c319d16e7fdb83b5bfebf3cb98116559c47&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42199&comment_hash=26beb44110ae441f38f915593ed34c319d16e7fdb83b5bfebf3cb98116559c47&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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(sql): validate Custom SQL metric has an aggregate under GROUP BY [superset]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.

Copilot commented on code in PR #42199:
URL: https://github.com/apache/superset/pull/42199#discussion_r3608337122


##########
superset/sql/parse.py:
##########
@@ -161,6 +161,29 @@ def _check_script_length(script: str, engine: str | None) -> None:
 }
 
 
+def has_aggregate(expression: str, engine: str = "base") -> bool:
+    """
+    Return True if the SQL expression contains an aggregate function, ignoring
+    windowed aggregates (``SUM(x) OVER (...)``) which don't collapse rows and are
+    just as invalid under a GROUP BY as a plain column.
+
+    Deliberately permissive so a valid query is never wrongly blocked: an
+    aggregate inside a scalar subquery still counts, and it fails open (returns
+    True) on a parse error or an unmodelled function (``exp.Anonymous``) that
+    might itself be an aggregate.
+    """
+    dialect = SQLGLOT_DIALECTS.get(engine)
+    try:
+        parsed = sqlglot.parse_one(f"SELECT {expression}", dialect=dialect)
+    except Exception:
+        return True
+    if parsed.find(exp.Anonymous):
+        return True
+    return any(
+        agg.find_ancestor(exp.Window) is None for agg in parsed.find_all(exp.AggFunc)
+    )

Review Comment:
   `has_aggregate` currently ignores *any* aggregate function that has a `Window` ancestor. This will misclassify expressions like `SUM(SUM(x)) OVER ()` as having no aggregate and can incorrectly raise a validation error for grouped queries, even though the inner `SUM(x)` is a real aggregate and the SQL pattern is valid in many engines. Consider only ignoring aggregates that are directly windowed (i.e., whose immediate parent is `exp.Window`).



##########
tests/unit_tests/sql/parse_tests.py:
##########
@@ -4399,3 +4400,27 @@ def test_backtick_fallback_logs_warning(caplog: pytest.LogCaptureFixture) -> Non
         record.levelname == "WARNING" and "MySQL dialect" in record.getMessage()
         for record in caplog.records
     )
+
+
+@pytest.mark.parametrize(
+    "expression,expected",
+    [
+        ("GREATEST(confirmed, predicted)", False),
+        ("MAX(GREATEST(a, b))", True),
+        ("SUM(x)", True),
+        ("COUNT(*)", True),
+        ("SUM(x) OVER (PARTITION BY y)", False),
+        ("ROW_NUMBER() OVER ()", False),

Review Comment:
   Test coverage for `has_aggregate` is missing the nested-aggregate-in-window case (e.g. `SUM(SUM(x)) OVER ()`). With the current implementation this would be treated as non-aggregate, but it should count as having an aggregate due to the inner `SUM(x)`. Adding this case helps prevent regressions in the GROUP BY validation logic.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org