This is an automated email from the ASF dual-hosted git repository. michael-s-molina pushed a commit to branch 6.2 in repository https://gitbox.apache.org/repos/asf/superset.git
commit ec0f2e6d1488db128efeca6e9f789fdbbf212abb Author: Luiz Otavio <[email protected]> AuthorDate: Fri Jul 10 16:54:33 2026 -0300 fix(cross-filter): correctly cast adhoc column types when cross filtering (#41936) Co-authored-by: Claude Sonnet 4.6 <[email protected]> (cherry picked from commit e0b1fe4ff4c1cde9d5709e04aa2c47e6651a1f4f) --- superset/connectors/sqla/models.py | 21 ++- superset/models/helpers.py | 16 ++- superset/models/sql_lab.py | 13 +- tests/unit_tests/models/helpers_test.py | 233 ++++++++++++++++++++++++++++++-- 4 files changed, 263 insertions(+), 20 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 050eee5e5a7..b012c8756ba 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -1598,7 +1598,7 @@ class SqlaTable( col: AdhocColumn, force_type_check: bool = False, template_processor: BaseTemplateProcessor | None = None, - ) -> ColumnElement: + ) -> tuple[ColumnElement, utils.GenericDataType | None]: """ Turn an adhoc column into a sqlalchemy column. @@ -1607,8 +1607,13 @@ class SqlaTable( This is needed to validate if a filter with an adhoc column is applicable. :param template_processor: template_processor instance - :returns: The metric defined as a sqlalchemy column - :rtype: sqlalchemy.sql.column + :returns: A tuple of (SQLAlchemy column, generic column type). The + generic type is populated when the column type is resolved + (either because the adhoc column matches a physical column, or + because ``force_type_check`` triggered a DB probe); otherwise + ``None``. Callers use it to coerce filter values to the correct + Python type (e.g. numeric casts for numeric adhoc expressions). + :rtype: tuple[sqlalchemy.sql.ColumnElement, Optional[GenericDataType]] """ label = utils.get_column_name(col) sql_expression = col["sqlExpression"] @@ -1617,6 +1622,7 @@ class SqlaTable( is_dttm = False pdf = None is_column_reference = col.get("isColumnReference", False) + generic_type: utils.GenericDataType | None = None # First, check if this is a column reference that exists in metadata if col_in_metadata := self.get_column(sql_expression): @@ -1626,6 +1632,7 @@ class SqlaTable( ) is_dttm = col_in_metadata.is_temporal pdf = col_in_metadata.python_date_format + generic_type = col_in_metadata.type_generic else: # Column doesn't exist in metadata or is not a reference - treat as ad-hoc # expression Note: If isColumnReference=true but column not found, we still @@ -1681,6 +1688,12 @@ class SqlaTable( if not col_desc: raise SupersetGenericDBErrorException("Column not found") is_dttm = col_desc[0]["is_dttm"] # type: ignore + # ResultSet already resolves the generic type from the + # driver's cursor.description; reuse it so callers can + # coerce filter values correctly (e.g. numeric IN-lists + # stay unquoted for numeric adhoc expressions like + # CAST(... AS BIGINT)). + generic_type = col_desc[0].get("type_generic") except SupersetGenericDBErrorException as ex: raise ColumnNotFoundException(message=str(ex)) from ex @@ -1690,7 +1703,7 @@ class SqlaTable( pdf=pdf, time_grain=time_grain, ) - return self.make_sqla_column_compatible(sqla_column, label) + return self.make_sqla_column_compatible(sqla_column, label), generic_type def _get_series_orderby( self, diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 3364e589fd4..d0db5812776 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -2275,7 +2275,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods col: "AdhocColumn", # type: ignore # noqa: F821 force_type_check: bool = False, template_processor: Optional[BaseTemplateProcessor] = None, - ) -> ColumnElement: + ) -> tuple[ColumnElement, Optional[GenericDataType]]: raise NotImplementedError() def _get_top_groups( @@ -2826,7 +2826,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods None, ) if adhoc_col: - col = self.adhoc_column_to_sqla( + col, _unused = self.adhoc_column_to_sqla( col=adhoc_col, template_processor=template_processor, ) @@ -2877,7 +2877,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods outer = literal_column(f"({selected})") outer = self.make_sqla_column_compatible(outer, selected) else: - outer = self.adhoc_column_to_sqla( + outer, _unused = self.adhoc_column_to_sqla( col=selected, template_processor=template_processor, ) @@ -3005,6 +3005,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods op = utils.FilterOperator(flt["op"].upper()) col_obj: Optional["TableColumn"] = None sqla_col: Optional[Column] = None + adhoc_generic_type: Optional[GenericDataType] = None is_metric_filter = ( False # Track if this is a filter on a metric (needs HAVING clause) ) @@ -3012,7 +3013,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods col_obj = dttm_col elif is_adhoc_column(flt_col): try: - sqla_col = self.adhoc_column_to_sqla( + sqla_col, adhoc_generic_type = self.adhoc_column_to_sqla( flt_col, force_type_check=True, template_processor=template_processor, @@ -3088,6 +3089,13 @@ class ExploreMixin: # pylint: disable=too-many-public-methods if col_spec and not col_advanced_data_type: target_generic_type = col_spec.generic_type + elif adhoc_generic_type is not None and not col_advanced_data_type: + # Adhoc columns have no TableColumn metadata; fall back to + # the generic type resolved by adhoc_column_to_sqla so + # filter values get coerced to match the SQL expression + # (e.g. numeric IN-lists stay unquoted when the expression + # casts to BIGINT). + target_generic_type = adhoc_generic_type else: target_generic_type = GenericDataType.STRING eq = self.filter_values_handler( diff --git a/superset/models/sql_lab.py b/superset/models/sql_lab.py index 382b272af42..447439a18b8 100644 --- a/superset/models/sql_lab.py +++ b/superset/models/sql_lab.py @@ -71,6 +71,7 @@ from superset.sqllab.limiting_factor import LimitingFactor from superset.superset_typing import ExplorableData, QueryObjectDict from superset.utils import json from superset.utils.core import ( + GenericDataType, get_column_name, LongText, MediumText, @@ -414,13 +415,15 @@ class Query( col: "AdhocColumn", # type: ignore # noqa: F821 force_type_check: bool = False, template_processor: Optional[BaseTemplateProcessor] = None, - ) -> ColumnElement: + ) -> tuple[ColumnElement, Optional[GenericDataType]]: """ Turn an adhoc column into a sqlalchemy column. :param col: Adhoc column definition :param template_processor: template_processor instance - :returns: The metric defined as a sqlalchemy column - :rtype: sqlalchemy.sql.column + :returns: A tuple of (SQLAlchemy column, generic column type). The + generic type is resolved from query result column metadata when + the adhoc label matches a known column; otherwise ``None``. + :rtype: tuple[sqlalchemy.sql.ColumnElement, Optional[GenericDataType]] """ label = get_column_name(col) expression = self._process_sql_expression( @@ -431,7 +434,9 @@ class Query( template_processor=template_processor, ) sqla_column = literal_column(expression) - return self.make_sqla_column_compatible(sqla_column, label) + col_meta = next((c for c in self.columns if c.column_name == label), None) + generic_type = col_meta.type_generic if col_meta else None + return self.make_sqla_column_compatible(sqla_column, label), generic_type class SavedQuery( diff --git a/tests/unit_tests/models/helpers_test.py b/tests/unit_tests/models/helpers_test.py index c2a32416c2e..d9aac9f80d5 100644 --- a/tests/unit_tests/models/helpers_test.py +++ b/tests/unit_tests/models/helpers_test.py @@ -31,6 +31,7 @@ from sqlalchemy.pool import StaticPool from sqlalchemy.sql.elements import ColumnElement from superset.superset_typing import AdhocColumn +from superset.utils.core import GenericDataType if TYPE_CHECKING: from superset.models.core import Database @@ -1419,10 +1420,12 @@ def test_adhoc_column_to_sqla_with_column_reference(database: Database) -> None: "isColumnReference": True, } - result = table.adhoc_column_to_sqla(col_with_spaces) + result, generic_type = table.adhoc_column_to_sqla(col_with_spaces) # Should return a valid SQLAlchemy column assert result is not None + # TEXT column should resolve to STRING generic type + assert generic_type == GenericDataType.STRING result_str = str(result) # The column name should be present (may or may not be quoted depending on dialect) @@ -1470,9 +1473,11 @@ def test_adhoc_column_to_sqla_preserves_column_type_for_time_grain( } # Should not raise ColumnNotFoundException - result = table.adhoc_column_to_sqla(date_col) + result, generic_type = table.adhoc_column_to_sqla(date_col) assert result is not None + # DATE column maps to the TEMPORAL generic type + assert generic_type == GenericDataType.TEMPORAL result_str = str(result) # Verify the column name is present (may be quoted depending on dialect) @@ -1513,7 +1518,7 @@ def test_adhoc_column_to_sqla_with_temporal_column_types(database: Database) -> "columnType": "BASE_AXIS", } - result = table.adhoc_column_to_sqla(time_col) + result, _ = table.adhoc_column_to_sqla(time_col) assert result is not None result_str = str(result) @@ -1611,7 +1616,7 @@ def test_adhoc_column_with_spaces_generates_quoted_sql(database: Database) -> No "isColumnReference": True, } - result = table.adhoc_column_to_sqla(col_with_spaces) + result, _ = table.adhoc_column_to_sqla(col_with_spaces) # Compile the column to SQL to see how it's rendered with database.get_sqla_engine() as engine: @@ -1632,7 +1637,7 @@ def test_adhoc_column_with_spaces_generates_quoted_sql(database: Database) -> No "isColumnReference": True, } - result_numeric = table.adhoc_column_to_sqla(col_numeric) + result_numeric, _ = table.adhoc_column_to_sqla(col_numeric) with database.get_sqla_engine() as engine: sql_numeric = str( @@ -1681,8 +1686,8 @@ def test_adhoc_column_with_spaces_in_full_query(database: Database) -> None: } # Get SQLAlchemy columns - customer_sqla = table.adhoc_column_to_sqla(customer_col) - order_sqla = table.adhoc_column_to_sqla(order_col) + customer_sqla, _ = table.adhoc_column_to_sqla(customer_col) + order_sqla, _ = table.adhoc_column_to_sqla(order_col) # Build a full query tbl = table.get_sqla_table() @@ -1912,7 +1917,7 @@ def _run_probe( ), patch.object(spec_cls, "type_probe_needs_row", type_probe_needs_row), ): - result = table.adhoc_column_to_sqla(adhoc_col) + result, _ = table.adhoc_column_to_sqla(adhoc_col) assert result is not None assert len(captured) == 1, "Expected exactly one type-probe query" @@ -2011,3 +2016,215 @@ def test_filter_by_verbose_name_resolves_to_column( assert "WHERE" in sql, f"Expected WHERE clause, got SQL: {sql}" assert "country_code" in sql, f"Expected filter on 'country_code', got SQL: {sql}" assert "'US'" in sql, f"Expected filter value 'US', got SQL: {sql}" + + +def _adhoc_col_with_probed_type( + database: Database, + probed_type: str, + probed_is_dttm: bool = False, +) -> "GenericDataType | None": + """ + Run adhoc_column_to_sqla against a mocked DB probe that returns + ``probed_type``, and return the resolved generic type. + + Uses an expression (not a bare column name) so the metadata-lookup branch + is skipped and the probe branch runs. + """ + from superset.connectors.sqla.models import SqlaTable, TableColumn + + table = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[TableColumn(column_name="pending_duration_seconds", type="BIGINT")], + ) + adhoc_col: AdhocColumn = { + "sqlExpression": "CAST(FLOOR(pending_duration_seconds / 86400) AS BIGINT)", + "label": "Pending Days", + } + + def fake_get_columns_description( + db: object, + catalog: object, + schema: object, + sql: str, + ) -> list[dict[str, object]]: + """Return a single-column description mirroring SupersetResultSet output.""" + # Mirror what SupersetResultSet.columns builds: it derives type_generic + # from the native type via db_engine_spec.get_column_spec(). + spec = table.db_engine_spec.get_column_spec(native_type=probed_type) + type_generic = spec.generic_type if spec else None + return [ + { + "column_name": "Pending Days", + "name": "Pending Days", + "type": probed_type, + "type_generic": type_generic, + "is_dttm": probed_is_dttm, + } + ] + + with patch( + "superset.connectors.sqla.models.get_columns_description", + side_effect=fake_get_columns_description, + ): + _, generic_type = table.adhoc_column_to_sqla( + adhoc_col, + force_type_check=True, + ) + return generic_type + + +def test_adhoc_column_to_sqla_returns_numeric_type_from_probe( + database: Database, +) -> None: + """ + Regression: when the adhoc SQL expression casts to a numeric type, + adhoc_column_to_sqla must surface the NUMERIC generic type from the DB + probe so downstream filter-value coercion unquotes numeric values. + """ + assert _adhoc_col_with_probed_type(database, "BIGINT") == GenericDataType.NUMERIC + + +def test_adhoc_column_to_sqla_returns_string_type_from_probe( + database: Database, +) -> None: + """String-typed adhoc expressions should report the STRING generic type.""" + assert _adhoc_col_with_probed_type(database, "VARCHAR") == GenericDataType.STRING + + +def test_adhoc_column_to_sqla_skips_probe_when_not_forced( + database: Database, +) -> None: + """ + Without ``force_type_check`` and no time grain, the probe is skipped and + the returned generic type is None (there's no cheap way to infer it). + """ + from superset.connectors.sqla.models import SqlaTable, TableColumn + + table = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[TableColumn(column_name="a", type="INTEGER")], + ) + adhoc_col: AdhocColumn = { + "sqlExpression": "CAST(a AS BIGINT)", + "label": "a_bigint", + } + + def _should_not_be_called(*_: object, **__: object) -> list[dict[str, object]]: + """Sentinel that fails if the DB probe is unexpectedly triggered.""" + raise AssertionError("probe should not run without force_type_check") + + with patch( + "superset.connectors.sqla.models.get_columns_description", + side_effect=_should_not_be_called, + ): + _, generic_type = table.adhoc_column_to_sqla(adhoc_col) + + assert generic_type is None + + +def test_adhoc_column_to_sqla_returns_type_from_column_metadata( + database: Database, +) -> None: + """ + When the adhoc ``sqlExpression`` resolves to a real column in the dataset, + the returned generic type comes from that column's ``type`` — not from a + DB probe. + """ + from superset.connectors.sqla.models import SqlaTable, TableColumn + + table = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[TableColumn(column_name="amount", type="BIGINT")], + ) + adhoc_col: AdhocColumn = { + "sqlExpression": "amount", + "label": "amount", + "isColumnReference": True, + } + + def _should_not_be_called(*_: object, **__: object) -> list[dict[str, object]]: + """Sentinel that fails if the DB probe runs despite metadata resolution.""" + raise AssertionError( + "probe should not run when the column resolves from metadata" + ) + + with patch( + "superset.connectors.sqla.models.get_columns_description", + side_effect=_should_not_be_called, + ): + _, generic_type = table.adhoc_column_to_sqla( + adhoc_col, + force_type_check=True, + ) + + assert generic_type == GenericDataType.NUMERIC + + +def test_numeric_adhoc_filter_value_is_unquoted_in_where_clause( + database: Database, +) -> None: + """ + End-to-end regression for the cross-filter bug: an IN-filter on an adhoc + numeric expression must render the value as an int (``IN (3)``) rather + than a string (``IN ('3')``). + + Before the fix, filter value coercion defaulted to STRING for adhoc + columns because the filter path could not discover the column's type. + """ + from superset.connectors.sqla.models import SqlaTable, TableColumn + + table = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[TableColumn(column_name="pending_duration_seconds", type="BIGINT")], + ) + adhoc_col: AdhocColumn = { + "sqlExpression": ( + "CAST(FLOOR(pending_duration_seconds / (60 * 60 * 24)) AS BIGINT)" + ), + "label": "Pending Days", + } + + def fake_get_columns_description( + *_: object, **__: object + ) -> list[dict[str, object]]: + """Return a BIGINT column description for the numeric adhoc expression.""" + return [ + { + "column_name": "Pending Days", + "name": "Pending Days", + "type": "BIGINT", + "type_generic": GenericDataType.NUMERIC, + "is_dttm": False, + } + ] + + with patch( + "superset.connectors.sqla.models.get_columns_description", + side_effect=fake_get_columns_description, + ): + sqla_query = table.get_sqla_query( + columns=["pending_duration_seconds"], + filter=[{"col": adhoc_col, "op": "IN", "val": ["3"]}], + is_timeseries=False, + row_limit=10, + ) + + with database.get_sqla_engine() as engine: + sql = str( + sqla_query.sqla_query.compile( + dialect=engine.dialect, + compile_kwargs={"literal_binds": True}, + ) + ) + + # The numeric value should be emitted without quotes. + assert "IN (3)" in sql, f"Expected numeric IN-list, got SQL: {sql}" + assert "IN ('3')" not in sql, f"Value should not be quoted, got SQL: {sql}"
