You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/10/14 09:55:43 UTC
(superset) 02/03: fix(sqla): omit constant expressions from GROUP BY
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-literal-adhoc-column-group-by
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 7a6fefaa5b9a0daee09b23833c0d1fe1384c499c
Author: Claude Code <no...@anthropic.com>
AuthorDate: Wed Oct 14 09:32:23 2026 +0000
fix(sqla): omit constant expressions from GROUP BY
A Custom SQL dimension that is a bare constant (e.g. 'Other') was emitted
verbatim as a GROUP BY key. PostgreSQL rejects a non-integer constant there
("non-integer constant in GROUP BY"), and an integer constant is read as a
select-list ordinal.
Omit provably-constant keys (bare string/numeric/boolean literal or NULL,
parentheses stripped) from the GROUP BY of the main query, the series-limit
inner subquery and the "Others" rebuilds; they stay in SELECT under their
label. If every key is constant and the query aggregates, GROUP BY is
omitted; without an aggregate the keys are left unchanged. Expressions that
merely contain a constant (CASE, CAST, concatenation) remain keys.
Co-Authored-By: Claude Sonnet 5.5 <no...@anthropic.com>
---
superset/models/helpers.py | 80 ++++++++++-
superset/sql/parse.py | 22 +++
.../models/groupby_literal_column_test.py | 152 ++++++++++++++++++---
tests/unit_tests/sql/parse_tests.py | 30 ++++
4 files changed, 258 insertions(+), 26 deletions(-)
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index c74825c706c..6a498cf628a 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,
@@ -4243,6 +4255,46 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
return or_(*groups)
+ def _groupby_clause_exprs(
+ self, groupby_exprs: Iterable[Any], has_aggregates: bool
+ ) -> 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.
+
+ When every key is constant and the query aggregates, GROUP BY is omitted
+ altogether, yielding the single aggregate row. Against an empty input
+ that is one row of NULL aggregates rather than no rows. Without an
+ aggregate the keys are left untouched, since dropping them would turn
+ the query into a per-row select.
+ """
+ keys = list(groupby_exprs)
+ non_constant = [key for key in keys if not self._is_constant_groupby_expr(key)]
+ if len(non_constant) == len(keys) or (not non_constant and not has_aggregates):
+ return keys
+ return non_constant
+
+ 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],
@@ -5205,7 +5257,11 @@ 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(), bool(metrics_exprs)
+ )
+ )
where_clause_and: list[ColumnElement] = []
having_clause_and: list[ColumnElement] = []
@@ -5740,7 +5796,9 @@ 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, True)
+ )
ob = inner_main_metric_expr
if series_limit_metric:
@@ -5794,7 +5852,11 @@ 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(), bool(metrics_exprs)
+ )
+ )
# Re-apply WHERE and HAVING clauses lost during query reconstruction
qry = self._reapply_query_filters(
@@ -5868,7 +5930,11 @@ 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(), bool(metrics_exprs)
+ )
+ )
# 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 c8fa8064dbd..436cfc79a86 100644
--- a/superset/sql/parse.py
+++ b/superset/sql/parse.py
@@ -268,6 +268,28 @@ 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, numeric or
+ boolean literal, or ``NULL``, 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.
+ """
+ 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
+ return isinstance(
+ parsed.expressions[0].unnest(), (exp.Literal, exp.Null, exp.Boolean)
+ )
+
+
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
index 30d7bc460eb..079b5b50cdf 100644
--- a/tests/unit_tests/models/groupby_literal_column_test.py
+++ b/tests/unit_tests/models/groupby_literal_column_test.py
@@ -26,19 +26,29 @@ same literal is fine in the ``SELECT`` list.
from __future__ import annotations
+from typing import Any
+
+import pytest
import sqlglot
from sqlalchemy.orm.session import Session
from sqlglot import expressions as exp
+from superset.connectors.sqla.models import SqlaTable
-def test_literal_custom_sql_dimension_is_not_a_bare_constant_in_group_by(
- session: Session,
-) -> None:
+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"}
+
+
+@pytest.fixture
+def table(session: Session) -> SqlaTable:
from superset.connectors.sqla.models import SqlaTable, TableColumn
from superset.models.core import Database
SqlaTable.metadata.create_all(session.get_bind())
- table = SqlaTable(
+ return SqlaTable(
database=Database(
database_name="db", sqlalchemy_uri="postgresql://user:pass@host/db"
),
@@ -47,32 +57,52 @@ def test_literal_custom_sql_dimension_is_not_a_bare_constant_in_group_by(
columns=[
TableColumn(column_name="region", type="TEXT"),
TableColumn(column_name="amount", type="FLOAT"),
+ TableColumn(column_name="constant_col", type="TEXT", expression="'Other'"),
],
)
- sqla_query = table.get_sqla_query(
- columns=[
- {"label": "bucket", "sqlExpression": "'Other'", "expressionType": "SQL"},
- "region",
- ],
- metrics=[
- {"expressionType": "SQL", "sqlExpression": "SUM(amount)", "label": "total"}
- ],
- is_timeseries=False,
- row_limit=100,
- filter=[],
- extras={},
- orderby=[],
- ).sqla_query
+
+def _compile(table: SqlaTable, **overrides: Any) -> tuple[exp.Select, 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:
sql = str(
sqla_query.compile(
dialect=engine.dialect, compile_kwargs={"literal_binds": True}
)
)
-
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
@@ -91,3 +121,87 @@ def test_literal_custom_sql_dimension_is_not_a_bare_constant_in_group_by(
assert any(
isinstance(key, exp.Column) and key.name == "region" for key in group_keys
), sql
+
+
+@pytest.mark.parametrize(
+ "expression",
+ ["'Other'", "('Other')", "3", "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
+
+
+@pytest.mark.parametrize(
+ "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_omit_group_by(table: SqlaTable) -> None:
+ select, sql = _compile(table, columns=[_adhoc("'Other'")])
+ assert select.named_selects == ["bucket", "total"], sql
+ assert select.args.get("group") is None, 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
+
+
+@pytest.mark.parametrize("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
diff --git a/tests/unit_tests/sql/parse_tests.py b/tests/unit_tests/sql/parse_tests.py
index 0684d9fa53c..2d6cc690517 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,
@@ -6936,6 +6937,35 @@ def test_has_aggregate_fail_closed(expression: str, expected: bool) -> None:
assert has_aggregate(expression, fail_open=False) is expected
+@pytest.mark.parametrize(
+ "expression,engine,expected",
+ [
+ ("'Other'", "postgresql", True),
+ ("('Other')", "postgresql", True),
+ ("42", "postgresql", True),
+ ("1.5", "postgresql", True),
+ ("NULL", "postgresql", True),
+ ("TRUE", "postgresql", 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.
+ """
+ assert is_constant_expression(expression, engine) is expected
+
+
@pytest.mark.parametrize(
"engine,expected",
[