You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/07/11 02:06:07 UTC
(superset) 01/01: fix(trino): quote nested ROW field paths per-segment when building physical column expressions
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix/issue-27034-superset-fails-to-use-trino-s-nested-fie
in repository https://gitbox.apache.org/repos/asf/superset.git
commit ab0bbdc615363457bdd875fa9295288982c00a9a
Author: Evan <ev...@preset.io>
AuthorDate: Fri Jul 10 19:04:39 2026 -0700
fix(trino): quote nested ROW field paths per-segment when building physical column expressions
When `expand_rows` is enabled, Trino nested `ROW` fields (e.g. `metadata.uuid`)
are expanded into dotted-name dataset columns, but `fetch_metadata` never set
an `expression` for the new `TableColumn`, so SQLAlchemy quoted the whole
dotted name as a single identifier (`"metadata.uuid"`), which Trino rejects.
This uses the per-segment quoted expression already computed by
`TrinoEngineSpec._expand_columns` (`"metadata"."uuid"`) so charts, filters,
and dashboards can reference these columns without falling back to Custom SQL.
Fixes #27034
---
superset/connectors/sqla/models.py | 9 +++-
superset/db_engine_specs/trino.py | 7 ++-
superset/superset_typing.py | 6 +++
tests/unit_tests/connectors/sqla/models_test.py | 66 +++++++++++++++++++++++++
tests/unit_tests/db_engine_specs/test_trino.py | 5 ++
5 files changed, 91 insertions(+), 2 deletions(-)
diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py
index ec5a12eb926..27c4a8eb7db 100644
--- a/superset/connectors/sqla/models.py
+++ b/superset/connectors/sqla/models.py
@@ -2005,6 +2005,11 @@ class SqlaTable(
columns = []
for col in new_columns:
old_column = old_columns_by_name.pop(col["column_name"], None)
+ # Some engine specs (e.g. Trino, when expanding nested `ROW` columns)
+ # provide an explicit SQL expression that must be used to select the
+ # physical column, since simply quoting the dotted `column_name` as a
+ # single identifier is not valid syntax for these nested fields.
+ expression = col.get("expression") or ""
if not old_column:
results.added.append(col["column_name"])
new_column = TableColumn(
@@ -2017,12 +2022,14 @@ class SqlaTable(
if col.get("comment"):
new_column.description = col["comment"]
db_engine_spec.alter_new_orm_column(new_column)
+ if expression:
+ new_column.expression = expression
else:
new_column = old_column
if new_column.type != col["type"]:
results.modified.append(col["column_name"])
new_column.type = col["type"]
- new_column.expression = ""
+ new_column.expression = expression
# Set description from comment field if available
if col.get("comment"):
new_column.description = col["comment"]
diff --git a/superset/db_engine_specs/trino.py b/superset/db_engine_specs/trino.py
index 0747a8ce4a2..906db82eba0 100644
--- a/superset/db_engine_specs/trino.py
+++ b/superset/db_engine_specs/trino.py
@@ -602,7 +602,11 @@ class TrinoEngineSpec(PrestoBaseEngineSpec):
Expanded columns are named foo.bar.baz and we provide a query_as property to
instruct the base engine spec how to correctly query them: instead of quoting
the whole string they have to be quoted like "foo"."bar"."baz" and we then
- alias them to the full dotted string for ease of reference.
+ alias them to the full dotted string for ease of reference. We also provide
+ an `expression` property with the same correctly quoted path (without the
+ alias), so that a physical `TableColumn` created from this metadata selects
+ the column correctly instead of quoting the whole dotted name as a single
+ (invalid) identifier.
"""
# pylint: disable=import-outside-toplevel
from trino.sqlalchemy import datatype
@@ -625,6 +629,7 @@ class TrinoEngineSpec(PrestoBaseEngineSpec):
column_name=name,
type=inner_type,
is_dttm=is_dttm,
+ expression=query_name,
query_as=f'{query_name} AS "{name}"',
)
cols.extend(cls._expand_columns(inner_col))
diff --git a/superset/superset_typing.py b/superset/superset_typing.py
index 8081fc4675b..3b32d8c549c 100644
--- a/superset/superset_typing.py
+++ b/superset/superset_typing.py
@@ -132,6 +132,12 @@ class ResultSetColumnType(TypedDict):
query_as: NotRequired[Any]
+ # A SQL expression (without an alias) that should be used to select this
+ # column, e.g. for a nested field whose path segments must each be quoted
+ # separately rather than the whole dotted name being quoted as a single
+ # identifier.
+ expression: NotRequired[Any]
+
CacheConfig: TypeAlias = dict[str, Any]
DbapiDescriptionRow: TypeAlias = tuple[
diff --git a/tests/unit_tests/connectors/sqla/models_test.py b/tests/unit_tests/connectors/sqla/models_test.py
index 2909bf4f308..db023211c8c 100644
--- a/tests/unit_tests/connectors/sqla/models_test.py
+++ b/tests/unit_tests/connectors/sqla/models_test.py
@@ -542,6 +542,72 @@ def test_fetch_metadata_with_comment_field_existing_columns(
assert columns_by_name["name"].description == "Updated name description"
+def test_fetch_metadata_sets_expression_for_expanded_nested_columns(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that fetch_metadata uses the `expression` hint provided by the db engine
+ spec (e.g. Trino's expansion of nested `ROW` columns via `expand_rows`) to set
+ the physical `TableColumn.expression`.
+
+ Without this, a nested column like `metadata.uuid` would have no expression,
+ causing SQLAlchemy to render the whole dotted `column_name` as a single quoted
+ identifier (`"metadata.uuid"`), which Trino rejects, instead of the correct
+ per-segment quoting (`"metadata"."uuid"`).
+
+ See: https://github.com/apache/superset/issues/27034
+ """
+ # Mock database
+ database = mocker.MagicMock()
+ database.get_metrics.return_value = []
+
+ # Mock db_engine_spec
+ mock_db_engine_spec = mocker.MagicMock()
+ mock_db_engine_spec.alter_new_orm_column = mocker.MagicMock()
+ database.db_engine_spec = mock_db_engine_spec
+
+ # Create table with a pre-existing (already synced) expanded column, to also
+ # cover the "sync columns from source" (re-fetch) code path
+ table = SqlaTable(table_name="test_table_nested", database=database)
+ table.id = 1
+
+ existing_col = TableColumn(
+ column_name="metadata.uuid",
+ type="VARCHAR",
+ table=table,
+ expression="",
+ )
+ table.columns = [existing_col]
+
+ mock_columns = [
+ {
+ "column_name": "metadata",
+ "type": "ROW",
+ },
+ {
+ "column_name": "metadata.uuid",
+ "type": "VARCHAR",
+ "expression": '"metadata"."uuid"',
+ "query_as": '"metadata"."uuid" AS "metadata.uuid"',
+ },
+ ]
+
+ mock_session = mocker.patch("superset.connectors.sqla.models.db.session")
+ mock_session.query.return_value.filter.return_value.all.return_value = [
+ existing_col
+ ]
+ mocker.patch.object(table, "external_metadata", return_value=mock_columns)
+ mocker.patch(
+ "superset.connectors.sqla.models.config", {"SQLA_TABLE_MUTATOR": lambda x: None}
+ )
+
+ table.fetch_metadata()
+
+ columns_by_name = {col.column_name: col for col in table.columns}
+ assert not columns_by_name["metadata"].expression
+ assert columns_by_name["metadata.uuid"].expression == '"metadata"."uuid"'
+
+
def test_fetch_metadata_mixed_comment_scenarios(mocker: MockerFixture) -> None:
"""Test fetch_metadata with mix of new/existing columns and with/without
comments
diff --git a/tests/unit_tests/db_engine_specs/test_trino.py b/tests/unit_tests/db_engine_specs/test_trino.py
index fcce320208e..0462e6f9e89 100644
--- a/tests/unit_tests/db_engine_specs/test_trino.py
+++ b/tests/unit_tests/db_engine_specs/test_trino.py
@@ -758,6 +758,7 @@ def test_get_columns_expand_rows(mocker: MockerFixture):
column_name="field1.a",
type=types.VARCHAR(),
is_dttm=False,
+ expression='"field1"."a"',
query_as='"field1"."a" AS "field1.a"',
),
ResultSetColumnType(
@@ -765,6 +766,7 @@ def test_get_columns_expand_rows(mocker: MockerFixture):
column_name="field1.b",
type=types.DATE(),
is_dttm=True,
+ expression='"field1"."b"',
query_as='"field1"."b" AS "field1.b"',
),
ResultSetColumnType(
@@ -775,6 +777,7 @@ def test_get_columns_expand_rows(mocker: MockerFixture):
column_name="field2.r1",
type=datatype.parse_sqltype("row(a varchar, b varchar)"),
is_dttm=False,
+ expression='"field2"."r1"',
query_as='"field2"."r1" AS "field2.r1"',
),
ResultSetColumnType(
@@ -782,6 +785,7 @@ def test_get_columns_expand_rows(mocker: MockerFixture):
column_name="field2.r1.a",
type=types.VARCHAR(),
is_dttm=False,
+ expression='"field2"."r1"."a"',
query_as='"field2"."r1"."a" AS "field2.r1.a"',
),
ResultSetColumnType(
@@ -789,6 +793,7 @@ def test_get_columns_expand_rows(mocker: MockerFixture):
column_name="field2.r1.b",
type=types.VARCHAR(),
is_dttm=False,
+ expression='"field2"."r1"."b"',
query_as='"field2"."r1"."b" AS "field2.r1.b"',
),
ResultSetColumnType(