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:41 UTC
(superset) branch fix-literal-adhoc-column-group-by created (now 5597b3eefb9)
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a change to branch fix-literal-adhoc-column-group-by
in repository https://gitbox.apache.org/repos/asf/superset.git
at 5597b3eefb9 fix(sqla): keep GROUP BY when every key is constant; treat TRUE/FALSE as identifiers on SQLite
This branch includes the following new commits:
new b52e6e5c540 test(sqla): add failing guard for literal Custom SQL column in GROUP BY
new 7a6fefaa5b9 fix(sqla): omit constant expressions from GROUP BY
new 5597b3eefb9 fix(sqla): keep GROUP BY when every key is constant; treat TRUE/FALSE as identifiers on SQLite
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 02/03: fix(sqla): omit constant expressions from GROUP BY
Posted by jl...@apache.org.
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",
[
(superset) 01/03: test(sqla): add failing guard for literal Custom SQL column in GROUP BY
Posted by jl...@apache.org.
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 b52e6e5c540ae28adc0ed8f1b899e50547133433
Author: Claude Code <no...@anthropic.com>
AuthorDate: Wed Oct 14 09:20:12 2026 +0000
test(sqla): add failing guard for literal Custom SQL column in GROUP BY
A Custom SQL dimension holding a bare string literal is emitted verbatim as
a GROUP BY key, which PostgreSQL rejects with "non-integer constant in
GROUP BY". Pin the compiled PostgreSQL GROUP BY so the constant is not a
group key while the labelled column stays in SELECT.
Co-Authored-By: Claude Sonnet 5.5 <no...@anthropic.com>
---
.../models/groupby_literal_column_test.py | 93 ++++++++++++++++++++++
1 file changed, 93 insertions(+)
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..30d7bc460eb
--- /dev/null
+++ b/tests/unit_tests/models/groupby_literal_column_test.py
@@ -0,0 +1,93 @@
+# 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
+
+import sqlglot
+from sqlalchemy.orm.session import Session
+from sqlglot import expressions as exp
+
+
+def test_literal_custom_sql_dimension_is_not_a_bare_constant_in_group_by(
+ session: Session,
+) -> None:
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.models.core import Database
+
+ SqlaTable.metadata.create_all(session.get_bind())
+ table = SqlaTable(
+ database=Database(
+ database_name="db", sqlalchemy_uri="postgresql://user:pass@host/db"
+ ),
+ schema=None,
+ table_name="orders",
+ columns=[
+ TableColumn(column_name="region", type="TEXT"),
+ TableColumn(column_name="amount", type="FLOAT"),
+ ],
+ )
+
+ 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
+ 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)
+
+ # 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
(superset) 03/03: fix(sqla): keep GROUP BY when every key is constant; treat TRUE/FALSE as identifiers on SQLite
Posted by jl...@apache.org.
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 5597b3eefb988233500982b78323bacac0f24d24
Author: Claude Code <no...@anthropic.com>
AuthorDate: Wed Oct 14 09:46:39 2026 +0000
fix(sqla): keep GROUP BY when every key is constant; treat TRUE/FALSE as identifiers on SQLite
Only omit constant GROUP BY keys while a non-constant key remains. When
every key is constant the keys are left unchanged, since whether GROUP BY
can be dropped depends on the query aggregating, which is not known there.
On SQLite TRUE and FALSE are identifiers that resolve to a same-named
column, so they are not bare constants: dropping (TRUE) as a key would
merge groups. Boolean literals stay constants on other engines.
Co-Authored-By: Claude Sonnet 5.5 <no...@anthropic.com>
---
superset/models/helpers.py | 32 ++-----
superset/sql/parse.py | 15 ++-
.../models/groupby_literal_column_test.py | 103 ++++++++++++++++++---
tests/unit_tests/sql/parse_tests.py | 9 +-
4 files changed, 119 insertions(+), 40 deletions(-)
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index 6a498cf628a..bc4d6a8441a 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -4255,9 +4255,7 @@ 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]:
+ def _groupby_clause_exprs(self, groupby_exprs: Iterable[Any]) -> list[Any]:
"""
Return the expressions to emit in the GROUP BY clause.
@@ -4267,17 +4265,13 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
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.
+ 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)]
- if len(non_constant) == len(keys) or (not non_constant and not has_aggregates):
- return keys
- return non_constant
+ return non_constant or keys
def _is_constant_groupby_expr(self, expr: Any) -> bool:
"""
@@ -5258,9 +5252,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
qry = qry.group_by(grouping_sets_clause(gs_levels))
else:
qry = qry.group_by(
- *self._groupby_clause_exprs(
- groupby_all_columns.values(), bool(metrics_exprs)
- )
+ *self._groupby_clause_exprs(groupby_all_columns.values())
)
where_clause_and: list[ColumnElement] = []
@@ -5796,9 +5788,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(
- *self._groupby_clause_exprs(inner_groupby_exprs, True)
- )
+ subq = subq.group_by(*self._groupby_clause_exprs(inner_groupby_exprs))
ob = inner_main_metric_expr
if series_limit_metric:
@@ -5853,9 +5843,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
qry = sa.select(*select_exprs)
if groupby_all_columns:
qry = qry.group_by(
- *self._groupby_clause_exprs(
- groupby_all_columns.values(), bool(metrics_exprs)
- )
+ *self._groupby_clause_exprs(groupby_all_columns.values())
)
# Re-apply WHERE and HAVING clauses lost during query reconstruction
@@ -5931,9 +5919,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
qry = sa.select(*select_exprs)
if groupby_all_columns:
qry = qry.group_by(
- *self._groupby_clause_exprs(
- groupby_all_columns.values(), bool(metrics_exprs)
- )
+ *self._groupby_clause_exprs(groupby_all_columns.values())
)
# Re-apply WHERE and HAVING clauses lost during query reconstruction
diff --git a/superset/sql/parse.py b/superset/sql/parse.py
index 436cfc79a86..a29f84398f8 100644
--- a/superset/sql/parse.py
+++ b/superset/sql/parse.py
@@ -270,13 +270,17 @@ 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.
+ 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:
@@ -285,9 +289,10 @@ def is_constant_expression(expression: str, engine: str = "base") -> bool:
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)
- )
+ node = parsed.expressions[0].unnest()
+ if isinstance(node, exp.Boolean):
+ return dialect != Dialects.SQLITE
+ return isinstance(node, (exp.Literal, exp.Null))
class LimitMethod(enum.Enum):
diff --git a/tests/unit_tests/models/groupby_literal_column_test.py b/tests/unit_tests/models/groupby_literal_column_test.py
index 079b5b50cdf..7a0ee5655af 100644
--- a/tests/unit_tests/models/groupby_literal_column_test.py
+++ b/tests/unit_tests/models/groupby_literal_column_test.py
@@ -30,6 +30,7 @@ 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
@@ -42,16 +43,13 @@ 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
+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="postgresql://user:pass@host/db"
- ),
+ database=Database(database_name="db", sqlalchemy_uri=uri),
schema=None,
table_name="orders",
columns=[
@@ -59,10 +57,16 @@ def table(session: Session) -> SqlaTable:
TableColumn(column_name="amount", type="FLOAT"),
TableColumn(column_name="constant_col", type="TEXT", expression="'Other'"),
],
+ metrics=[SqlMetric(metric_name="constant_metric", expression="42")],
)
-def _compile(table: SqlaTable, **overrides: Any) -> tuple[exp.Select, str]:
+@pytest.fixture
+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,
@@ -74,11 +78,15 @@ def _compile(table: SqlaTable, **overrides: Any) -> tuple[exp.Select, str]:
query_obj.update(overrides)
sqla_query = table.get_sqla_query(**query_obj).sqla_query
with table.database.get_sqla_engine() as engine:
- sql = str(
+ 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
@@ -173,10 +181,19 @@ def test_group_by_is_unchanged_without_constant_keys(table: SqlaTable) -> None:
assert _group_keys(select) == ["region"], sql
-def test_all_constant_keys_with_a_metric_omit_group_by(table: SqlaTable) -> None:
+def test_all_constant_keys_with_a_metric_keep_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
+ assert _group_keys(select) == ["'Other'"], sql
+
+
+@pytest.mark.parametrize("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:
@@ -205,3 +222,67 @@ def test_constant_dimension_is_omitted_from_series_limit_group_by(
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 2d6cc690517..221c3b55ddb 100644
--- a/tests/unit_tests/sql/parse_tests.py
+++ b/tests/unit_tests/sql/parse_tests.py
@@ -6946,6 +6946,12 @@ def test_has_aggregate_fail_closed(expression: str, expected: bool) -> None:
("1.5", "postgresql", True),
("NULL", "postgresql", True),
("TRUE", "postgresql", True),
+ ("(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),
@@ -6961,7 +6967,8 @@ def test_has_aggregate_fail_closed(expression: str, expected: bool) -> None:
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.
+ 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