This is an automated email from the ASF dual-hosted git repository.
rusackas 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 298aa2f0ba4 fix(dataset): bump changed_on on column refresh so chart
cache invalidates (#43921)
298aa2f0ba4 is described below
commit 298aa2f0ba4b58c1a4c87fb9bc36be4dcd917c92
Author: Prajasree <[email protected]>
AuthorDate: Thu Sep 24 02:04:06 2026 +0530
fix(dataset): bump changed_on on column refresh so chart cache invalidates
(#43921)
Co-authored-by: rusackas <[email protected]>
Co-authored-by: Evan Rusackas <[email protected]>
Co-authored-by: Claude Sonnet 5 <[email protected]>
---
superset/connectors/sqla/models.py | 53 +++++-
tests/integration_tests/datasets/api_tests.py | 15 +-
tests/unit_tests/connectors/sqla/models_test.py | 234 ++++++++++++++++++++++++
3 files changed, 292 insertions(+), 10 deletions(-)
diff --git a/superset/connectors/sqla/models.py
b/superset/connectors/sqla/models.py
index fc58e885d58..7bb0b48855a 100644
--- a/superset/connectors/sqla/models.py
+++ b/superset/connectors/sqla/models.py
@@ -23,7 +23,7 @@ import re
from collections import defaultdict
from collections.abc import Hashable
from dataclasses import dataclass, field
-from datetime import timedelta
+from datetime import datetime, timedelta
from typing import Any, Callable, cast, Optional, Union
from urllib.parse import parse_qsl, quote, urlencode, urlsplit, urlunsplit
@@ -153,6 +153,27 @@ class MetadataResult:
modified: list[str] = field(default_factory=list)
+def _is_calculated_column(column: TableColumn) -> bool:
+ """Return whether *column* is a user-defined virtual column.
+
+ ``fetch_metadata`` keeps calculated columns that the source table does
+ not list. Engine specs such as Trino also store an ``expression`` on
+ expanded nested ``ROW`` fields, whose name is the dotted path (e.g.
+ ``metadata.uuid``) and whose expression is always that same path
+ quoted per-part (e.g. ``"metadata"."uuid"``). Those are still physical
+ columns: if the source no longer lists them they must be dropped so
+ chart cache keys invalidate. A user-authored calculated column can
+ also have a dotted name, so the dot alone cannot be the signal; only
+ drop columns whose expression matches Trino's quoted-path pattern for
+ its own name. See #43918.
+ """
+ if not column.expression:
+ return False
+ name = column.column_name or ""
+ quoted_path = ".".join(f'"{part}"' for part in name.split("."))
+ return column.expression != quoted_path
+
+
METRIC_FORM_DATA_PARAMS = [
"metric",
"metric_2",
@@ -2385,7 +2406,12 @@ class SqlaTable(
new_column.expression = expression
else:
new_column = old_column
- if new_column.type != col["type"]:
+ # Type and physical expression both feed generated SQL, so
+ # either change is schema drift that must invalidate chart
+ # cache keys (see changed_on bump below).
+ if new_column.type != col["type"] or (
+ (new_column.expression or "") != expression
+ ):
results.modified.append(col["column_name"])
new_column.type = col["type"]
new_column.expression = expression
@@ -2400,11 +2426,15 @@ class SqlaTable(
# Add back calculated (virtual) columns, i.e. those that weren't
matched
# against `new_columns` above and are thus still present in
- # `old_columns_by_name`. Columns that were matched are already
appended to
- # `columns` in the loop above, and re-adding them here (e.g. via
`old_columns`)
- # would duplicate any synced physical column that also carries a truthy
- # `expression`, such as Trino's expanded nested `ROW` fields.
- columns.extend([col for col in old_columns_by_name.values() if
col.expression])
+ # `old_columns_by_name`. Nested physical ROW fields also carry an
+ # expression; they are not calculated columns and must not be kept
+ # when the source no longer lists them (delete-orphan then removes
+ # the TableColumn row).
+ leftover_columns = list(old_columns_by_name.values())
+ dropped_physical_columns = any(
+ not _is_calculated_column(col) for col in leftover_columns
+ )
+ columns.extend(col for col in leftover_columns if
_is_calculated_column(col))
self.columns = columns
if not self.main_dttm_col:
@@ -2414,6 +2444,15 @@ class SqlaTable(
# Apply config supplied mutations.
current_app.config["SQLA_TABLE_MUTATOR"](self)
+ # Child TableColumn rows own the FK, so mutating them (and reassigning
+ # ``self.columns``) does not emit an UPDATE on this tables row.
+ # AuditMixinNullable.changed_on onupdate therefore never fires, and
+ # query_cache_key() keeps serving results computed against the previous
+ # column definitions. Force the same bump DatasetDAO.update() applies
+ # when columns are saved. See #43918.
+ if results.added or results.modified or dropped_physical_columns:
+ self.changed_on = datetime.now()
+
db.session.merge(self)
return results
diff --git a/tests/integration_tests/datasets/api_tests.py
b/tests/integration_tests/datasets/api_tests.py
index d5fa10902a6..6a6ff7e8a23 100644
--- a/tests/integration_tests/datasets/api_tests.py
+++ b/tests/integration_tests/datasets/api_tests.py
@@ -2349,17 +2349,22 @@ class TestDatasetApi(SupersetTestCase):
"""
dataset = self.insert_default_dataset()
- # delete a column
+ # delete a column so refresh has schema drift to sync
id_column = (
db.session.query(TableColumn)
.filter_by(table_id=dataset.id, column_name="id")
.one()
)
- self.items_to_delete = [id_column]
+ db.session.delete(id_column)
+ db.session.commit()
+ db.session.refresh(dataset)
+ current_changed_on = dataset.changed_on
self.login(ADMIN_USERNAME)
uri = f"api/v1/dataset/{dataset.id}/refresh"
- rv = self.put_assert_metric(uri, {}, "refresh")
+ with freeze_time() as frozen:
+ frozen.tick(delta=timedelta(seconds=3))
+ rv = self.put_assert_metric(uri, {}, "refresh")
assert rv.status_code == 200
# Assert the column is restored on refresh
id_column = (
@@ -2368,6 +2373,10 @@ class TestDatasetApi(SupersetTestCase):
.one()
)
assert id_column is not None
+ # Refresh mutates child TableColumn rows only, so changed_on must be
+ # force-bumped or chart cache keys stay stale. See #43918.
+ updated_dataset =
db.session.query(SqlaTable).filter_by(id=dataset.id).first()
+ assert updated_dataset.changed_on > current_changed_on
self.items_to_delete = [dataset]
def test_dataset_item_refresh_not_found(self):
diff --git a/tests/unit_tests/connectors/sqla/models_test.py
b/tests/unit_tests/connectors/sqla/models_test.py
index 4d9b9e92bd7..41212930f9e 100644
--- a/tests/unit_tests/connectors/sqla/models_test.py
+++ b/tests/unit_tests/connectors/sqla/models_test.py
@@ -15,10 +15,12 @@
# specific language governing permissions and limitations
# under the License.
+from datetime import datetime
from unittest.mock import MagicMock
import pandas as pd
import pytest
+from freezegun import freeze_time
from jinja2.exceptions import UndefinedError
from pytest_mock import MockerFixture
from sqlalchemy import create_engine
@@ -937,6 +939,238 @@ def
test_fetch_metadata_empty_comment_field_handling(mocker: MockerFixture) -> N
assert columns_by_name["col_with_valid_comment"].description == "Valid
comment"
+def _table_for_fetch_metadata(
+ mocker: MockerFixture,
+ source_columns: list[dict[str, str]],
+ existing: list[dict[str, str]] | None = None,
+) -> SqlaTable:
+ """Build a SqlaTable whose ``fetch_metadata`` reads *source_columns*."""
+ database = mocker.MagicMock()
+ database.get_metrics.return_value = []
+ database.db_engine_spec = mocker.MagicMock()
+ table = SqlaTable(table_name="test_table", database=database)
+ table.id = 1
+ existing_cols = [
+ TableColumn(
+ column_name=spec["column_name"],
+ type=spec["type"],
+ table=table,
+ expression=spec.get("expression") or "",
+ )
+ for spec in existing or []
+ ]
+ table.columns = existing_cols
+ mock_session = mocker.patch("superset.connectors.sqla.models.db.session")
+ mock_session.query.return_value.filter.return_value.all.return_value =
existing_cols
+ mocker.patch.object(table, "external_metadata",
return_value=source_columns)
+ return table
+
+
+def test_fetch_metadata_bumps_changed_on_when_column_type_changes(
+ mocker: MockerFixture,
+) -> None:
+ """Schema drift on an existing column must bump ``changed_on``.
+
+ ``query_cache_key`` includes ``datasource.changed_on``; without this bump
+ a Refresh-columns action would keep serving chart results computed
+ against the previous type. See #43918.
+ """
+ table = _table_for_fetch_metadata(
+ mocker,
+ source_columns=[{"column_name": "revenue", "type": "INTEGER"}],
+ existing=[{"column_name": "revenue", "type": "VARCHAR"}],
+ )
+ table.changed_on = datetime(2024, 6, 1, 12, 0, 0)
+
+ with freeze_time("2024-06-01 12:00:05"):
+ result = table.fetch_metadata()
+
+ assert result.modified == ["revenue"]
+ assert table.changed_on == datetime(2024, 6, 1, 12, 0, 5)
+
+
+def test_fetch_metadata_bumps_changed_on_when_column_added(
+ mocker: MockerFixture,
+) -> None:
+ """A newly discovered source column must bump ``changed_on``."""
+ table = _table_for_fetch_metadata(
+ mocker,
+ source_columns=[
+ {"column_name": "id", "type": "INTEGER"},
+ {"column_name": "name", "type": "VARCHAR"},
+ ],
+ existing=[{"column_name": "id", "type": "INTEGER"}],
+ )
+ table.changed_on = datetime(2024, 6, 1, 12, 0, 0)
+
+ with freeze_time("2024-06-01 12:00:05"):
+ result = table.fetch_metadata()
+
+ assert result.added == ["name"]
+ assert table.changed_on == datetime(2024, 6, 1, 12, 0, 5)
+
+
+def test_fetch_metadata_bumps_changed_on_when_physical_column_removed(
+ mocker: MockerFixture,
+) -> None:
+ """Dropping a physical source column must bump ``changed_on``."""
+ table = _table_for_fetch_metadata(
+ mocker,
+ source_columns=[{"column_name": "id", "type": "INTEGER"}],
+ existing=[
+ {"column_name": "id", "type": "INTEGER"},
+ {"column_name": "name", "type": "VARCHAR"},
+ ],
+ )
+ table.changed_on = datetime(2024, 6, 1, 12, 0, 0)
+
+ with freeze_time("2024-06-01 12:00:05"):
+ result = table.fetch_metadata()
+
+ assert result.removed == ["name"]
+ assert table.changed_on == datetime(2024, 6, 1, 12, 0, 5)
+
+
+def test_fetch_metadata_drops_nested_physical_column_and_bumps_changed_on(
+ mocker: MockerFixture,
+) -> None:
+ """A dropped nested ROW field has an expression but is still physical.
+
+ Keeping it would leave a stale TableColumn and skip the changed_on bump.
+ """
+ table = _table_for_fetch_metadata(
+ mocker,
+ source_columns=[{"column_name": "id", "type": "INTEGER"}],
+ existing=[
+ {"column_name": "id", "type": "INTEGER"},
+ {
+ "column_name": "metadata.uuid",
+ "type": "VARCHAR",
+ "expression": '"metadata"."uuid"',
+ },
+ ],
+ )
+ table.changed_on = datetime(2024, 6, 1, 12, 0, 0)
+
+ with freeze_time("2024-06-01 12:00:05"):
+ result = table.fetch_metadata()
+
+ assert "metadata.uuid" in result.removed
+ assert all(col.column_name != "metadata.uuid" for col in table.columns)
+ assert table.changed_on == datetime(2024, 6, 1, 12, 0, 5)
+
+
+def test_fetch_metadata_bumps_changed_on_when_expression_changes(
+ mocker: MockerFixture,
+) -> None:
+ """A physical expression change is schema drift and must bump
``changed_on``."""
+ table = _table_for_fetch_metadata(
+ mocker,
+ source_columns=[
+ {
+ "column_name": "metadata.uuid",
+ "type": "VARCHAR",
+ "expression": '"metadata"."uuid"',
+ }
+ ],
+ existing=[
+ {
+ "column_name": "metadata.uuid",
+ "type": "VARCHAR",
+ "expression": "",
+ }
+ ],
+ )
+ table.changed_on = datetime(2024, 6, 1, 12, 0, 0)
+
+ with freeze_time("2024-06-01 12:00:05"):
+ result = table.fetch_metadata()
+
+ assert result.modified == ["metadata.uuid"]
+ assert table.changed_on == datetime(2024, 6, 1, 12, 0, 5)
+
+
+def test_fetch_metadata_does_not_bump_changed_on_when_schema_unchanged(
+ mocker: MockerFixture,
+) -> None:
+ """A no-op refresh must not invalidate chart cache keys."""
+ table = _table_for_fetch_metadata(
+ mocker,
+ source_columns=[{"column_name": "revenue", "type": "INTEGER"}],
+ existing=[{"column_name": "revenue", "type": "INTEGER"}],
+ )
+ original_changed_on = datetime(2024, 6, 1, 12, 0, 0)
+ table.changed_on = original_changed_on
+
+ with freeze_time("2024-06-01 12:00:05"):
+ result = table.fetch_metadata()
+
+ assert result.added == []
+ assert result.modified == []
+ assert result.removed == []
+ assert table.changed_on == original_changed_on
+
+
+def test_fetch_metadata_does_not_bump_changed_on_for_kept_virtual_columns(
+ mocker: MockerFixture,
+) -> None:
+ """Calculated columns are reported in ``removed`` but kept; that is not
drift."""
+ table = _table_for_fetch_metadata(
+ mocker,
+ source_columns=[{"column_name": "id", "type": "INTEGER"}],
+ existing=[
+ {"column_name": "id", "type": "INTEGER"},
+ {
+ "column_name": "profit",
+ "type": "INTEGER",
+ "expression": "revenue - cost",
+ },
+ ],
+ )
+ original_changed_on = datetime(2024, 6, 1, 12, 0, 0)
+ table.changed_on = original_changed_on
+
+ with freeze_time("2024-06-01 12:00:05"):
+ result = table.fetch_metadata()
+
+ assert "profit" in result.removed
+ assert any(col.column_name == "profit" for col in table.columns)
+ assert table.changed_on == original_changed_on
+
+
+def test_fetch_metadata_keeps_dotted_calculated_column(
+ mocker: MockerFixture,
+) -> None:
+ """A user calculated column with a dotted name must survive a refresh.
+
+ Only Trino's quoted-path expression signature for expanded ``ROW``
+ fields identifies a leftover as physical; a bare dot in the name is
+ not enough, since ``DatasetColumnsPutSchema.column_name`` allows any
+ name up to 255 characters. See #43918.
+ """
+ table = _table_for_fetch_metadata(
+ mocker,
+ source_columns=[{"column_name": "id", "type": "INTEGER"}],
+ existing=[
+ {"column_name": "id", "type": "INTEGER"},
+ {
+ "column_name": "revenue.usd",
+ "type": "INTEGER",
+ "expression": "revenue * fx_rate",
+ },
+ ],
+ )
+ original_changed_on = datetime(2024, 6, 1, 12, 0, 0)
+ table.changed_on = original_changed_on
+
+ with freeze_time("2024-06-01 12:00:05"):
+ result = table.fetch_metadata()
+
+ assert "revenue.usd" in result.removed
+ assert any(col.column_name == "revenue.usd" for col in table.columns)
+ assert table.changed_on == original_changed_on
+
+
@pytest.mark.parametrize(
"supports_cross_catalog,table_name,catalog,schema,expected_name,expected_schema",
[
