fix: address critical code review issues

- Fix SQL LIMIT portability by using database.apply_limit_to_sql() instead of hard-coded LIMIT syntax (supports SQL Server, Oracle, etc.)
- Add virtual dataset and expression column handling to skip unsupported cases
- Fix test mocking to include engine context manager and apply_limit_to_sql
- Add test coverage for virtual datasets and expression columns

Note: format_map integration will be added to superset/models/helpers.py after rebase

Author: Claude Code

superset/datasets/datetime_format_detector.py

@@ -70,6 +70,23 @@ def detect_column_format(
             )
             return None
 
+        # Skip expression columns - they don't have stored data to sample
+        if column.expression:
+            logger.debug(
+                "Column %s is an expression column, skipping format detection",
+                column.column_name,
+            )
+            return None
+
+        # Skip virtual datasets - they use SQL queries, not physical tables
+        if dataset.is_virtual:
+            logger.debug(
+                "Dataset %s is virtual, skipping format detection for column %s",
+                dataset.table_name,
+                column.column_name,
+            )
+            return None
+
         try:
             # Build SQL query using database's identifier quoting
             # Note: Column and table names come from internal metadata, not user input
@@ -97,10 +114,13 @@ def detect_column_format(
                 # S608: false positive - using dialect's identifier preparer
                 sql = (  # noqa: S608
                     f"SELECT {column_name_quoted} FROM {full_table} "  # noqa: S608
-                    f"WHERE {column_name_quoted} IS NOT NULL "  # noqa: S608
-                    f"LIMIT {self.sample_size}"
+                    f"WHERE {column_name_quoted} IS NOT NULL"  # noqa: S608
                 )
 
+            # Apply database-specific LIMIT using apply_limit_to_sql
+            # This handles different SQL dialects (LIMIT, TOP, FETCH FIRST, etc.)
+            sql = database.apply_limit_to_sql(sql, limit=self.sample_size, force=True)
+
             # Execute query and get results
             df = database.get_df(sql, dataset.schema)
 

tests/unit_tests/datasets/test_datetime_format_detector.py

@@ -31,6 +31,23 @@ def mock_dataset() -> MagicMock:
     dataset = MagicMock(spec=SqlaTable)
     dataset.table_name = "test_table"
     dataset.schema = "test_schema"
+    dataset.is_virtual = False
+
+    # Mock the database engine and dialect for identifier quoting
+    mock_engine = MagicMock()
+    mock_dialect = MagicMock()
+    mock_dialect.identifier_preparer.quote = lambda x: f'"{x}"'
+    mock_engine.dialect = mock_dialect
+
+    # Mock the context manager returned by get_sqla_engine()
+    dataset.database.get_sqla_engine.return_value.__enter__.return_value = mock_engine
+    dataset.database.get_sqla_engine.return_value.__exit__.return_value = None
+
+    # Mock apply_limit_to_sql to return SQL with LIMIT
+    dataset.database.apply_limit_to_sql = (
+        lambda sql, limit, force: f"{sql} LIMIT {limit}"
+    )
+
     return dataset
 
 
@@ -41,6 +58,7 @@ def mock_column() -> MagicMock:
     column.column_name = "date_column"
     column.is_temporal = True
     column.datetime_format = None
+    column.expression = None  # Not an expression column
     return column
 
 
@@ -109,15 +127,18 @@ def test_detect_all_formats(mock_dataset: MagicMock) -> None:
     col1.column_name = "date1"
     col1.is_temporal = True
     col1.datetime_format = None
+    col1.expression = None
 
     col2 = MagicMock(spec=TableColumn)
     col2.column_name = "date2"
     col2.is_temporal = True
     col2.datetime_format = None
+    col2.expression = None
 
     col3 = MagicMock(spec=TableColumn)
     col3.column_name = "text_col"
     col3.is_temporal = False
+    col3.expression = None
 
     mock_dataset.columns = [col1, col2, col3]
 
@@ -144,6 +165,7 @@ def test_detect_all_formats_skip_existing(mock_dataset: MagicMock) -> None:
     col1.column_name = "date1"
     col1.is_temporal = True
     col1.datetime_format = "%Y-%m-%d"
+    col1.expression = None
 
     mock_dataset.columns = [col1]
 
@@ -161,6 +183,7 @@ def test_detect_all_formats_force_redetection(mock_dataset: MagicMock) -> None:
     col1.column_name = "date1"
     col1.is_temporal = True
     col1.datetime_format = "%Y-%m-%d"
+    col1.expression = None
 
     mock_dataset.columns = [col1]
     mock_dataset.table_name = "test_table"
@@ -174,3 +197,29 @@ def test_detect_all_formats_force_redetection(mock_dataset: MagicMock) -> None:
 
     assert results["date1"] == "%m/%d/%Y"
     assert col1.datetime_format == "%m/%d/%Y"
+
+
+def test_detect_column_format_virtual_dataset(
+    mock_dataset: MagicMock, mock_column: MagicMock
+) -> None:
+    """Test that virtual datasets are skipped."""
+    mock_dataset.is_virtual = True
+
+    detector = DatetimeFormatDetector()
+    detected_format = detector.detect_column_format(mock_dataset, mock_column)
+
+    assert detected_format is None
+    mock_dataset.database.get_df.assert_not_called()
+
+
+def test_detect_column_format_expression_column(
+    mock_dataset: MagicMock, mock_column: MagicMock
+) -> None:
+    """Test that expression columns are skipped."""
+    mock_column.expression = "DATE_ADD(some_date, INTERVAL 1 DAY)"
+
+    detector = DatetimeFormatDetector()
+    detected_format = detector.detect_column_format(mock_dataset, mock_column)
+
+    assert detected_format is None
+    mock_dataset.database.get_df.assert_not_called()