fix(explore): require a value for simple adhoc filters before allowing save

The Save button in the adhoc filter popover stayed enabled when a
comparator-taking operator had no value, because `AdhocFilter.isValid()`
only rejected a `null` comparator. An unset comparator is `undefined`, not
`null`: selecting a subject resets it (and falls back to the `IN` operator),
and the value Select's clear affordance emits `undefined` as well.

This was most visible on boolean columns, whose operator list is restricted
to unary operators, so a freshly picked boolean column lands on `IN` with no
value and the popover looks complete. Saving sent a filter with no `val` to
the query API, which tripped a bare `assert isinstance(eq, (tuple, list))`
in the query builder and surfaced as a generic error instead of inline
client-side validation.

Extend the existing check to treat `undefined` like `null`, matching the
empty-array guard already applied to `IN`/`NOT IN` comparators. Unary
operators are unaffected: they short-circuit earlier via
DISABLE_INPUT_OPERATORS.

.github/labeler.yml

@@ -29,7 +29,7 @@
 "dependencies:python":
 - changed-files:
   - any-glob-to-any-file:
-    - 'superset/requirements/**'
+    - 'requirements/**'
     - 'superset/translations/requirements.txt'
     - 'RELEASING/requirements.txt'
 

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/AdhocFilter.test.ts

@@ -207,6 +207,38 @@ describe('AdhocFilter', () => {
     expect(adhocFilter10.isValid()).toBe(true);
   });
 
