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
)
+
+
[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),
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: [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]
