This is an automated email from the ASF dual-hosted git repository.
justinpark 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 cb319c9b817 fix(mcp): trust dataset is_dttm flag when applying
time_grain to VARCHAR temporal columns (#42288)
cb319c9b817 is described below
commit cb319c9b8179668a46781eca55628311470b04a5
Author: JUST.in DO IT <[email protected]>
AuthorDate: Thu Jul 23 10:00:22 2026 -0700
fix(mcp): trust dataset is_dttm flag when applying time_grain to VARCHAR
temporal columns (#42288)
---
superset/mcp_service/chart/chart_utils.py | 87 +++++++++++++---------
.../mcp_service/chart/test_chart_utils.py | 83 +++++++++++++++++++++
2 files changed, 135 insertions(+), 35 deletions(-)
diff --git a/superset/mcp_service/chart/chart_utils.py
b/superset/mcp_service/chart/chart_utils.py
index fbd59b03c2f..ea8079ea1e9 100644
--- a/superset/mcp_service/chart/chart_utils.py
+++ b/superset/mcp_service/chart/chart_utils.py
@@ -289,20 +289,59 @@ def _find_dataset_by_id_or_uuid(dataset_id: int | str |
None) -> "SqlaTable | No
return DatasetDAO.find_by_id_or_uuid(str(dataset_id))
+def _is_dataset_column_temporal(
+ col: Any, column_name: str, db_engine_spec: Any
+) -> bool:
+ """Decide temporality for a single dataset column, mirroring
+ TableColumn.is_temporal: native temporal SQL types are always
+ temporal, and is_dttm=True is otherwise trusted over the raw SQL
+ type -- this is the standard, supported way to mark a non-temporal
+ column (e.g. a VARCHAR "ds" partition column on Hive/Presto/Trino)
+ as a date.
+
+ The one case guarded against is a plain NUMERIC column (e.g. an
+ integer "year"/"month" column) that Superset's column-name
+ heuristics may have mis-flagged as is_dttm=True with no
+ python_date_format to parse it -- applying DATE_TRUNC/time_grain to
+ that would fail at query time.
+ """
+ from superset.utils.core import GenericDataType
+
+ is_dttm = bool(getattr(col, "is_dttm", False))
+ col_type = col.type
+ if not col_type:
+ return is_dttm # No type info, trust is_dttm flag
+
+ column_spec = db_engine_spec.get_column_spec(col_type)
+ generic_type = column_spec.generic_type if column_spec else None
+
+ if generic_type == GenericDataType.TEMPORAL:
+ return True
+ if not is_dttm:
+ return False
+ if generic_type != GenericDataType.NUMERIC or getattr(
+ col, "python_date_format", None
+ ):
+ return True
+
+ logger.debug(
+ "Column '%s' is marked is_dttm=True but has numeric type '%s' with "
+ "no python_date_format; treating as non-temporal to avoid an "
+ "invalid DATE_TRUNC on a numeric column",
+ column_name,
+ col_type,
+ )
+ return False
+
+
def is_column_truly_temporal(
column_name: str,
dataset_id: int | str | None,
dataset: "SqlaTable | None" = None,
) -> bool:
"""
- Check if a column is truly temporal based on its SQL data type.
-
- This is important because Superset may mark columns as is_dttm=True based
on
- column name heuristics (e.g., "year", "month"), but if the actual SQL type
is
- BIGINT or INTEGER, DATE_TRUNC will fail.
-
- Uses the database engine spec's column type mapping to determine the actual
- GenericDataType, bypassing the is_dttm flag which may be set incorrectly.
+ Check if a column is truly temporal, mirroring TableColumn.is_temporal
+ (see ``_is_dataset_column_temporal`` for the precedence rules).
Args:
column_name: Name of the column to check
@@ -311,9 +350,8 @@ def is_column_truly_temporal(
redundant DAO lookup when the caller already resolved it.
Returns:
- True if the column has a real temporal SQL type, False otherwise
+ True if the column should be treated as temporal, False otherwise
"""
- from superset.utils.core import GenericDataType
if not dataset_id and dataset is None:
return True # Default to temporal if we can't check (backward
compatible)
@@ -325,34 +363,11 @@ def is_column_truly_temporal(
if not dataset:
return True # Default to temporal if dataset not found
- # Find the column and check its actual type using db_engine_spec
column_lower = column_name.lower()
for col in dataset.columns:
if col.column_name.lower() == column_lower:
- col_type = col.type
- if not col_type:
- # No type info, trust is_dttm flag
- return getattr(col, "is_dttm", False)
-
- # Use the db_engine_spec to get the actual GenericDataType
- # This bypasses the is_dttm flag and checks the real SQL type
db_engine_spec = dataset.database.db_engine_spec
- column_spec = db_engine_spec.get_column_spec(col_type)
-
- if column_spec:
- is_temporal = column_spec.generic_type ==
GenericDataType.TEMPORAL
- if not is_temporal:
- logger.debug(
- "Column '%s' has type '%s' (generic: %s), "
- "treating as non-temporal",
- column_name,
- col_type,
- column_spec.generic_type,
- )
- return is_temporal
-
- # If no column_spec, trust is_dttm flag
- return getattr(col, "is_dttm", False)
+ return _is_dataset_column_temporal(col, column_name,
db_engine_spec)
return True # Default if column not found
@@ -707,7 +722,9 @@ def configure_temporal_handling(
form_data.setdefault("_mcp_warnings", []).append(
f"time_grain='{time_grain}' was ignored because the x-axis "
f"column is not a temporal type. time_grain only applies to "
- f"DATE/DATETIME/TIMESTAMP columns."
+ f"DATE/DATETIME/TIMESTAMP columns, or other column types "
+ f"explicitly marked as temporal (is_dttm) with a "
+ f"python_date_format on the dataset."
)
diff --git a/tests/unit_tests/mcp_service/chart/test_chart_utils.py
b/tests/unit_tests/mcp_service/chart/test_chart_utils.py
index 473a88a76eb..256ee7032c4 100644
--- a/tests/unit_tests/mcp_service/chart/test_chart_utils.py
+++ b/tests/unit_tests/mcp_service/chart/test_chart_utils.py
@@ -1421,11 +1421,15 @@ class TestIsColumnTrulyTemporal:
column_name: str,
column_type: str,
generic_type: GenericDataType,
+ is_dttm: bool = False,
+ python_date_format: str | None = None,
):
"""Helper to create a mock dataset with proper db_engine_spec"""
mock_column = MagicMock()
mock_column.column_name = column_name
mock_column.type = column_type
+ mock_column.is_dttm = is_dttm
+ mock_column.python_date_format = python_date_format
mock_db_engine_spec = MagicMock()
mock_column_spec = ColumnSpec(
@@ -1576,6 +1580,85 @@ class TestIsColumnTrulyTemporal:
result = is_column_truly_temporal("year", 123)
assert result is True
+ @patch("superset.daos.dataset.DatasetDAO")
+ def test_returns_true_for_varchar_ds_column_with_is_dttm_and_date_format(
+ self, mock_dao
+ ) -> None:
+ """VARCHAR 'ds' column explicitly marked is_dttm with a date format
+ (e.g. Hive/Presto string partition column) should be treated as
+ temporal, mirroring TableColumn.is_temporal in Superset core."""
+ mock_dataset = self._create_mock_dataset(
+ "ds",
+ "VARCHAR",
+ GenericDataType.STRING,
+ is_dttm=True,
+ python_date_format="%Y-%m-%d",
+ )
+ mock_dao.find_by_id_or_uuid.return_value = mock_dataset
+
+ result = is_column_truly_temporal("ds", 123)
+ assert result is True
+
+ @patch("superset.daos.dataset.DatasetDAO")
+ def test_returns_true_for_varchar_ds_column_with_is_dttm_no_date_format(
+ self, mock_dao
+ ) -> None:
+ """VARCHAR column marked is_dttm without a python_date_format is
+ still trusted as temporal -- only NUMERIC columns require a date
+ format before being trusted (guards against name-based
+ misdetection like a plain integer 'year' column)."""
+ mock_dataset = self._create_mock_dataset(
+ "ds", "VARCHAR", GenericDataType.STRING, is_dttm=True
+ )
+ mock_dao.find_by_id_or_uuid.return_value = mock_dataset
+
+ result = is_column_truly_temporal("ds", 123)
+ assert result is True
+
+ @patch("superset.daos.dataset.DatasetDAO")
+ def test_returns_false_for_varchar_column_without_is_dttm(self, mock_dao)
-> None:
+ """A plain VARCHAR column not marked temporal stays non-temporal."""
+ mock_dataset = self._create_mock_dataset(
+ "description", "VARCHAR", GenericDataType.STRING, is_dttm=False
+ )
+ mock_dao.find_by_id_or_uuid.return_value = mock_dataset
+
+ result = is_column_truly_temporal("description", 123)
+ assert result is False
+
+ @patch("superset.daos.dataset.DatasetDAO")
+ def test_returns_false_for_numeric_column_with_is_dttm_no_date_format(
+ self, mock_dao
+ ) -> None:
+ """A numeric 'year' column mis-flagged is_dttm=True by naming
+ heuristics, with no python_date_format to parse it, is still
+ rejected to avoid an invalid DATE_TRUNC on a numeric column."""
+ mock_dataset = self._create_mock_dataset(
+ "year", "BIGINT", GenericDataType.NUMERIC, is_dttm=True
+ )
+ mock_dao.find_by_id_or_uuid.return_value = mock_dataset
+
+ result = is_column_truly_temporal("year", 123)
+ assert result is False
+
+ @patch("superset.daos.dataset.DatasetDAO")
+ def test_returns_true_for_numeric_column_with_is_dttm_and_date_format(
+ self, mock_dao
+ ) -> None:
+ """A numeric epoch column explicitly marked is_dttm with a
+ python_date_format (e.g. 'epoch_s') is trusted as temporal."""
+ mock_dataset = self._create_mock_dataset(
+ "created_epoch",
+ "BIGINT",
+ GenericDataType.NUMERIC,
+ is_dttm=True,
+ python_date_format="epoch_s",
+ )
+ mock_dao.find_by_id_or_uuid.return_value = mock_dataset
+
+ result = is_column_truly_temporal("created_epoch", 123)
+ assert result is True
+
class TestConfigureTemporalHandling:
"""Test configure_temporal_handling function"""
