EnxDev commented on code in PR #45271:
URL: https://github.com/apache/superset/pull/45271#discussion_r4328500527
##########
superset/models/helpers.py:
##########
@@ -5740,7 +5788,7 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
if _inner_filter is not None:
inner_time_filter = [_inner_filter]
subq = subq.where(and_(*(where_clause_and +
inner_time_filter)))
- subq = subq.group_by(*inner_groupby_exprs)
+ subq =
subq.group_by(*self._groupby_clause_exprs(inner_groupby_exprs))
Review Comment:
The all-constant fallback bites here in a case that isn't the disclosed one.
A line chart with a time x-axis, one dimension `'Other'` and a series limit
gets a fixed outer query (`GROUP BY ds`), but the inner subquery only groups by
series columns, so it's back to `GROUP BY 'Other'` and Postgres still rejects
the chart.
When every series column is constant there's exactly one series, so the
limit can't cut anything. Could we skip the series-limit subquery (and the
Others rebuild) in that case? A test with a non-constant x-axis plus a single
constant series column would pin it.
##########
superset/sql/parse.py:
##########
@@ -268,6 +268,33 @@ def has_aggregate(
)
+def is_constant_expression(expression: str, engine: str = "base") -> bool:
+ """
+ Return True if the SQL expression is a bare constant: a string or numeric
+ literal, ``NULL``, or (outside SQLite) a boolean literal, optionally
wrapped
+ in parentheses.
+
+ Anything that merely contains a constant (``CASE WHEN 1=1 THEN 'x' END``,
+ ``CAST('x' AS VARCHAR)``, ``'a' || col``) is not a bare constant. Fails
closed
+ (returns False) on a parse error, so an expression that can't be proven
+ constant is never treated as one.
+
+ ``TRUE``/``FALSE`` are not bare constants on SQLite: they are ordinary
+ identifiers there, resolved to a column of that name when one exists.
+ """
+ dialect = SQLGLOT_DIALECTS.get(engine)
+ try:
+ parsed = sqlglot.parse_one(f"SELECT {expression}", dialect=dialect)
+ except Exception:
+ return False
+ if not isinstance(parsed, exp.Select) or len(parsed.expressions) != 1:
+ return False
+ node = parsed.expressions[0].unnest()
+ if isinstance(node, exp.Boolean):
+ return dialect != Dialects.SQLITE
+ return isinstance(node, (exp.Literal, exp.Null))
Review Comment:
sqlglot parses `-1` and `-1.5` as `Neg(Literal)`, so they come back
non-constant and stay in GROUP BY. Postgres folds the minus into the constant,
so a `-1` sentinel dimension still fails (`GROUP BY position -1 is not in
select list`), and `-1.5` hits the same `non-integer constant` error.
`is_number` already treats a negated numeric literal as a number:
```suggestion
return node.is_number or isinstance(node, (exp.Literal, exp.Null))
```
Worth adding `-1` and `(-1.5)` to the parametrized cases.
--
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]