+  test('is invalid when a comparator-taking operator has no comparator', () => {
+    // A comparator that was never set, or that was cleared through the value
+    // Select's clear affordance, is `undefined` rather than `null` or `[]`.
+    const adhocFilter1 = new AdhocFilter({
+      expressionType: ExpressionTypes.Simple,
+      subject: 'is_intro',
+      operator: 'IN',
+      comparator: undefined,
+      clause: Clauses.Where,
+    });
+    expect(adhocFilter1.isValid()).toBe(false);
+
+    const adhocFilter2 = new AdhocFilter({
+      expressionType: ExpressionTypes.Simple,
+      subject: 'is_intro',
+      operator: '==',
+      comparator: undefined,
+      clause: Clauses.Where,
+    });
+    expect(adhocFilter2.isValid()).toBe(false);
+
+    // `false` is a legitimate boolean comparator, not a missing value
+    const adhocFilter3 = new AdhocFilter({
+      expressionType: ExpressionTypes.Simple,
+      subject: 'is_intro',
+      operator: '==',
+      comparator: false,
+      clause: Clauses.Where,
+    });
+    expect(adhocFilter3.isValid()).toBe(true);
+  });
+
   test('can translate from simple expressions to sql expressions', () => {
     const adhocFilter1 = new AdhocFilter({
       expressionType: ExpressionTypes.Simple,

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.ts

@@ -163,8 +163,10 @@ export default class AdhocFilter {
           // A non-empty array of values ('IN' or 'NOT IN' clauses)
           return this.comparator.length > 0;
         }
-        // A value has been selected or typed
-        return this.comparator !== null;
+        // A value has been selected or typed. An unset comparator is
+        // `undefined` rather than `null`: picking a new subject resets it, and
+        // the value Select's clear affordance emits `undefined` too.
+        return this.comparator != null;
       }
     }
 

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/AdhocFilterEditPopover.test.tsx

@@ -181,6 +181,29 @@ describe('AdhocFilterEditPopover', () => {
     expect(saveButton).toBeDisabled();
   });
 
+  test('disables save button when a boolean column has no value selected', async () => {
+    const booleanColumn = { type: 'BOOL', column_name: 'is_intro' };
+    renderPopover({
+      adhocFilter: new AdhocFilter({
+        expressionType: ExpressionTypes.Simple,
+        clause: Clauses.Where,
+      }),
+      options: [booleanColumn],
+      datasource: { columns: [booleanColumn], filter_select: false },
+    });
+
+    // Picking the subject resets the comparator to `undefined`; the value
+    // control is then left untouched, mirroring the reported repro.
+    await userEvent.click(screen.getByTestId('select-element'));
+    await userEvent.click(
+      await screen.findByRole('option', { name: /is_intro/ }),
+    );
+
+    expect(
+      screen.getByTestId('adhoc-filter-edit-popover-save-button'),
+    ).toBeDisabled();
+  });
+
   test('initiates resize when resize handle is dragged', async () => {
     const onResize = jest.fn();
     renderPopover({ onResize });

superset/connectors/sqla/models.py

@@ -1228,11 +1228,7 @@ class TableColumn(AuditMixinNullable, ImportExportMixin, CertificationMixin, Mod
             expression = self._validate_stored_expression(expression)
             col = literal_column(expression, type_=type_)
         else:
-            identifier = db_engine_spec.prepare_identifier(
-                cast(str, self.column_name),
-                normalize_columns=bool(getattr(self.table, "normalize_columns", False)),
-            )
-            col = column(identifier, type_=type_)
+            col = column(self.column_name, type_=type_)
         col = self.database.make_sqla_column_compatible(col, label)
         return col
 
@@ -1258,15 +1254,12 @@ class TableColumn(AuditMixinNullable, ImportExportMixin, CertificationMixin, Mod
 
         pdf = self.python_date_format
         is_epoch = pdf in ("epoch_s", "epoch_ms")
-        db_engine_spec = self.db_engine_spec
-        column_spec = db_engine_spec.get_column_spec(self.type, db_extra=self.db_extra)
+        column_spec = self.db_engine_spec.get_column_spec(
+            self.type, db_extra=self.db_extra
+        )
         type_ = column_spec.sqla_type if column_spec else DateTime
         if not self.expression and not time_grain and not is_epoch:
-            identifier = db_engine_spec.prepare_identifier(
-                cast(str, self.column_name),
-                normalize_columns=bool(getattr(self.table, "normalize_columns", False)),
-            )
-            sqla_col = column(identifier, type_=type_)
+            sqla_col = column(self.column_name, type_=type_)
             return self.database.make_sqla_column_compatible(sqla_col, label)
         if expression := self.expression:
             if template_processor:
@@ -1291,12 +1284,8 @@ class TableColumn(AuditMixinNullable, ImportExportMixin, CertificationMixin, Mod
             expression = self._validate_stored_expression(expression)
             col = literal_column(expression, type_=type_)
         else:
-            identifier = db_engine_spec.prepare_identifier(
-                cast(str, self.column_name),
-                normalize_columns=bool(getattr(self.table, "normalize_columns", False)),
-            )
-            col = column(identifier, type_=type_)
-        time_expr = db_engine_spec.get_timestamp_expr(col, pdf, time_grain)
+            col = column(self.column_name, type_=type_)
+        time_expr = self.db_engine_spec.get_timestamp_expr(col, pdf, time_grain)
         return self.database.make_sqla_column_compatible(time_expr, label)
 
     @property

superset/db_engine_specs/README.md

@@ -83,7 +83,7 @@ The tables below (generated via `python superset/db_engine_specs/lib.py`) summar
 | Databricks (legacy) | 70 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | StarRocks | 69 | Supported | Partial | Supported | Partial | Partial | Partial |
 | SingleStore | 68 | Supported | Partial | Supported | Not supported | Partial | Not supported |
-| ClickHouse Connect (Superset) | 61 | Supported | Partial | Partial | Partial | Partial | Not supported |
+| ClickHouse Connect (Superset) | 62 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | Google Sheets | 61 | Supported | Partial | Supported | Supported | Partial | Partial |
 | Aurora MySQL (Data API) | 59 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | MariaDB | 59 | Supported | Partial | Supported | Partial | Partial | Not supported |
@@ -91,7 +91,7 @@ The tables below (generated via `python superset/db_engine_specs/lib.py`) summar
 | OceanBase | 59 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | MotherDuck | 58 | Supported | Partial | Supported | Not supported | Partial | Not supported |
 | KustoSQL | 54 | Supported | Partial | Supported | Partial | Partial | Not supported |
-| ClickHouse | 51 | Supported | Partial | Partial | Partial | Partial | Not supported |
+| ClickHouse | 52 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | Databend | 51 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | Apache Drill | 50 | Supported | Partial | Supported | Partial | Partial | Partial |
 | Apache Druid | 47 | Partial | Partial | Supported | Partial | Partial | Not supported |
@@ -293,8 +293,8 @@ The tables below (generated via `python superset/db_engine_specs/lib.py`) summar
 | Aurora MySQL (Data API) | True | True | True | True | True | True | True | True |
 | Aurora PostgreSQL (Data API) | True | True | True | True | True | True | True | True |
 | Azure Synapse | True | True | True | True | True | True | True | True |
-| ClickHouse | False | True | True | True | True | True | True | True |
-| ClickHouse Connect (Superset) | False | True | True | True | True | True | True | True |
+| ClickHouse | True | True | True | True | True | True | True | True |
+| ClickHouse Connect (Superset) | True | True | True | True | True | True | True | True |
 | CockroachDB | True | True | True | True | True | True | True | True |
 | Couchbase | True | True | True | True | False | True | True | True |
 | CrateDB | True | True | True | True | True | True | True | True |

superset/db_engine_specs/base.py

@@ -2808,19 +2808,6 @@ class BaseEngineSpec:  # pylint: disable=too-many-public-methods
 
         return name
 
-    @classmethod
-    def prepare_identifier(
-        cls,
-        name: str,
-        normalize_columns: bool = False,
-    ) -> str:
-        """
-        Prepare a physical identifier for SQLAlchemy column construction.
-
-        The default preserves SQLAlchemy's automatic identifier-quoting behavior.
-        """
-        return name
-
     @classmethod
     def quote_table(cls, table: Table, dialect: Dialect) -> str:
         """

superset/db_engine_specs/clickhouse.py

@@ -112,6 +112,7 @@ class ClickHouseBaseEngineSpec(BaseEngineSpec):
 
     _time_grain_expressions = {
         None: "{col}",
+        "PT1S": "toStartOfSecond(toDateTime64({col}, 3))",
         "PT1M": "toStartOfMinute(toDateTime({col}))",
         "PT5M": "toDateTime(intDiv(toUInt32(toDateTime({col})), 300)*300)",
         "PT10M": "toDateTime(intDiv(toUInt32(toDateTime({col})), 600)*600)",

superset/db_engine_specs/snowflake.py

@@ -33,7 +33,6 @@ from marshmallow import fields, Schema
 from sqlalchemy import text, types
 from sqlalchemy.engine.reflection import Inspector
 from sqlalchemy.engine.url import URL
-from sqlalchemy.sql import quoted_name
 
 from superset.constants import TimeGrain
 from superset.databases.utils import make_url_safe
@@ -99,17 +98,6 @@ class SnowflakeEngineSpec(PostgresBaseEngineSpec):
     supports_catalog = supports_dynamic_catalog = supports_cross_catalog_queries = True
     supports_grouping_sets = True
 
-    @classmethod
-    def prepare_identifier(
-        cls,
-        name: str,
-        normalize_columns: bool = False,
-    ) -> str:
-        """Preserve exact-case physical identifiers when columns are not normalized."""
-        if normalize_columns:
-            return name
-        return quoted_name(name, quote=True)
-
     metadata = {
         "description": "Snowflake is a cloud-native data warehouse.",
         "logo": "snowflake.svg",

superset/models/helpers.py

@@ -3899,11 +3899,7 @@ class ExploreMixin:  # pylint: disable=too-many-public-methods
             expression = self._validate_stored_expression(expression)
             col = literal_column(expression, type_=type_)
         else:
-            identifier = db_engine_spec.prepare_identifier(
-                cast(str, tbl_column.column_name),
-                normalize_columns=bool(self.normalize_columns),
-            )
-            col = sa.column(identifier, type_=type_)
+            col = sa.column(tbl_column.column_name, type_=type_)
         col = self.make_sqla_column_compatible(col, label)
         return col
 

tests/unit_tests/connectors/sqla/models_test.py

@@ -21,7 +21,6 @@ import pandas as pd
 import pytest
 from pytest_mock import MockerFixture
 from sqlalchemy import create_engine
-from sqlalchemy.dialects import sqlite
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy.orm.session import Session
 
@@ -50,70 +49,6 @@ from superset.superset_typing import QueryObjectDict
 from superset.utils import json
 
 
-def test_get_sqla_col_quotes_snowflake_case_sensitive_identifier(
-    mocker: MockerFixture,
-) -> None:
-    """Snowflake physical columns retain their exact reflected case in generated SQL."""
-    from superset.db_engine_specs.snowflake import SnowflakeEngineSpec
-
-    database = Database(database_name="db", sqlalchemy_uri="sqlite://")
-    mocker.patch.object(
-        Database,
-        "get_db_engine_spec",
-        return_value=SnowflakeEngineSpec,
-    )
-    table = SqlaTable(
-        table_name="bug_test",
-        database=database,
-        normalize_columns=False,
-    )
-    tbl_column = TableColumn(column_name="id", type="INTEGER", table=table)
-
-    rendered = str(
-        tbl_column.get_sqla_col().compile(
-            dialect=sqlite.dialect(),
-            compile_kwargs={"literal_binds": True},
-        )
-    )
-
-    assert rendered == '"id"'
-
-
-@pytest.mark.parametrize("time_grain", [None, "P1D"])
-def test_get_timestamp_expression_quotes_snowflake_case_sensitive_identifier(
-    mocker: MockerFixture,
-    time_grain: str | None,
-) -> None:
-    """Snowflake timestamp paths quote exact-case physical columns."""
-    from superset.db_engine_specs.snowflake import SnowflakeEngineSpec
-
-    database = Database(database_name="db", sqlalchemy_uri="sqlite://")
-    mocker.patch.object(
-        Database,
-        "get_db_engine_spec",
-        return_value=SnowflakeEngineSpec,
-    )
-    table = SqlaTable(
-        table_name="bug_test",
-        database=database,
-        normalize_columns=False,
-    )
-    tbl_column = TableColumn(
-        column_name="created_at",
-        type="TIMESTAMP",
-        table=table,
-    )
-
-    rendered = str(
-        tbl_column.get_timestamp_expression(time_grain=time_grain).compile(
-            dialect=sqlite.dialect(),
-            compile_kwargs={"literal_binds": True},
-        )
-    )
-
-    assert '"created_at"' in rendered
-
-
 def test_query_bubbles_errors(mocker: MockerFixture) -> None:
     """
     Test that the `query` method bubbles exceptions correctly.

tests/unit_tests/db_engine_specs/test_base.py

@@ -291,13 +291,6 @@ def test_get_default_catalog(mocker: MockerFixture) -> None:
     assert BaseEngineSpec.get_default_catalog(database) is None
 
 
-def test_prepare_identifier_returns_name_unchanged() -> None:
-    name = "physical_column"
-
-    assert BaseEngineSpec.prepare_identifier(name, normalize_columns=False) is name
-    assert BaseEngineSpec.prepare_identifier(name, normalize_columns=True) is name
-
-
 def test_quote_table() -> None:
     """
     Test the `quote_table` function.

tests/unit_tests/db_engine_specs/test_clickhouse.py

@@ -62,6 +62,20 @@ def test_convert_dttm(
     assert_convert_dttm(spec, target_type, expected_result, dttm)
 
 
+@pytest.mark.parametrize(
+    "time_grain,expected",
+    [
+        (None, "{col}"),
+        ("PT1S", "toStartOfSecond(toDateTime64({col}, 3))"),
+        ("PT1M", "toStartOfMinute(toDateTime({col}))"),
+    ],
+)
+def test_time_grain_expressions(time_grain: Optional[str], expected: str) -> None:
+    from superset.db_engine_specs.clickhouse import ClickHouseBaseEngineSpec
+
+    assert ClickHouseBaseEngineSpec._time_grain_expressions[time_grain] == expected
+
+
 def test_convert_dttm_normalizes_aware_datetime_to_utc() -> None:
     from superset.db_engine_specs.clickhouse import (
         ClickHouseEngineSpec as spec,  # noqa: N813

tests/unit_tests/db_engine_specs/test_snowflake.py

@@ -24,7 +24,6 @@ from unittest import mock
 import pytest
 from pytest_mock import MockerFixture
 from sqlalchemy.engine.url import make_url
-from sqlalchemy.sql import quoted_name
 
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
 from superset.utils import json
@@ -32,32 +31,6 @@ from tests.unit_tests.db_engine_specs.utils import assert_convert_dttm
 from tests.unit_tests.fixtures.common import dttm  # noqa: F401
 
 
-@pytest.mark.parametrize("name", ["lowercase", "UPPERCASE"])
-def test_prepare_identifier_quotes_exact_case_names(name: str) -> None:
-    from superset.db_engine_specs.snowflake import SnowflakeEngineSpec
-
-    identifier = SnowflakeEngineSpec.prepare_identifier(
-        name,
-        normalize_columns=False,
-    )
-
-    assert isinstance(identifier, quoted_name)
-    assert str(identifier) == name
-    assert identifier.quote is True
-
-
-def test_prepare_identifier_preserves_normalized_name() -> None:
-    from superset.db_engine_specs.snowflake import SnowflakeEngineSpec
-
-    name = "lowercase"
-    identifier = SnowflakeEngineSpec.prepare_identifier(
-        name,
-        normalize_columns=True,
-    )
-
-    assert identifier is name
-
-
 @pytest.mark.parametrize(
     "target_type,expected_result",
     [

tests/unit_tests/models/helpers_test.py

@@ -4552,40 +4552,6 @@ def test_simple_metric_quotes_column_requiring_quoting(database: Database) -> No
     )
 
 
-def test_convert_tbl_column_quotes_snowflake_case_sensitive_identifier(
-    database: Database,
-    mocker: MockerFixture,
-) -> None:
-    """The chart query-object path quotes exact-case Snowflake physical columns."""
-    from superset.connectors.sqla.models import SqlaTable, TableColumn
-    from superset.db_engine_specs.snowflake import SnowflakeEngineSpec
-    from superset.models.core import Database
-
-    mocker.patch.object(
-        Database,
-        "get_db_engine_spec",
-        return_value=SnowflakeEngineSpec,
-    )
-    table = SqlaTable(
-        database=database,
-        table_name="bug_test",
-        normalize_columns=False,
-    )
-    tbl_column = TableColumn(column_name="name", type="VARCHAR", table=table)
-
-    with database.get_sqla_engine() as engine:
-        dialect = engine.dialect
-
-    rendered = str(
-        table.convert_tbl_column_to_sqla_col(tbl_column).compile(
-            dialect=dialect,
-            compile_kwargs={"literal_binds": True},
-        )
-    )
-
-    assert rendered == '"name"'
-
-
 @pytest.mark.parametrize(
     "native_type",
     [