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: [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]
