This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 43006c1d054 fix(explore): omit constant Custom SQL dimensions from
GROUP BY (#45271)
43006c1d054 is described below
commit 43006c1d054a63830796610569576ae5bf5c2d12
Author: Joe Li <[email protected]>
AuthorDate: Thu Oct 22 15:26:00 2026 -0700
fix(explore): omit constant Custom SQL dimensions from GROUP BY (#45271)
Co-authored-by: Claude Code <[email protected]>
---
superset/models/helpers.py | 66 ++++-
superset/sql/parse.py | 27 ++
.../models/groupby_literal_column_test.py | 288 +++++++++++++++++++++
tests/unit_tests/sql/parse_tests.py | 44 ++++
4 files changed, 418 insertions(+), 7 deletions(-)
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index 0613eb934d1..12180f82775 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -26,7 +26,7 @@ import dataclasses
import logging
import re
import uuid
-from collections.abc import Hashable, Iterator
+from collections.abc import Hashable, Iterable, Iterator
from contextlib import contextmanager
from datetime import datetime, timedelta, timezone
from typing import (
@@ -72,7 +72,13 @@ from sqlalchemy.orm import (
)
from sqlalchemy.orm.session import ORMExecuteState
from sqlalchemy.sql import visitors
-from sqlalchemy.sql.elements import ColumnElement, Grouping, literal_column,
TextClause
+from sqlalchemy.sql.elements import (
+ ColumnClause,
+ ColumnElement,
+ Grouping,
+ literal_column,
+ TextClause,
+)
from sqlalchemy.sql.expression import Label, Select, TextAsFrom
from sqlalchemy.sql.selectable import Alias, TableClause
from sqlalchemy_utils import UUIDType
@@ -117,7 +123,13 @@ from superset.exceptions import (
from superset.extensions import feature_flag_manager
from superset.jinja_context import BaseTemplateProcessor
from superset.sql.metric_normalization import normalize_custom_metric
-from superset.sql.parse import has_aggregate, sanitize_clause, SQLScript,
SQLStatement
+from superset.sql.parse import (
+ has_aggregate,
+ is_constant_expression,
+ sanitize_clause,
+ SQLScript,
+ SQLStatement,
+)
from superset.superset_typing import (
AdhocColumn,
AdhocMetric,
@@ -4373,6 +4385,40 @@ class ExploreMixin: # pylint:
disable=too-many-public-methods
return or_(*groups)
+ def _groupby_clause_exprs(self, groupby_exprs: Iterable[Any]) -> list[Any]:
+ """
+ Return the expressions to emit in the GROUP BY clause.
+
+ A bare constant (e.g. a Custom SQL dimension of ``'Other'``) never
changes
+ how rows are grouped, and several engines reject it as a GROUP BY key
+ (PostgreSQL: ``non-integer constant in GROUP BY``; an integer constant
is
+ read as a select-list ordinal). Such keys are omitted here; the SELECT
+ list still carries them under their label.
+
+ Constants are only omitted while at least one other key remains. If
every
+ key is constant the keys are left untouched: whether GROUP BY can be
+ dropped then depends on the query aggregating, which is not known here.
+ """
+ keys = list(groupby_exprs)
+ non_constant = [key for key in keys if not
self._is_constant_groupby_expr(key)]
+ return non_constant or keys
+
+ def _is_constant_groupby_expr(self, expr: Any) -> bool:
+ """
+ Whether a GROUP BY key is provably a bare constant.
+
+ Only raw SQL text (``literal_column``, as built for Custom SQL and
+ calculated columns) is inspected; any other SQLAlchemy construct is
+ reported as non-constant without being compiled.
+ """
+ while isinstance(expr, (Label, Grouping)):
+ expr = expr.element
+ return (
+ isinstance(expr, ColumnClause)
+ and expr.is_literal
+ and is_constant_expression(expr.name, self.database.backend)
+ )
+
def _apply_series_others_grouping(
self,
select_exprs: list[Any],
@@ -5335,7 +5381,9 @@ class ExploreMixin: # pylint:
disable=too-many-public-methods
]
qry = qry.group_by(grouping_sets_clause(gs_levels))
else:
- qry = qry.group_by(*groupby_all_columns.values())
+ qry = qry.group_by(
+ *self._groupby_clause_exprs(groupby_all_columns.values())
+ )
where_clause_and: list[ColumnElement] = []
having_clause_and: list[ColumnElement] = []
@@ -5931,7 +5979,7 @@ class ExploreMixin: # pylint:
disable=too-many-public-methods
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))
ob = inner_main_metric_expr
if series_limit_metric:
@@ -5985,7 +6033,9 @@ class ExploreMixin: # pylint:
disable=too-many-public-methods
# Reconstruct query with modified expressions
qry = sa.select(*select_exprs)
if groupby_all_columns:
- qry = qry.group_by(*groupby_all_columns.values())
+ qry = qry.group_by(
+
*self._groupby_clause_exprs(groupby_all_columns.values())
+ )
# Re-apply WHERE and HAVING clauses lost during query
reconstruction
qry = self._reapply_query_filters(
@@ -6059,7 +6109,9 @@ class ExploreMixin: # pylint:
disable=too-many-public-methods
# Reconstruct query with modified expressions
qry = sa.select(*select_exprs)
if groupby_all_columns:
- qry = qry.group_by(*groupby_all_columns.values())
+ qry = qry.group_by(
+
*self._groupby_clause_exprs(groupby_all_columns.values())
+ )
# Re-apply WHERE and HAVING clauses lost during query
reconstruction
qry = self._reapply_query_filters(
diff --git a/superset/sql/parse.py b/superset/sql/parse.py
index 0ce5b2f23e1..f9b46ec75a8 100644
--- a/superset/sql/parse.py
+++ b/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 (including a negated number such as ``-1``), ``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 node.is_number or isinstance(node, (exp.Literal, exp.Null))
+
+
class LimitMethod(enum.Enum):
"""
Limit methods.
diff --git a/tests/unit_tests/models/groupby_literal_column_test.py
b/tests/unit_tests/models/groupby_literal_column_test.py
new file mode 100644
index 00000000000..daf0d5bb7b1
--- /dev/null
+++ b/tests/unit_tests/models/groupby_literal_column_test.py
@@ -0,0 +1,288 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+A Custom SQL dimension holding a bare string literal (``'Other'``) must not
+reach the ``GROUP BY`` clause as that bare constant.
+
+PostgreSQL and its relatives reject a non-integer constant there
+(``non-integer constant in GROUP BY``), so the chart query fails even though
the
+same literal is fine in the ``SELECT`` list.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+import pytest
+import sqlglot
+from sqlalchemy import create_engine
+from sqlalchemy.orm.session import Session
+from sqlglot import expressions as exp
+
+from superset.connectors.sqla.models import SqlaTable
+
+METRICS = [{"expressionType": "SQL", "sqlExpression": "SUM(amount)", "label":
"total"}]
+
+
+def _adhoc(sql_expression: str, label: str = "bucket") -> dict[str, str]:
+ return {"label": label, "sqlExpression": sql_expression, "expressionType":
"SQL"}
+
+
+def _make_table(session: Session, uri: str) -> SqlaTable:
+ from superset.connectors.sqla.models import SqlaTable, SqlMetric,
TableColumn
+ from superset.models.core import Database
+
+ SqlaTable.metadata.create_all(session.get_bind())
+ return SqlaTable(
+ database=Database(database_name="db", sqlalchemy_uri=uri),
+ schema=None,
+ table_name="orders",
+ columns=[
+ TableColumn(column_name="region", type="TEXT"),
+ TableColumn(column_name="amount", type="FLOAT"),
+ TableColumn(column_name="constant_col", type="TEXT",
expression="'Other'"),
+ ],
+ metrics=[SqlMetric(metric_name="constant_metric", expression="42")],
+ )
+
+
[email protected]
+def table(session: Session) -> SqlaTable:
+ return _make_table(session, "postgresql://user:pass@host/db")
+
+
+def _compile_sql(table: SqlaTable, **overrides: Any) -> str:
+ query_obj: dict[str, Any] = {
+ "metrics": METRICS,
+ "is_timeseries": False,
+ "row_limit": 100,
+ "filter": [],
+ "extras": {},
+ "orderby": [],
+ }
+ query_obj.update(overrides)
+ sqla_query = table.get_sqla_query(**query_obj).sqla_query
+ with table.database.get_sqla_engine() as engine:
+ return str(
+ sqla_query.compile(
+ dialect=engine.dialect, compile_kwargs={"literal_binds": True}
+ )
+ )
+
+
+def _compile(table: SqlaTable, **overrides: Any) -> tuple[exp.Select, str]:
+ sql = _compile_sql(table, **overrides)
+ select = sqlglot.parse_one(sql, dialect="postgres")
+ assert isinstance(select, exp.Select)
+ return select, sql
+
+
+def _group_keys(select: exp.Select) -> list[str]:
+ group = select.args.get("group")
+ return [key.sql(dialect="postgres") for key in group.expressions] if group
else []
+
+
+def _constant_keys(select: exp.Select) -> list[str]:
+ """GROUP BY keys that are a bare constant (an integer is a select
ordinal)."""
+ group = select.args.get("group")
+ return [
+ key.sql(dialect="postgres")
+ for key in (group.expressions if group else [])
+ if isinstance(key.unnest(), (exp.Literal, exp.Null, exp.Boolean))
+ ]
+
+
+def test_literal_custom_sql_dimension_is_not_a_bare_constant_in_group_by(
+ table: SqlaTable,
+) -> None:
+ select, sql = _compile(table, columns=[_adhoc("'Other'"), "region"])
+
+ # The literal dimension is still selected under its label ...
+ assert "bucket" in select.named_selects, sql
+
+ # ... but it must not be a GROUP BY key on its own: only the real dimension
+ # groups the rows.
+ group = select.args.get("group")
+ group_keys = group.expressions if group else []
+ bare_constants = [
+ key.sql(dialect="postgres")
+ for key in group_keys
+ if isinstance(key.unnest(), (exp.Literal, exp.Null, exp.Boolean))
+ and not key.unnest().is_int
+ ]
+ assert not bare_constants, f"bare constant in GROUP BY:
{bare_constants}\n{sql}"
+ assert any(
+ isinstance(key, exp.Column) and key.name == "region" for key in
group_keys
+ ), sql
+
+
[email protected](
+ "expression",
+ ["'Other'", "('Other')", "3", "1.5", "-1", "(-1.5)", "NULL", "TRUE"],
+)
+def test_constant_custom_sql_dimension_is_omitted_from_group_by(
+ table: SqlaTable, expression: str
+) -> None:
+ select, sql = _compile(table, columns=[_adhoc(expression), "region"])
+ assert select.named_selects == ["bucket", "region", "total"], sql
+ assert _group_keys(select) == ["region"], sql
+
+
+def test_constant_calculated_column_is_omitted_from_group_by(
+ table: SqlaTable,
+) -> None:
+ select, sql = _compile(table, columns=["constant_col", "region"])
+ assert "constant_col" in select.named_selects, sql
+ assert _group_keys(select) == ["region"], sql
+
+
+def test_constant_raw_string_dimension_is_omitted_from_group_by(
+ table: SqlaTable,
+) -> None:
+ select, sql = _compile(table, columns=["'Other'", "region"])
+ assert _group_keys(select) == ["region"], sql
+
+
[email protected](
+ "expression",
+ [
+ "CASE WHEN 1=1 THEN 'x' END",
+ "CAST('x' AS VARCHAR)",
+ "'a' || region",
+ "UPPER('x')",
+ ],
+)
+def test_expression_containing_a_constant_stays_a_group_by_key(
+ table: SqlaTable, expression: str
+) -> None:
+ select, sql = _compile(table, columns=[_adhoc(expression), "region"])
+ keys = _group_keys(select)
+ assert len(keys) == 2, sql
+ assert keys[-1] == "region", sql
+
+
+def test_group_by_is_unchanged_without_constant_keys(table: SqlaTable) -> None:
+ select, sql = _compile(table, columns=["region"])
+ assert _group_keys(select) == ["region"], sql
+
+
+def test_all_constant_keys_with_a_metric_keep_group_by(table: SqlaTable) ->
None:
+ select, sql = _compile(table, columns=[_adhoc("'Other'")])
+ assert _group_keys(select) == ["'Other'"], sql
+
+
[email protected]("expression", ["1", "'Other'"])
+def test_all_constant_keys_with_a_non_aggregate_metric_keep_group_by(
+ table: SqlaTable, expression: str
+) -> None:
+ select, sql = _compile(
+ table, metrics=["constant_metric"], columns=[_adhoc(expression)]
+ )
+ assert _group_keys(select) == [expression], sql
+
+
+def test_all_constant_keys_without_a_metric_keep_group_by(table: SqlaTable) ->
None:
+ select, sql = _compile(table, metrics=[], columns=[_adhoc("'Other'")])
+ assert _group_keys(select) == ["'Other'"], sql
+
+
[email protected]("group_others", [False, True])
+def test_constant_dimension_is_omitted_from_series_limit_group_by(
+ table: SqlaTable, group_others: bool
+) -> None:
+ dimensions = [_adhoc("'Other'"), "region"]
+ select, sql = _compile(
+ table,
+ columns=dimensions,
+ groupby=dimensions,
+ series_columns=["bucket", "region"],
+ series_limit=3,
+ group_others_when_limit_reached=group_others,
+ )
+ subquery = next(
+ sub for sub in select.find_all(exp.Subquery) if sub.alias ==
"series_limit"
+ )
+ inner = subquery.this
+ assert isinstance(inner, exp.Select)
+ assert _constant_keys(inner) == [], sql
+ assert _group_keys(inner) == ["region"], sql
+ assert _constant_keys(select) == [], sql
+
+
+def test_series_limit_with_non_aggregate_metric_keeps_a_real_key(
+ table: SqlaTable,
+) -> None:
+ dimensions = [_adhoc("'Other'"), "region"]
+ select, sql = _compile(
+ table,
+ metrics=["constant_metric", *METRICS],
+ columns=dimensions,
+ groupby=dimensions,
+ series_columns=["bucket", "region"],
+ series_limit=3,
+ )
+ inner = next(
+ sub for sub in select.find_all(exp.Subquery) if sub.alias ==
"series_limit"
+ ).this
+ assert isinstance(inner, exp.Select)
+ assert _group_keys(inner) == ["region"], sql
+
+
+def test_series_limit_keeps_keys_when_every_dimension_is_constant(
+ table: SqlaTable,
+) -> None:
+ dimensions = [_adhoc("'Other'")]
+ select, sql = _compile(
+ table,
+ columns=dimensions,
+ groupby=dimensions,
+ series_columns=["bucket"],
+ series_limit=3,
+ )
+ inner = next(
+ sub for sub in select.find_all(exp.Subquery) if sub.alias ==
"series_limit"
+ ).this
+ assert isinstance(inner, exp.Select)
+ assert _group_keys(inner) == ["'Other'"], sql
+ assert _group_keys(select) == ["'Other'"], sql
+
+
+def test_sqlite_boolean_keyword_dimension_resolves_to_the_column(
+ session: Session,
+) -> None:
+ """
+ SQLite has no boolean literals: ``TRUE`` is an identifier that resolves to
a
+ same-named column, so ``(TRUE)`` is a real grouping key there, not a
constant.
+ """
+ table = _make_table(session, "sqlite://")
+ sql = _compile_sql(table, columns=[_adhoc("(TRUE)", "flag"), "region"])
+
+ with create_engine("sqlite://").begin() as connection:
+ connection.exec_driver_sql(
+ 'CREATE TABLE orders (region TEXT, "TRUE" TEXT, amount FLOAT)'
+ )
+ connection.exec_driver_sql(
+ "INSERT INTO orders VALUES ('west', 'A', 2), ('west', 'B', 3), "
+ "('west', 'A', 4)"
+ )
+ rows = connection.exec_driver_sql(sql).fetchall()
+
+ assert sorted((flag, total) for flag, _region, total in rows) == [
+ ("A", 6),
+ ("B", 3),
+ ], sql
diff --git a/tests/unit_tests/sql/parse_tests.py
b/tests/unit_tests/sql/parse_tests.py
index d6a17f194f5..34edba12c3e 100644
--- a/tests/unit_tests/sql/parse_tests.py
+++ b/tests/unit_tests/sql/parse_tests.py
@@ -39,6 +39,7 @@ from superset.sql.parse import (
extract_tables_from_statement,
folds_unquoted_object_names,
has_aggregate,
+ is_constant_expression,
JinjaSQLResult,
KQLTokenType,
KustoKQLStatement,
@@ -7046,6 +7047,49 @@ def test_has_aggregate_fail_closed(expression: str,
expected: bool) -> None:
assert has_aggregate(expression, fail_open=False) is expected
[email protected](
+ "expression,engine,expected",
+ [
+ ("'Other'", "postgresql", True),
+ ("('Other')", "postgresql", True),
+ ("42", "postgresql", True),
+ ("1.5", "postgresql", True),
+ ("NULL", "postgresql", True),
+ ("TRUE", "postgresql", True),
+ ("-1", "postgresql", True),
+ ("(-1.5)", "postgresql", True),
+ ("-1", "sqlite", True),
+ ("-region", "postgresql", False),
+ ("-(1 + 1)", "postgresql", False),
+ ("- 'a'", "postgresql", False),
+ ("-TRUE", "postgresql", False),
+ ("(FALSE)", "mysql", True),
+ ("TRUE", "sqlite", False),
+ ("(FALSE)", "sqlite", False),
+ ("'Other'", "sqlite", True),
+ ("NULL", "sqlite", True),
+ ("42", "sqlite", True),
+ ('"Other"', "mysql", True),
+ ('"Other"', "postgresql", False),
+ ("region", "postgresql", False),
+ ("CASE WHEN 1=1 THEN 'x' END", "postgresql", False),
+ ("CAST('x' AS VARCHAR)", "postgresql", False),
+ ("UPPER('x')", "postgresql", False),
+ ("'a' || region", "postgresql", False),
+ ("1 + 1", "postgresql", False),
+ ("'a', 'b'", "postgresql", False),
+ (")(", "postgresql", False),
+ ],
+)
+def test_is_constant_expression(expression: str, engine: str, expected: bool)
-> None:
+ """
+ Only a bare literal or ``NULL`` (optionally parenthesized) is constant; an
+ expression that merely contains one, or can't be parsed, is not. ``TRUE``
and
+ ``FALSE`` are identifiers on SQLite, so they are not constant there.
+ """
+ assert is_constant_expression(expression, engine) is expected
+
+
@pytest.mark.parametrize(
"engine,expected",
[
