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
)
+
+
[email protected](
+ "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>
[](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)
[](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>
[](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)
[](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: [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]
