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 c510946df55 fix: BigQuery pivot-table Custom SQL row label with a
space drops from GROUPING SETS (#43720)
c510946df55 is described below
commit c510946df550b2aa83c8d7f1d1077c07ea62f7fc
Author: Joe Li <[email protected]>
AuthorDate: Tue Sep 8 16:58:49 2026 -0700
fix: BigQuery pivot-table Custom SQL row label with a space drops from
GROUPING SETS (#43720)
Co-authored-by: Vitor Avila <[email protected]>
---
superset/models/helpers.py | 30 ++++++++++---
tests/unit_tests/models/helpers_test.py | 74 +++++++++++++++++++++++++++++++++
2 files changed, 99 insertions(+), 5 deletions(-)
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index 06f8274025f..41caeffc8da 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -80,7 +80,6 @@ from superset import db, is_feature_enabled
from superset.advanced_data_type.types import AdvancedDataTypeResponse
from superset.common.db_query_status import QueryStatus
from superset.common.grouping_sets import (
- grouping_id_column,
grouping_marker_label,
grouping_sets_clause,
)
@@ -4867,10 +4866,31 @@ class ExploreMixin: # pylint:
disable=too-many-public-methods
and groupby_all_columns
and db_engine_spec.supports_grouping_sets
)
+ # Both the GROUPING() marker labels and the `grouping_sets` level
+ # definitions sent by the frontend (see buildQuery.ts) are expressed in
+ # terms of the column's logical/requested label (``.key``), not the
+ # engine-mutated SQL alias (``.name``). BigQuery, for example, mangles
+ # labels containing spaces (e.g. a Custom SQL column named "Test Row")
+ # into something like "Test_Row_a1b2c3" for `.name`, while `.key` keeps
+ # the original "Test Row". Keying by `.name` here would silently drop
+ # such columns from every rollup level (the `col in ...` guard below),
+ # producing an invalid ``GROUP BY GROUPING SETS`` clause that omits a
+ # selected, non-aggregated column.
+ groupby_columns_by_label = {
+ gby_expr.key: gby_expr for gby_expr in groupby_all_columns.values()
+ }
if use_grouping_sets:
+ # Route the marker through `make_sqla_column_compatible` like every
+ # other selected column: the SQL-level alias is engine-mutated if
+ # required (e.g. BigQuery rejects aliases with spaces), while
+ # `.key` keeps the unmutated marker label so it lines up with the
+ # `groupby_columns_by_label` keys above and with what the frontend
+ # looks for when splitting the combined result back per level.
select_exprs = select_exprs + [
- grouping_id_column(gby_expr, grouping_marker_label(name))
- for name, gby_expr in groupby_all_columns.items()
+ self.make_sqla_column_compatible(
+ sa.func.grouping(gby_expr), grouping_marker_label(label)
+ )
+ for label, gby_expr in groupby_columns_by_label.items()
]
# Expected output columns
@@ -4887,9 +4907,9 @@ class ExploreMixin: # pylint:
disable=too-many-public-methods
if use_grouping_sets:
gs_levels = [
[
- groupby_all_columns[col]
+ groupby_columns_by_label[col]
for col in level
- if col in groupby_all_columns
+ if col in groupby_columns_by_label
]
for level in grouping_sets or []
]
diff --git a/tests/unit_tests/models/helpers_test.py
b/tests/unit_tests/models/helpers_test.py
index bdefca9cde2..0ecfb7af7e7 100644
--- a/tests/unit_tests/models/helpers_test.py
+++ b/tests/unit_tests/models/helpers_test.py
@@ -4888,6 +4888,80 @@ def test_get_sqla_query_dotted_struct_column_bigquery(
assert "`forecasts.original`" not in sql
+def test_get_sqla_query_grouping_sets_preserves_bigquery_mutated_label(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ A pivot table with a non-additive metric issues a `grouping_sets` query
+ (see plugin-chart-pivot-table/src/plugin/buildQuery.ts), whose rollup
+ levels are expressed in terms of each column's original, unmutated label
+ (e.g. a Custom SQL row labelled "Test Row"). BigQuery mutates SQL aliases
+ that contain non-word characters, so the same column's `Label.name` ends
+ up as something like "Test_Row_<hash>". Looking up each level's columns by
+ that mutated name instead of the original label silently drops the column
+ from every rollup level, producing a `GROUP BY GROUPING SETS` clause that
+ doesn't cover a selected, non-aggregated column.
+ """
+ bigquery = pytest.importorskip("sqlalchemy_bigquery")
+
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.models.core import Database
+
+ SqlaTable.metadata.create_all(session.get_bind())
+
+ dialect = bigquery.BigQueryDialect()
+
+ @contextmanager
+ def fake_engine(*args, **kwargs):
+ engine = MagicMock()
+ engine.dialect = dialect
+ yield engine
+
+ database = Database(database_name="bq",
sqlalchemy_uri="bigquery://project")
+ mocker.patch.object(database, "get_sqla_engine", new=fake_engine)
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="orders",
+ columns=[TableColumn(column_name="amount", type="FLOAT")],
+ )
+
+ row_col: AdhocColumn = {
+ "sqlExpression": "some_column",
+ "label": "Test Row",
+ }
+ metric: AdhocMetric = {
+ "expressionType": "SIMPLE",
+ "column": {"column_name": "amount"},
+ "aggregate": "AVG",
+ "label": "avg_amount",
+ }
+
+ sqlaq = table.get_sqla_query(
+ columns=[row_col],
+ metrics=[metric],
+ extras={},
+ filter=[],
+ granularity=None,
+ is_timeseries=False,
+ grouping_sets=[["Test Row"], []],
+ )
+ sql = str(
+ sqlaq.sqla_query.compile(
+ dialect=dialect, compile_kwargs={"literal_binds": True}
+ )
+ )
+
+ # Before the fix, looking up each level's columns by the BigQuery-mutated
+ # alias (e.g. "Test_Row_<hash>") instead of the original "Test Row" label
+ # sent by the frontend silently dropped the column from every level,
+ # producing `GROUPING SETS ((), ())` -- a GROUP BY that doesn't cover the
+ # selected, non-aggregated "some_column" expression.
+ assert "GROUPING SETS((some_column), ())" in sql
+
+
def test_temporal_epoch_string_filter_is_coerced_for_bigquery() -> None:
"""
Drill-to-detail can send JavaScript timestamp strings for temporal values.
