You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/10/15 02:58:10 UTC
(superset) branch fix-negated-filters-keep-null-rows created (now 5fe82a3f840)
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a change to branch fix-negated-filters-keep-null-rows
in repository https://gitbox.apache.org/repos/asf/superset.git
at 5fe82a3f840 fix(models): keep NULL rows in !=, NOT LIKE and NOT ILIKE filters
This branch includes the following new commits:
new 7402f1b5db8 test(models): guard NULL handling of !=, NOT LIKE and NOT ILIKE filters
new 6e40fe22e52 test(models): extend NULL-handling guards to arrays, HAVING, engine hooks and advanced types
new 0a641ca2901 test(models): guard custom plugin NULL operands, raw array operands and move guards off shared tail positions
new 5fe82a3f840 fix(models): keep NULL rows in !=, NOT LIKE and NOT ILIKE filters
The 4 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 03/04: test(models): guard custom plugin NULL operands, raw array operands and move guards off shared tail positions
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-negated-filters-keep-null-rows
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 0a641ca2901515b27e830bf15b3c6a21fb235b27
Author: Claude <no...@anthropic.com>
AuthorDate: Thu Oct 15 01:48:16 2026 +0000
test(models): guard custom plugin NULL operands, raw array operands and move guards off shared tail positions
Add a custom advanced data type whose negated filters already translate an
explicit NULL operand to IS NOT NULL: those translations must stay unwrapped,
while the same type given a value must keep NULL rows. Add raw (non-string)
array operands, including empty and zero-element arrays, for != and NOT IN.
Place the new test blocks away from the end of the shared test modules.
Co-Authored-By: Claude Sonnet 5.5 <no...@anthropic.com>
---
.../mcp_service/test_dashboard_scope_sql.py | 48 +-
tests/unit_tests/models/helpers_test.py | 1559 +++++++++++---------
tests/unit_tests/models/test_multivalue_filter.py | 38 +
3 files changed, 897 insertions(+), 748 deletions(-)
diff --git a/tests/unit_tests/mcp_service/test_dashboard_scope_sql.py b/tests/unit_tests/mcp_service/test_dashboard_scope_sql.py
index 7b7e2776e19..6ab12384c6b 100644
--- a/tests/unit_tests/mcp_service/test_dashboard_scope_sql.py
+++ b/tests/unit_tests/mcp_service/test_dashboard_scope_sql.py
@@ -163,6 +163,30 @@ def test_aggregate_is_computed_over_filtered_rows(
assert run(warehouse, scoped(database, sql)) == [(30,)]
+def test_not_equals_keeps_null_rows_like_the_dataset_path(
+ database: Any, warehouse: sa.engine.Engine
+) -> None:
+ """``client != 'A'`` excludes only A: B (300) and the NULL client (1000) stay."""
+ not_a = DashboardConstraints(
+ ({"col": "client", "op": "!=", "val": "A"},), None, None
+ )
+ assert run(
+ warehouse, scoped(database, "SELECT SUM(amount) FROM orders", not_a)
+ ) == [(1300,)]
+
+
+def test_not_equals_null_operand_stays_is_not_null(
+ database: Any, warehouse: sa.engine.Engine
+) -> None:
+ """A NULL operand still means IS NOT NULL: the NULL client row is excluded."""
+ not_null = DashboardConstraints(
+ ({"col": "client", "op": "!=", "val": None},), None, None
+ )
+ assert run(
+ warehouse, scoped(database, "SELECT SUM(amount) FROM orders", not_null)
+ ) == [(330,)]
+
+
@pytest.mark.parametrize(
"sql",
[
@@ -338,30 +362,6 @@ def test_null_aware_in_list_matches_the_dataset_path(
) == [(1030,)]
-def test_not_equals_keeps_null_rows_like_the_dataset_path(
- database: Any, warehouse: sa.engine.Engine
-) -> None:
- """``client != 'A'`` excludes only A: B (300) and the NULL client (1000) stay."""
- not_a = DashboardConstraints(
- ({"col": "client", "op": "!=", "val": "A"},), None, None
- )
- assert run(
- warehouse, scoped(database, "SELECT SUM(amount) FROM orders", not_a)
- ) == [(1300,)]
-
-
-def test_not_equals_null_operand_stays_is_not_null(
- database: Any, warehouse: sa.engine.Engine
-) -> None:
- """A NULL operand still means IS NOT NULL: the NULL client row is excluded."""
- not_null = DashboardConstraints(
- ({"col": "client", "op": "!=", "val": None},), None, None
- )
- assert run(
- warehouse, scoped(database, "SELECT SUM(amount) FROM orders", not_null)
- ) == [(330,)]
-
-
def test_time_range_is_applied_to_the_resolved_datetime_column(
database: Any, warehouse: sa.engine.Engine
) -> None:
diff --git a/tests/unit_tests/models/helpers_test.py b/tests/unit_tests/models/helpers_test.py
index 470cc9a4b2d..5d083a32a20 100644
--- a/tests/unit_tests/models/helpers_test.py
+++ b/tests/unit_tests/models/helpers_test.py
@@ -5655,887 +5655,998 @@ def test_like_filter_on_string_column_does_not_cast(database: Database) -> None:
)
-def test_get_sqla_query_calculated_column_inlined_in_raw_records(
- database: Database,
-) -> None:
- """Regression test for #34784: a calculated column selected as an adhoc
- column in a raw-records query (no groupby/metrics) must have its stored
- SQL expression inlined, not emitted as a bare column reference."""
- from superset.connectors.sqla.models import SqlaTable, TableColumn
- from superset.models.helpers import SqlaQuery
+# Rows used by the NULL-handling tests for negated filters. ``state`` is the
+# column that is selected back out; the other columns are the filter targets for
+# the column-type cases (``ip`` and ``port`` carry the advanced data types). The
+# last row is NULL in every filtered column.
+_NEGATION_COLUMNS = ("id", "state", "n", "flag", "ts", "uid", "ip", "port")
+_NEGATION_ROWS = [
+ (
+ 1,
+ "CA",
+ 1,
+ 1,
+ "2024-01-01 00:00:00",
+ "aaaaaaaa-0000-0000-0000-000000000001",
+ 16843009, # 1.1.1.1
+ 80,
+ ),
+ (
+ 2,
+ "NY",
+ 2,
+ 0,
+ "2024-02-01 00:00:00",
+ "bbbbbbbb-0000-0000-0000-000000000002",
+ 3232235777, # 192.168.1.1
+ 443,
+ ),
+ (
+ 3,
+ "TX",
+ 3,
+ 0,
+ "2024-03-01 00:00:00",
+ "cccccccc-0000-0000-0000-000000000003",
+ 3232236033, # 192.168.2.1
+ 8080,
+ ),
+ (4, None, None, None, None, None, None, None),
+]
- table: SqlaTable = SqlaTable(
+
+def _negation_table(database: Database) -> SqlaTable:
+ """Create ``negation_t`` holding the ``_NEGATION_ROWS`` and register it."""
+ from sqlalchemy import text
+
+ from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn
+
+ with database.get_sqla_engine() as engine:
+ with engine.begin() as connection:
+ connection.execute(text("DROP TABLE IF EXISTS negation_t"))
+ connection.execute(
+ text(
+ "CREATE TABLE negation_t (id INTEGER, state TEXT, n INTEGER, "
+ "flag BOOLEAN, ts TIMESTAMP, uid UUID, ip INTEGER, port INTEGER, "
+ "state_copy TEXT)"
+ )
+ )
+ for row in _NEGATION_ROWS:
+ connection.execute(
+ text(
+ "INSERT INTO negation_t "
+ "(id, state, n, flag, ts, uid, ip, port) "
+ "VALUES (:id, :state, :n, :flag, :ts, :uid, :ip, :port)"
+ ),
+ dict(zip(_NEGATION_COLUMNS, row, strict=True)),
+ )
+ connection.execute(text("UPDATE negation_t SET state_copy = state"))
+
+ return SqlaTable(
database=database,
schema=None,
- table_name="t",
+ table_name="negation_t",
columns=[
- TableColumn(column_name="a", type="INTEGER"),
- TableColumn(column_name="b", type="TEXT"),
+ TableColumn(column_name="id", type="INTEGER"),
+ TableColumn(column_name="state", type="TEXT"),
+ TableColumn(column_name="n", type="INTEGER"),
+ TableColumn(column_name="flag", type="BOOLEAN"),
+ TableColumn(column_name="ts", type="TIMESTAMP", is_dttm=True),
+ TableColumn(column_name="uid", type="UUID"),
TableColumn(
- column_name="name_test",
+ column_name="ip", type="INTEGER", advanced_data_type="internet_address"
+ ),
+ TableColumn(column_name="port", type="INTEGER", advanced_data_type="port"),
+ TableColumn(
+ column_name="state_copy",
type="TEXT",
- expression="CASE WHEN a > 0 THEN 'positive' ELSE 'non-positive' END",
+ advanced_data_type="null_aware_type",
+ ),
+ TableColumn(
+ column_name="upper_state", type="TEXT", expression="UPPER(state)"
),
],
+ metrics=[SqlMetric(metric_name="sum_n", expression="SUM(n)")],
)
- # No metrics/groupby => the ``elif columns:`` branch runs.
- adhoc_col: AdhocColumn = {
- "sqlExpression": "name_test",
- "label": "name_test",
- "isColumnReference": True,
- }
- sqlaq: SqlaQuery = table.get_sqla_query(
- columns=[adhoc_col],
- is_timeseries=False,
- row_limit=10,
- )
+def _run_negation_query(
+ database: Database, table: SqlaTable, flt: QueryObjectFilterClause, **kwargs: Any
+) -> tuple[str, set[str | None]]:
+ """
+ Build ``flt`` with the real ``get_sqla_query``, compile it and run it on
+ SQLite. Returns the compiled SQL and the ``state`` values of the surviving
+ rows (``None`` for the all-NULL row).
+ """
+ query = table.get_sqla_query(
+ **{
+ "columns": ["state"],
+ "metrics": [],
+ "extras": {},
+ "filter": [flt],
+ "granularity": None,
+ "is_timeseries": False,
+ **kwargs,
+ }
+ )
+ sql = database.compile_sqla_query(query.sqla_query)
with database.get_sqla_engine() as engine:
- sql: str = str(
- sqlaq.sqla_query.compile(
- dialect=engine.dialect,
- compile_kwargs={"literal_binds": True},
- )
- )
+ with engine.connect() as connection:
+ return sql, {row[0] for row in connection.exec_driver_sql(sql).fetchall()}
- # The stored expression must be inlined rather than emitted as a bare
- # ``name_test`` reference.
- assert "CASE WHEN a > 0" in sql
- assert "'positive'" in sql
- assert "'non-positive'" in sql
+
+def _states_matching_filter(
+ database: Database, flt: QueryObjectFilterClause
+) -> set[str | None]:
+ """The ``state`` values of the ``_NEGATION_ROWS`` that survive ``flt``."""
+ return _run_negation_query(database, _negation_table(database), flt)[1]
-def test_values_for_column_uses_read_limit_retry(
- mocker: MockerFixture,
- database: Database,
+@pytest.mark.parametrize(
+ "flt",
+ [
+ pytest.param({"col": "state", "op": "!=", "val": "CA"}, id="not-equals-text"),
+ pytest.param(
+ {"col": "state", "op": "NOT LIKE", "val": "%C%"}, id="not-like-text"
+ ),
+ pytest.param(
+ {"col": "state", "op": "NOT ILIKE", "val": "%c%"}, id="not-ilike-text"
+ ),
+ ],
+)
+def test_negated_filter_keeps_null_rows(
+ database: Database, flt: QueryObjectFilterClause
) -> None:
"""
- Physical-table datasets run filter-value SQL through the database
- read-limit retry so engines like ClickHouse can bound the read when the
- engine rejects it.
- """
- import pandas as pd
-
- from superset.connectors.sqla.models import SqlaTable, TableColumn
-
- table = SqlaTable(
- database=database,
- schema=None,
- table_name="t",
- columns=[TableColumn(column_name="a")],
- )
-
- retry = mocker.patch.object(
- database,
- "run_with_sampling_read_limit_retry",
- side_effect=lambda sql, run: run(sql),
- )
- with patch(
- "pandas.read_sql_query",
- return_value=pd.DataFrame({"column_values": [1]}),
- ):
- table.values_for_column("a")
+ ``!=``, ``NOT LIKE`` and ``NOT ILIKE`` only exclude the rows they name.
- retry.assert_called_once()
+ In SQL ``NULL != 'CA'`` is not true, so a bare ``state != 'CA'`` would also
+ drop the row whose ``state`` is NULL. A negated filter must keep it.
+ """
+ assert _states_matching_filter(database, flt) == {"NY", "TX", None}
-def test_values_for_column_virtual_dataset_skips_read_limit_retry(
- mocker: MockerFixture,
- database: Database,
+@pytest.mark.parametrize(
+ "flt",
+ [
+ pytest.param({"col": "n", "op": "!=", "val": 1}, id="not-equals-numeric"),
+ pytest.param({"col": "n", "op": "!=", "val": "1"}, id="not-equals-numeric-str"),
+ pytest.param({"col": "flag", "op": "!=", "val": True}, id="not-equals-boolean"),
+ pytest.param(
+ {"col": "ts", "op": "!=", "val": "2024-01-01 00:00:00"},
+ id="not-equals-temporal",
+ ),
+ pytest.param(
+ {"col": "upper_state", "op": "!=", "val": "CA"},
+ id="not-equals-calculated-column",
+ ),
+ pytest.param(
+ {
+ "col": {
+ "expressionType": "SQL",
+ "sqlExpression": "UPPER(state)",
+ "label": "upper_state_expr",
+ },
+ "op": "!=",
+ "val": "CA",
+ },
+ id="not-equals-adhoc-sql-column",
+ ),
+ pytest.param(
+ {"col": "n", "op": "NOT LIKE", "val": "1%"},
+ id="not-like-numeric-cast-to-string",
+ ),
+ pytest.param(
+ {"col": "n", "op": "NOT ILIKE", "val": "1%"},
+ id="not-ilike-numeric-cast-to-string",
+ ),
+ pytest.param(
+ {"col": "uid", "op": "NOT LIKE", "val": "aaaaaaaa%"},
+ id="not-like-uuid-cast-to-string",
+ ),
+ pytest.param(
+ {"col": "uid", "op": "NOT ILIKE", "val": "AAAAAAAA%"},
+ id="not-ilike-uuid-cast-to-string",
+ ),
+ ],
+)
+def test_negated_filter_keeps_null_rows_across_column_types(
+ database: Database, flt: QueryObjectFilterClause
) -> None:
"""
- Virtual datasets embed user-authored SQL and must stay governed by
- operator read limits.
+ The NULL row is kept for numeric, boolean, temporal, UUID, calculated and
+ ad-hoc SQL expression columns, including the LIKE paths that cast the column
+ to a string first.
"""
- import pandas as pd
-
- from superset.connectors.sqla.models import SqlaTable, TableColumn
+ assert _states_matching_filter(database, flt) == {"NY", "TX", None}
- table = SqlaTable(
- database=database,
- schema=None,
- table_name="virtual_t",
- sql="SELECT a FROM t",
- columns=[TableColumn(column_name="a")],
- )
- retry = mocker.patch.object(
- database,
- "run_with_sampling_read_limit_retry",
- side_effect=lambda sql, run: run(sql),
- )
- with patch(
- "pandas.read_sql_query",
- return_value=pd.DataFrame({"column_values": [1]}),
- ):
- table.values_for_column("a")
-
- retry.assert_not_called()
+@pytest.mark.parametrize(
+ "flt, expected",
+ [
+ pytest.param({"col": "state", "op": "==", "val": "CA"}, {"CA"}, id="equals"),
+ pytest.param({"col": "state", "op": "IN", "val": ["CA"]}, {"CA"}, id="in"),
+ pytest.param({"col": "state", "op": "LIKE", "val": "%C%"}, {"CA"}, id="like"),
+ pytest.param({"col": "state", "op": "ILIKE", "val": "%c%"}, {"CA"}, id="ilike"),
+ pytest.param(
+ {"col": "state", "op": "IS NULL", "val": None}, {None}, id="is-null"
+ ),
+ pytest.param(
+ {"col": "state", "op": "IS NOT NULL", "val": None},
+ {"CA", "NY", "TX"},
+ id="is-not-null",
+ ),
+ pytest.param(
+ {"col": "n", "op": ">", "val": 1}, {"NY", "TX"}, id="greater-than"
+ ),
+ pytest.param(
+ {"col": "state", "op": "NOT IN", "val": ["CA", None]},
+ {"NY", "TX"},
+ id="not-in-with-explicit-null-operand",
+ ),
+ pytest.param(
+ {"col": "state", "op": "!=", "val": None},
+ {"CA", "NY", "TX"},
+ id="not-equals-null-operand",
+ ),
+ pytest.param(
+ {"col": "state", "op": "!=", "val": "<NULL>"},
+ {"CA", "NY", "TX"},
+ id="not-equals-null-marker-operand",
+ ),
+ pytest.param(
+ {"col": "state", "op": "==", "val": None},
+ {None},
+ id="equals-null-operand",
+ ),
+ ],
+)
+def test_filters_with_unchanged_null_semantics(
+ database: Database, flt: QueryObjectFilterClause, expected: set[str | None]
+) -> None:
+ """
+ Operators that are not negated comparisons, and negated operators given an
+ explicit NULL operand, keep their NULL handling: ``!= NULL`` still means
+ ``IS NOT NULL`` and ``NOT IN (..., NULL)`` still excludes the NULL row.
+ """
+ assert _states_matching_filter(database, flt) == expected
-def test_get_query_str_extended_does_not_alter_system_sampling_sql(
- database: Database,
+@pytest.mark.parametrize(
+ "flt",
+ [
+ pytest.param({"col": "state", "op": "!=", "val": ""}, id="empty-string"),
+ pytest.param({"col": "n", "op": "!=", "val": 0}, id="zero"),
+ pytest.param({"col": "flag", "op": "!=", "val": False}, id="false"),
+ pytest.param({"col": "state", "op": "!=", "val": ["XX"]}, id="one-item-list"),
+ pytest.param(
+ {"col": "state", "op": "!=", "val": ["XX", "CA"]}, id="multi-item-list"
+ ),
+ ],
+)
+def test_negated_filter_with_falsy_or_list_operand_keeps_null_rows(
+ database: Database, flt: QueryObjectFilterClause
) -> None:
"""
- The bounded-read override is a retry-time concern; the generated (and
- user-visible) statement for a samples request must stay unmodified.
+ A falsy operand (``0``, ``False``, ``""``) is a real value, not an absent one,
+ so the NULL row is kept. A list operand for a scalar ``!=`` collapses to its
+ first item and is treated the same way.
"""
- from superset.connectors.sqla.models import SqlaTable, TableColumn
+ states = _states_matching_filter(database, flt)
+ assert None in states
- table = SqlaTable(
- database=database,
- schema=None,
- table_name="t",
- columns=[TableColumn(column_name="a")],
- )
- query_str_ext = table.get_query_str_extended(
- {
- "columns": ["a"],
- "extras": {"system_sampling": True},
- "is_timeseries": False,
- "row_limit": 10,
- }
- )
- assert "SETTINGS" not in query_str_ext.sql
+@pytest.mark.parametrize("val", [[], [None]], ids=["empty-list", "list-of-null"])
+def test_not_equals_with_empty_operand_stays_is_not_null(
+ database: Database, val: list[None]
+) -> None:
+ """A list operand with no usable item collapses to NULL and stays IS NOT NULL."""
+ flt = cast(QueryObjectFilterClause, {"col": "state", "op": "!=", "val": val})
+ assert _states_matching_filter(database, flt) == {"CA", "NY", "TX"}
-def test_query_system_sampling_uses_read_limit_retry(
- mocker: MockerFixture,
- database: Database,
-) -> None:
+def test_not_equals_on_metric_keeps_null_aggregates(database: Database) -> None:
"""
- The system_sampling extras marker (set by the samples query action)
- routes execution through the read-limit retry; ordinary chart queries
- are untouched.
+ A ``!=`` filter on a metric is applied in HAVING; a group whose aggregate is
+ NULL is kept alongside the groups with a different aggregate.
"""
- import pandas as pd
-
- from superset.connectors.sqla.models import SqlaTable, TableColumn
-
- table = SqlaTable(
- database=database,
- schema=None,
- table_name="t",
- columns=[TableColumn(column_name="a")],
- )
- retry = mocker.patch.object(
+ table = _negation_table(database)
+ # SUM(n) per state: CA=1, NY=2, TX=3, NULL state -> NULL
+ sql, states = _run_negation_query(
database,
- "run_with_sampling_read_limit_retry",
- side_effect=lambda sql, run: run(sql),
+ table,
+ {"col": "sum_n", "op": "!=", "val": 1},
+ columns=[],
+ groupby=["state"],
+ metrics=["sum_n"],
)
- mocker.patch.object(database, "get_df", return_value=pd.DataFrame({"a": [1]}))
+ assert "HAVING" in sql
+ assert states == {"NY", "TX", None}
- table.query(
- {
- "columns": ["a"],
- "extras": {"system_sampling": True},
- "is_timeseries": False,
- "row_limit": 10,
- }
- )
- retry.assert_called_once()
- retry.reset_mock()
- table.query({"columns": ["a"], "is_timeseries": False, "row_limit": 10})
- retry.assert_not_called()
+@pytest.mark.parametrize(
+ "flt, expected",
+ [
+ pytest.param(
+ {"col": "sum_n", "op": "==", "val": 1}, {"CA"}, id="equals-aggregate"
+ ),
+ pytest.param(
+ {"col": "sum_n", "op": "!=", "val": None},
+ {"CA", "NY", "TX"},
+ id="not-equals-null-operand",
+ ),
+ ],
+)
+def test_metric_filters_with_unchanged_null_semantics(
+ database: Database, flt: QueryObjectFilterClause, expected: set[str | None]
+) -> None:
+ """``==`` on a metric and ``!= NULL`` on a metric keep their behavior."""
+ _, states = _run_negation_query(
+ database,
+ _negation_table(database),
+ flt,
+ columns=[],
+ groupby=["state"],
+ metrics=["sum_n"],
+ )
+ assert states == expected
-def test_query_system_sampling_skips_virtual_dataset(
+@pytest.mark.parametrize(
+ "flt, null_target",
+ [
+ pytest.param(
+ {"col": "upper_state", "op": "!=", "val": "CA"},
+ r"my_isnull\(\(?UPPER\(state\)\)?\)",
+ id="not-equals-calculated-column",
+ ),
+ pytest.param(
+ {"col": "n", "op": "NOT LIKE", "val": "1%"},
+ r"my_isnull\(n\)",
+ id="not-like-checks-column-before-cast",
+ ),
+ pytest.param(
+ {"col": "n", "op": "NOT ILIKE", "val": "1%"},
+ r"my_isnull\(n\)",
+ id="not-ilike-checks-column-before-cast",
+ ),
+ pytest.param(
+ {"col": "state", "op": "!=", "val": "CA"},
+ r"my_isnull\(state\)",
+ id="not-equals-plain-column",
+ ),
+ ],
+)
+def test_negated_filter_null_check_uses_engine_spec_hook(
mocker: MockerFixture,
database: Database,
+ flt: QueryObjectFilterClause,
+ null_target: str,
) -> None:
"""
- Even sample requests do not receive the retry on virtual datasets.
+ The NULL check comes from the engine spec's ``handle_null_filter`` hook, so
+ engines with their own NULL syntax (e.g. KQL ``isnull(col)``) get it, and it
+ is applied to the filtered column itself rather than to its string cast.
"""
- import pandas as pd
+ from sqlalchemy import func
- from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.db_engine_specs.sqlite import SqliteEngineSpec
- table = SqlaTable(
- database=database,
- schema=None,
- table_name="virtual_t",
- sql="SELECT a FROM t",
- columns=[TableColumn(column_name="a")],
+ class NullFunctionEngineSpec(SqliteEngineSpec):
+ @classmethod
+ def handle_null_filter(cls, sqla_col: Any, op: FilterOperator) -> Any:
+ if op == FilterOperator.IS_NULL:
+ return func.my_isnull(sqla_col)
+ return super().handle_null_filter(sqla_col, op)
+
+ mocker.patch.object(
+ SqlaTable, "db_engine_spec", new=property(lambda self: NullFunctionEngineSpec)
)
- retry = mocker.patch.object(
- database,
- "run_with_sampling_read_limit_retry",
- side_effect=lambda sql, run: run(sql),
+ table = _negation_table(database)
+ query = table.get_sqla_query(
+ columns=["state"],
+ metrics=[],
+ extras={},
+ filter=[flt],
+ granularity=None,
+ is_timeseries=False,
)
- mocker.patch.object(database, "get_df", return_value=pd.DataFrame({"a": [1]}))
+ sql = database.compile_sqla_query(query.sqla_query)
- table.query(
- {
- "columns": ["a"],
- "extras": {"system_sampling": True},
- "is_timeseries": False,
- "row_limit": 10,
- }
- )
- retry.assert_not_called()
+ assert re.search(null_target, sql), sql
+ assert "IS NULL" not in sql.upper()
-def test_select_star_returns_unmodified_sql(
- mocker: MockerFixture,
+@pytest.fixture
+def advanced_data_types(mocker: MockerFixture) -> None:
+ mocker.patch(
+ "superset.models.helpers.feature_flag_manager.is_feature_enabled",
+ side_effect=lambda feature: feature == "ENABLE_ADVANCED_DATA_TYPES",
+ )
+
+
+@pytest.mark.usefixtures("advanced_data_types")
+@pytest.mark.parametrize(
+ "flt",
+ [
+ pytest.param({"col": "ip", "op": "!=", "val": "1.1.1.1"}, id="ip-not-equals"),
+ pytest.param({"col": "ip", "op": "NOT IN", "val": ["1.1.1.1"]}, id="ip-not-in"),
+ pytest.param({"col": "port", "op": "!=", "val": "80"}, id="port-not-equals"),
+ ],
+)
+def test_negated_advanced_data_type_filter_keeps_null_rows(
+ database: Database, flt: QueryObjectFilterClause
+) -> None:
+ """``!=`` / ``NOT IN`` on internet address and port columns keep NULL rows."""
+ states = _states_matching_filter(database, flt)
+ assert None in states
+ assert "CA" not in states
+
+
+@pytest.mark.usefixtures("advanced_data_types")
+@pytest.mark.parametrize(
+ "flt, expected_with_null",
+ [
+ pytest.param(
+ {"col": "ip", "op": "!=", "val": "192.168.1.0/24"},
+ {"CA", "TX", None},
+ id="ip-not-equals-cidr-range",
+ ),
+ pytest.param(
+ {"col": "port", "op": "NOT IN", "val": ["80", "443"]},
+ {"TX", None},
+ id="port-not-in-multiple",
+ ),
+ ],
+)
+def test_negated_advanced_data_type_filter_keeps_null_rows_with_ranges_and_lists(
database: Database,
+ flt: QueryObjectFilterClause,
+ expected_with_null: set[str | None],
) -> None:
- """
- select_star output is displayed in SQL Lab, returned by the API, and
- persisted as CTAS result-fetch SQL, so it must never carry the
- bounded-read override.
- """
- from superset.sql.parse import Table
+ """The CIDR-range ``!=`` and multi-value port ``NOT IN`` forms keep NULL rows."""
+ assert _states_matching_filter(database, flt) == expected_with_null
- retry_sql = mocker.patch.object(database, "sampling_read_limit_retry_sql")
- sql = database.select_star(Table("t"), limit=10, latest_partition=False)
- retry_sql.assert_not_called()
- assert "SETTINGS" not in sql
+@pytest.mark.usefixtures("advanced_data_types")
+def test_ip_not_in_cidr_range_keeps_null_row(database: Database) -> None:
+ """The NULL row survives a CIDR-range ``NOT IN``."""
+ flt: QueryObjectFilterClause = {
+ "col": "ip",
+ "op": "NOT IN",
+ "val": ["192.168.1.0/24"],
+ }
+ assert None in _states_matching_filter(database, flt)
-def test_adhoc_type_probe_does_not_get_sampling_retry(
- mocker: MockerFixture,
- database: Database,
+@pytest.mark.usefixtures("advanced_data_types")
+@pytest.mark.parametrize(
+ "flt, expected",
+ [
+ pytest.param(
+ {"col": "ip", "op": "==", "val": "1.1.1.1"}, {"CA"}, id="ip-equals"
+ ),
+ pytest.param({"col": "ip", "op": "IN", "val": ["1.1.1.1"]}, {"CA"}, id="ip-in"),
+ pytest.param(
+ {"col": "ip", "op": "==", "val": "192.168.1.0/24"},
+ {"NY"},
+ id="ip-equals-cidr-range",
+ ),
+ pytest.param(
+ {"col": "port", "op": "==", "val": "80"}, {"CA"}, id="port-equals"
+ ),
+ pytest.param(
+ {"col": "port", "op": "IN", "val": ["80", "443"]},
+ {"CA", "NY"},
+ id="port-in",
+ ),
+ ],
+)
+def test_advanced_data_type_filters_with_unchanged_null_semantics(
+ database: Database, flt: QueryObjectFilterClause, expected: set[str | None]
) -> None:
+ """Non-negated operators on advanced data type columns are unchanged."""
+ assert _states_matching_filter(database, flt) == expected
+
+
+@pytest.mark.usefixtures("advanced_data_types")
+def test_ip_not_in_cidr_range_non_null_rows_unchanged(database: Database) -> None:
"""
- The adhoc expression type probe is a zero-row WHERE FALSE query and must
- never be routed through the sampling read-limit retry.
+ Pins the current behavior for the non-NULL rows of a CIDR-range ``NOT IN``:
+ the range conjunction ``(col > end) AND (col < start)`` can never hold, so no
+ non-NULL row matches. Only the NULL row handling is meant to change.
"""
- from superset.connectors.sqla.models import SqlaTable, TableColumn
+ flt: QueryObjectFilterClause = {
+ "col": "ip",
+ "op": "NOT IN",
+ "val": ["192.168.1.0/24"],
+ }
+ assert _states_matching_filter(database, flt) - {None} == set()
- table = SqlaTable(
- database=database,
- schema=None,
- table_name="t",
- columns=[TableColumn(column_name="a", type="INTEGER")],
- )
- retry = mocker.patch.object(
- database,
- "run_with_sampling_read_limit_retry",
- side_effect=lambda sql, run: run(sql),
- )
- mocker.patch(
- "superset.connectors.sqla.models.get_columns_description",
- return_value=[{"is_dttm": False, "type_generic": GenericDataType.NUMERIC}],
+
+@pytest.fixture
+def null_aware_advanced_data_type(
+ mocker: MockerFixture, advanced_data_types: None
+) -> None:
+ """
+ Register a custom advanced data type whose translation understands NULL
+ operands: ``!= NULL`` / ``NOT IN (NULL)`` become ``IS NOT NULL`` (the plugin
+ request contract allows NULL values).
+ """
+ from flask import current_app
+ from sqlalchemy import and_
+
+ from superset.advanced_data_type.types import (
+ AdvancedDataType,
+ AdvancedDataTypeRequest,
+ AdvancedDataTypeResponse,
)
- adhoc_col: AdhocColumn = {
- "sqlExpression": "a + 1",
- "label": "probe_me",
- "columnType": "BASE_AXIS",
- "timeGrain": "P1D",
- }
- table.adhoc_column_to_sqla(adhoc_col)
+ def translate_type(req: AdvancedDataTypeRequest) -> AdvancedDataTypeResponse:
+ return {
+ "values": list(req["values"]),
+ "error_message": "",
+ "display_value": "",
+ "valid_filter_operators": [],
+ }
- retry.assert_not_called()
+ def translate_filter(col: Any, op: FilterOperator, values: list[Any]) -> Any:
+ non_null = [value for value in values if value is not None]
+ if op == FilterOperator.EQUALS:
+ return col == values[0]
+ if op == FilterOperator.IN:
+ return col.in_(non_null)
+ if op == FilterOperator.NOT_EQUALS:
+ return col.isnot(None) if values[0] is None else col != values[0]
+ if not non_null:
+ return col.isnot(None)
+ if len(non_null) < len(values):
+ return and_(col.isnot(None), ~col.in_(non_null))
+ return ~col.in_(non_null)
+
+ mocker.patch.dict(
+ current_app.config["ADVANCED_DATA_TYPES"],
+ {
+ "null_aware_type": AdvancedDataType(
+ verbose_name="null aware",
+ description="translates NULL operands itself",
+ valid_data_types=["str"],
+ translate_type=translate_type,
+ translate_filter=translate_filter,
+ )
+ },
+ )
-def test_filter_adhoc_column(database: Database) -> None:
+@pytest.mark.usefixtures("null_aware_advanced_data_type")
+@pytest.mark.parametrize(
+ "flt, expected",
+ [
+ pytest.param(
+ {"col": "state_copy", "op": "!=", "val": None},
+ {"CA", "NY", "TX"},
+ id="not-equals-null-operand",
+ ),
+ pytest.param(
+ {"col": "state_copy", "op": "NOT IN", "val": [None]},
+ {"CA", "NY", "TX"},
+ id="not-in-only-null",
+ ),
+ pytest.param(
+ {"col": "state_copy", "op": "NOT IN", "val": ["CA", None]},
+ {"NY", "TX"},
+ id="not-in-with-explicit-null",
+ ),
+ pytest.param(
+ {"col": "state_copy", "op": "==", "val": "CA"}, {"CA"}, id="equals"
+ ),
+ ],
+)
+def test_custom_advanced_data_type_explicit_null_translation_is_not_wrapped(
+ database: Database, flt: QueryObjectFilterClause, expected: set[str | None]
+) -> None:
"""
- Test that filter works with adhoc column labels.
- When filter contains a string that matches the label of an adhoc column
- in the columns list, it should correctly convert to a SQLAlchemy column
- instead of raising QueryObjectValidationError.
+ A plugin that translates an explicit NULL operand itself (``IS NOT NULL``) is
+ not wrapped with a keep-NULL disjunct, which would make it match every row.
"""
+ assert _states_matching_filter(database, flt) == expected
+
+
+@pytest.mark.usefixtures("null_aware_advanced_data_type")
+@pytest.mark.parametrize(
+ "flt",
+ [
+ pytest.param({"col": "state_copy", "op": "!=", "val": "CA"}, id="not-equals"),
+ pytest.param({"col": "state_copy", "op": "NOT IN", "val": ["CA"]}, id="not-in"),
+ ],
+)
+def test_custom_advanced_data_type_negation_with_value_keeps_null_rows(
+ database: Database, flt: QueryObjectFilterClause
+) -> None:
+ """The same plugin, given a non-NULL operand, gets its negation wrapped."""
+ assert _states_matching_filter(database, flt) == {"NY", "TX", None}
+
+
+def test_get_sqla_query_calculated_column_inlined_in_raw_records(
+ database: Database,
+) -> None:
+ """Regression test for #34784: a calculated column selected as an adhoc
+ column in a raw-records query (no groupby/metrics) must have its stored
+ SQL expression inlined, not emitted as a bare column reference."""
from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.models.helpers import SqlaQuery
- table = SqlaTable(
- table_name="test_table",
+ table: SqlaTable = SqlaTable(
database=database,
+ schema=None,
+ table_name="t",
columns=[
- TableColumn(column_name="name", type="TEXT"),
- TableColumn(column_name="real_name", type="TEXT"),
+ TableColumn(column_name="a", type="INTEGER"),
+ TableColumn(column_name="b", type="TEXT"),
+ TableColumn(
+ column_name="name_test",
+ type="TEXT",
+ expression="CASE WHEN a > 0 THEN 'positive' ELSE 'non-positive' END",
+ ),
],
)
- # Should not raise QueryObjectValidationError
- result = table.get_sqla_query(
- columns=[
- "name",
- {
- "expressionType": "SQL",
- "label": "full_name",
- "sqlExpression": "real_name",
- },
- ],
- orderby=[],
- metrics=[],
- extras={},
- filter=[
- {"col": "full_name", "op": "ILIKE", "val": "Zona%"}
- ], # Filter by adhoc column label
- granularity=None,
+ # No metrics/groupby => the ``elif columns:`` branch runs.
+ adhoc_col: AdhocColumn = {
+ "sqlExpression": "name_test",
+ "label": "name_test",
+ "isColumnReference": True,
+ }
+ sqlaq: SqlaQuery = table.get_sqla_query(
+ columns=[adhoc_col],
is_timeseries=False,
+ row_limit=10,
)
- assert result is not None
- # Verify the WHERE predicate uses the resolved adhoc expression and value.
with database.get_sqla_engine() as engine:
- sql = str(
- result.sqla_query.compile(
+ sql: str = str(
+ sqlaq.sqla_query.compile(
dialect=engine.dialect,
compile_kwargs={"literal_binds": True},
)
)
- assert "real_name AS full_name" in sql
- assert "WHERE" in sql
- # The adhoc column resolved by label is parenthesized in the WHERE clause,
- # consistent with inline adhoc columns, to guard operator precedence.
- assert "lower((real_name)) LIKE lower('Zona%')" in sql
+ # The stored expression must be inlined rather than emitted as a bare
+ # ``name_test`` reference.
+ assert "CASE WHEN a > 0" in sql
+ assert "'positive'" in sql
+ assert "'non-positive'" in sql
-def test_get_query_result_wraps_post_processing_type_error(
- database: "Database",
+def test_values_for_column_uses_read_limit_retry(
+ mocker: MockerFixture,
+ database: Database,
) -> None:
"""
- A raw TypeError from pandas inside exec_post_processing (e.g. resample.mean()
- on a DataFrame that contains object-dtype columns) must be surfaced as
- QueryObjectValidationError (400) rather than propagating as a system 500.
- """
- from datetime import timedelta
- from unittest.mock import patch
-
+ Physical-table datasets run filter-value SQL through the database
+ read-limit retry so engines like ClickHouse can bound the read when the
+ engine rejects it.
+ """
import pandas as pd
- from superset.common.query_object import QueryObject
- from superset.connectors.sqla.models import SqlaTable
- from superset.exceptions import QueryObjectValidationError
- from superset.models.helpers import QueryResult
-
- table = SqlaTable(table_name="t", database=database)
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
- # DatetimeIndex + object-dtype "category" column causes
- # df.resample("1D").mean() to raise TypeError in pandas ≥ 2.x
- df = pd.DataFrame(
- {"metric": [1.0, 2.0], "category": ["a", "b"]},
- index=pd.to_datetime(["2023-01-01", "2023-01-03"]),
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[TableColumn(column_name="a")],
)
- query_object = QueryObject(
- row_limit=10,
- post_processing=[
- {"operation": "resample", "options": {"method": "mean", "rule": "1D"}}
- ],
+ retry = mocker.patch.object(
+ database,
+ "run_with_sampling_read_limit_retry",
+ side_effect=lambda sql, run: run(sql),
)
-
- with (
- patch.object(
- table,
- "query",
- return_value=QueryResult(
- df=df,
- query="SELECT 1",
- duration=timedelta(0),
- sql_shifted_temporal_labels=set(),
- ),
- ),
- patch.object(table, "normalize_df", return_value=df),
- pytest.raises(QueryObjectValidationError),
+ with patch(
+ "pandas.read_sql_query",
+ return_value=pd.DataFrame({"column_values": [1]}),
):
- table.get_query_result(query_object)
-
-
-# Rows used by the NULL-handling tests for negated filters. ``state`` is the
-# column that is selected back out; the other columns are the filter targets for
-# the column-type cases (``ip`` and ``port`` carry the advanced data types). The
-# last row is NULL in every filtered column.
-_NEGATION_COLUMNS = ("id", "state", "n", "flag", "ts", "uid", "ip", "port")
-_NEGATION_ROWS = [
- (
- 1,
- "CA",
- 1,
- 1,
- "2024-01-01 00:00:00",
- "aaaaaaaa-0000-0000-0000-000000000001",
- 16843009, # 1.1.1.1
- 80,
- ),
- (
- 2,
- "NY",
- 2,
- 0,
- "2024-02-01 00:00:00",
- "bbbbbbbb-0000-0000-0000-000000000002",
- 3232235777, # 192.168.1.1
- 443,
- ),
- (
- 3,
- "TX",
- 3,
- 0,
- "2024-03-01 00:00:00",
- "cccccccc-0000-0000-0000-000000000003",
- 3232236033, # 192.168.2.1
- 8080,
- ),
- (4, None, None, None, None, None, None, None),
-]
+ table.values_for_column("a")
+ retry.assert_called_once()
-def _negation_table(database: Database) -> SqlaTable:
- """Create ``negation_t`` holding the ``_NEGATION_ROWS`` and register it."""
- from sqlalchemy import text
- from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn
+def test_values_for_column_virtual_dataset_skips_read_limit_retry(
+ mocker: MockerFixture,
+ database: Database,
+) -> None:
+ """
+ Virtual datasets embed user-authored SQL and must stay governed by
+ operator read limits.
+ """
+ import pandas as pd
- with database.get_sqla_engine() as engine:
- with engine.begin() as connection:
- connection.execute(text("DROP TABLE IF EXISTS negation_t"))
- connection.execute(
- text(
- "CREATE TABLE negation_t (id INTEGER, state TEXT, n INTEGER, "
- "flag BOOLEAN, ts TIMESTAMP, uid UUID, ip INTEGER, port INTEGER)"
- )
- )
- for row in _NEGATION_ROWS:
- connection.execute(
- text(
- "INSERT INTO negation_t VALUES "
- "(:id, :state, :n, :flag, :ts, :uid, :ip, :port)"
- ),
- dict(zip(_NEGATION_COLUMNS, row, strict=True)),
- )
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
- return SqlaTable(
+ table = SqlaTable(
database=database,
schema=None,
- table_name="negation_t",
- columns=[
- TableColumn(column_name="id", type="INTEGER"),
- TableColumn(column_name="state", type="TEXT"),
- TableColumn(column_name="n", type="INTEGER"),
- TableColumn(column_name="flag", type="BOOLEAN"),
- TableColumn(column_name="ts", type="TIMESTAMP", is_dttm=True),
- TableColumn(column_name="uid", type="UUID"),
- TableColumn(
- column_name="ip", type="INTEGER", advanced_data_type="internet_address"
- ),
- TableColumn(column_name="port", type="INTEGER", advanced_data_type="port"),
- TableColumn(
- column_name="upper_state", type="TEXT", expression="UPPER(state)"
- ),
- ],
- metrics=[SqlMetric(metric_name="sum_n", expression="SUM(n)")],
+ table_name="virtual_t",
+ sql="SELECT a FROM t",
+ columns=[TableColumn(column_name="a")],
)
-
-def _run_negation_query(
- database: Database, table: SqlaTable, flt: QueryObjectFilterClause, **kwargs: Any
-) -> tuple[str, set[str | None]]:
- """
- Build ``flt`` with the real ``get_sqla_query``, compile it and run it on
- SQLite. Returns the compiled SQL and the ``state`` values of the surviving
- rows (``None`` for the all-NULL row).
- """
- query = table.get_sqla_query(
- **{
- "columns": ["state"],
- "metrics": [],
- "extras": {},
- "filter": [flt],
- "granularity": None,
- "is_timeseries": False,
- **kwargs,
- }
+ retry = mocker.patch.object(
+ database,
+ "run_with_sampling_read_limit_retry",
+ side_effect=lambda sql, run: run(sql),
)
- sql = database.compile_sqla_query(query.sqla_query)
- with database.get_sqla_engine() as engine:
- with engine.connect() as connection:
- return sql, {row[0] for row in connection.exec_driver_sql(sql).fetchall()}
-
+ with patch(
+ "pandas.read_sql_query",
+ return_value=pd.DataFrame({"column_values": [1]}),
+ ):
+ table.values_for_column("a")
-def _states_matching_filter(
- database: Database, flt: QueryObjectFilterClause
-) -> set[str | None]:
- """The ``state`` values of the ``_NEGATION_ROWS`` that survive ``flt``."""
- return _run_negation_query(database, _negation_table(database), flt)[1]
+ retry.assert_not_called()
-@pytest.mark.parametrize(
- "flt",
- [
- pytest.param({"col": "state", "op": "!=", "val": "CA"}, id="not-equals-text"),
- pytest.param(
- {"col": "state", "op": "NOT LIKE", "val": "%C%"}, id="not-like-text"
- ),
- pytest.param(
- {"col": "state", "op": "NOT ILIKE", "val": "%c%"}, id="not-ilike-text"
- ),
- ],
-)
-def test_negated_filter_keeps_null_rows(
- database: Database, flt: QueryObjectFilterClause
+def test_get_query_str_extended_does_not_alter_system_sampling_sql(
+ database: Database,
) -> None:
"""
- ``!=``, ``NOT LIKE`` and ``NOT ILIKE`` only exclude the rows they name.
-
- In SQL ``NULL != 'CA'`` is not true, so a bare ``state != 'CA'`` would also
- drop the row whose ``state`` is NULL. A negated filter must keep it.
+ The bounded-read override is a retry-time concern; the generated (and
+ user-visible) statement for a samples request must stay unmodified.
"""
- assert _states_matching_filter(database, flt) == {"NY", "TX", None}
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[TableColumn(column_name="a")],
+ )
-@pytest.mark.parametrize(
- "flt",
- [
- pytest.param({"col": "n", "op": "!=", "val": 1}, id="not-equals-numeric"),
- pytest.param({"col": "n", "op": "!=", "val": "1"}, id="not-equals-numeric-str"),
- pytest.param({"col": "flag", "op": "!=", "val": True}, id="not-equals-boolean"),
- pytest.param(
- {"col": "ts", "op": "!=", "val": "2024-01-01 00:00:00"},
- id="not-equals-temporal",
- ),
- pytest.param(
- {"col": "upper_state", "op": "!=", "val": "CA"},
- id="not-equals-calculated-column",
- ),
- pytest.param(
- {
- "col": {
- "expressionType": "SQL",
- "sqlExpression": "UPPER(state)",
- "label": "upper_state_expr",
- },
- "op": "!=",
- "val": "CA",
- },
- id="not-equals-adhoc-sql-column",
- ),
- pytest.param(
- {"col": "n", "op": "NOT LIKE", "val": "1%"},
- id="not-like-numeric-cast-to-string",
- ),
- pytest.param(
- {"col": "n", "op": "NOT ILIKE", "val": "1%"},
- id="not-ilike-numeric-cast-to-string",
- ),
- pytest.param(
- {"col": "uid", "op": "NOT LIKE", "val": "aaaaaaaa%"},
- id="not-like-uuid-cast-to-string",
- ),
- pytest.param(
- {"col": "uid", "op": "NOT ILIKE", "val": "AAAAAAAA%"},
- id="not-ilike-uuid-cast-to-string",
- ),
- ],
-)
-def test_negated_filter_keeps_null_rows_across_column_types(
- database: Database, flt: QueryObjectFilterClause
-) -> None:
- """
- The NULL row is kept for numeric, boolean, temporal, UUID, calculated and
- ad-hoc SQL expression columns, including the LIKE paths that cast the column
- to a string first.
- """
- assert _states_matching_filter(database, flt) == {"NY", "TX", None}
+ query_str_ext = table.get_query_str_extended(
+ {
+ "columns": ["a"],
+ "extras": {"system_sampling": True},
+ "is_timeseries": False,
+ "row_limit": 10,
+ }
+ )
+ assert "SETTINGS" not in query_str_ext.sql
-@pytest.mark.parametrize(
- "flt, expected",
- [
- pytest.param({"col": "state", "op": "==", "val": "CA"}, {"CA"}, id="equals"),
- pytest.param({"col": "state", "op": "IN", "val": ["CA"]}, {"CA"}, id="in"),
- pytest.param({"col": "state", "op": "LIKE", "val": "%C%"}, {"CA"}, id="like"),
- pytest.param({"col": "state", "op": "ILIKE", "val": "%c%"}, {"CA"}, id="ilike"),
- pytest.param(
- {"col": "state", "op": "IS NULL", "val": None}, {None}, id="is-null"
- ),
- pytest.param(
- {"col": "state", "op": "IS NOT NULL", "val": None},
- {"CA", "NY", "TX"},
- id="is-not-null",
- ),
- pytest.param(
- {"col": "n", "op": ">", "val": 1}, {"NY", "TX"}, id="greater-than"
- ),
- pytest.param(
- {"col": "state", "op": "NOT IN", "val": ["CA", None]},
- {"NY", "TX"},
- id="not-in-with-explicit-null-operand",
- ),
- pytest.param(
- {"col": "state", "op": "!=", "val": None},
- {"CA", "NY", "TX"},
- id="not-equals-null-operand",
- ),
- pytest.param(
- {"col": "state", "op": "!=", "val": "<NULL>"},
- {"CA", "NY", "TX"},
- id="not-equals-null-marker-operand",
- ),
- pytest.param(
- {"col": "state", "op": "==", "val": None},
- {None},
- id="equals-null-operand",
- ),
- ],
-)
-def test_filters_with_unchanged_null_semantics(
- database: Database, flt: QueryObjectFilterClause, expected: set[str | None]
+def test_query_system_sampling_uses_read_limit_retry(
+ mocker: MockerFixture,
+ database: Database,
) -> None:
"""
- Operators that are not negated comparisons, and negated operators given an
- explicit NULL operand, keep their NULL handling: ``!= NULL`` still means
- ``IS NOT NULL`` and ``NOT IN (..., NULL)`` still excludes the NULL row.
+ The system_sampling extras marker (set by the samples query action)
+ routes execution through the read-limit retry; ordinary chart queries
+ are untouched.
"""
- assert _states_matching_filter(database, flt) == expected
+ import pandas as pd
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
-@pytest.mark.parametrize(
- "flt",
- [
- pytest.param({"col": "state", "op": "!=", "val": ""}, id="empty-string"),
- pytest.param({"col": "n", "op": "!=", "val": 0}, id="zero"),
- pytest.param({"col": "flag", "op": "!=", "val": False}, id="false"),
- pytest.param({"col": "state", "op": "!=", "val": ["XX"]}, id="one-item-list"),
- pytest.param(
- {"col": "state", "op": "!=", "val": ["XX", "CA"]}, id="multi-item-list"
- ),
- ],
-)
-def test_negated_filter_with_falsy_or_list_operand_keeps_null_rows(
- database: Database, flt: QueryObjectFilterClause
-) -> None:
- """
- A falsy operand (``0``, ``False``, ``""``) is a real value, not an absent one,
- so the NULL row is kept. A list operand for a scalar ``!=`` collapses to its
- first item and is treated the same way.
- """
- states = _states_matching_filter(database, flt)
- assert None in states
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[TableColumn(column_name="a")],
+ )
+ retry = mocker.patch.object(
+ database,
+ "run_with_sampling_read_limit_retry",
+ side_effect=lambda sql, run: run(sql),
+ )
+ mocker.patch.object(database, "get_df", return_value=pd.DataFrame({"a": [1]}))
+ table.query(
+ {
+ "columns": ["a"],
+ "extras": {"system_sampling": True},
+ "is_timeseries": False,
+ "row_limit": 10,
+ }
+ )
+ retry.assert_called_once()
-@pytest.mark.parametrize("val", [[], [None]], ids=["empty-list", "list-of-null"])
-def test_not_equals_with_empty_operand_stays_is_not_null(
- database: Database, val: list[None]
-) -> None:
- """A list operand with no usable item collapses to NULL and stays IS NOT NULL."""
- flt = cast(QueryObjectFilterClause, {"col": "state", "op": "!=", "val": val})
- assert _states_matching_filter(database, flt) == {"CA", "NY", "TX"}
+ retry.reset_mock()
+ table.query({"columns": ["a"], "is_timeseries": False, "row_limit": 10})
+ retry.assert_not_called()
-def test_not_equals_on_metric_keeps_null_aggregates(database: Database) -> None:
+def test_query_system_sampling_skips_virtual_dataset(
+ mocker: MockerFixture,
+ database: Database,
+) -> None:
"""
- A ``!=`` filter on a metric is applied in HAVING; a group whose aggregate is
- NULL is kept alongside the groups with a different aggregate.
+ Even sample requests do not receive the retry on virtual datasets.
"""
- table = _negation_table(database)
- # SUM(n) per state: CA=1, NY=2, TX=3, NULL state -> NULL
- sql, states = _run_negation_query(
- database,
- table,
- {"col": "sum_n", "op": "!=", "val": 1},
- columns=[],
- groupby=["state"],
- metrics=["sum_n"],
- )
- assert "HAVING" in sql
- assert states == {"NY", "TX", None}
+ import pandas as pd
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
-@pytest.mark.parametrize(
- "flt, expected",
- [
- pytest.param(
- {"col": "sum_n", "op": "==", "val": 1}, {"CA"}, id="equals-aggregate"
- ),
- pytest.param(
- {"col": "sum_n", "op": "!=", "val": None},
- {"CA", "NY", "TX"},
- id="not-equals-null-operand",
- ),
- ],
-)
-def test_metric_filters_with_unchanged_null_semantics(
- database: Database, flt: QueryObjectFilterClause, expected: set[str | None]
-) -> None:
- """``==`` on a metric and ``!= NULL`` on a metric keep their behavior."""
- _, states = _run_negation_query(
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="virtual_t",
+ sql="SELECT a FROM t",
+ columns=[TableColumn(column_name="a")],
+ )
+ retry = mocker.patch.object(
database,
- _negation_table(database),
- flt,
- columns=[],
- groupby=["state"],
- metrics=["sum_n"],
+ "run_with_sampling_read_limit_retry",
+ side_effect=lambda sql, run: run(sql),
)
- assert states == expected
-
+ mocker.patch.object(database, "get_df", return_value=pd.DataFrame({"a": [1]}))
-@pytest.mark.parametrize(
- "flt, null_target",
- [
- pytest.param(
- {"col": "upper_state", "op": "!=", "val": "CA"},
- r"my_isnull\(\(?UPPER\(state\)\)?\)",
- id="not-equals-calculated-column",
- ),
- pytest.param(
- {"col": "n", "op": "NOT LIKE", "val": "1%"},
- r"my_isnull\(n\)",
- id="not-like-checks-column-before-cast",
- ),
- pytest.param(
- {"col": "n", "op": "NOT ILIKE", "val": "1%"},
- r"my_isnull\(n\)",
- id="not-ilike-checks-column-before-cast",
- ),
- pytest.param(
- {"col": "state", "op": "!=", "val": "CA"},
- r"my_isnull\(state\)",
- id="not-equals-plain-column",
- ),
- ],
-)
-def test_negated_filter_null_check_uses_engine_spec_hook(
+ table.query(
+ {
+ "columns": ["a"],
+ "extras": {"system_sampling": True},
+ "is_timeseries": False,
+ "row_limit": 10,
+ }
+ )
+ retry.assert_not_called()
+
+
+def test_select_star_returns_unmodified_sql(
mocker: MockerFixture,
database: Database,
- flt: QueryObjectFilterClause,
- null_target: str,
) -> None:
"""
- The NULL check comes from the engine spec's ``handle_null_filter`` hook, so
- engines with their own NULL syntax (e.g. KQL ``isnull(col)``) get it, and it
- is applied to the filtered column itself rather than to its string cast.
+ select_star output is displayed in SQL Lab, returned by the API, and
+ persisted as CTAS result-fetch SQL, so it must never carry the
+ bounded-read override.
"""
- from sqlalchemy import func
+ from superset.sql.parse import Table
- from superset.connectors.sqla.models import SqlaTable
- from superset.db_engine_specs.sqlite import SqliteEngineSpec
+ retry_sql = mocker.patch.object(database, "sampling_read_limit_retry_sql")
+ sql = database.select_star(Table("t"), limit=10, latest_partition=False)
- class NullFunctionEngineSpec(SqliteEngineSpec):
- @classmethod
- def handle_null_filter(cls, sqla_col: Any, op: FilterOperator) -> Any:
- if op == FilterOperator.IS_NULL:
- return func.my_isnull(sqla_col)
- return super().handle_null_filter(sqla_col, op)
+ retry_sql.assert_not_called()
+ assert "SETTINGS" not in sql
- mocker.patch.object(
- SqlaTable, "db_engine_spec", new=property(lambda self: NullFunctionEngineSpec)
+
+def test_adhoc_type_probe_does_not_get_sampling_retry(
+ mocker: MockerFixture,
+ database: Database,
+) -> None:
+ """
+ The adhoc expression type probe is a zero-row WHERE FALSE query and must
+ never be routed through the sampling read-limit retry.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[TableColumn(column_name="a", type="INTEGER")],
)
- table = _negation_table(database)
- query = table.get_sqla_query(
- columns=["state"],
+ retry = mocker.patch.object(
+ database,
+ "run_with_sampling_read_limit_retry",
+ side_effect=lambda sql, run: run(sql),
+ )
+ mocker.patch(
+ "superset.connectors.sqla.models.get_columns_description",
+ return_value=[{"is_dttm": False, "type_generic": GenericDataType.NUMERIC}],
+ )
+
+ adhoc_col: AdhocColumn = {
+ "sqlExpression": "a + 1",
+ "label": "probe_me",
+ "columnType": "BASE_AXIS",
+ "timeGrain": "P1D",
+ }
+ table.adhoc_column_to_sqla(adhoc_col)
+
+ retry.assert_not_called()
+
+
+def test_filter_adhoc_column(database: Database) -> None:
+ """
+ Test that filter works with adhoc column labels.
+ When filter contains a string that matches the label of an adhoc column
+ in the columns list, it should correctly convert to a SQLAlchemy column
+ instead of raising QueryObjectValidationError.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ table = SqlaTable(
+ table_name="test_table",
+ database=database,
+ columns=[
+ TableColumn(column_name="name", type="TEXT"),
+ TableColumn(column_name="real_name", type="TEXT"),
+ ],
+ )
+
+ # Should not raise QueryObjectValidationError
+ result = table.get_sqla_query(
+ columns=[
+ "name",
+ {
+ "expressionType": "SQL",
+ "label": "full_name",
+ "sqlExpression": "real_name",
+ },
+ ],
+ orderby=[],
metrics=[],
extras={},
- filter=[flt],
+ filter=[
+ {"col": "full_name", "op": "ILIKE", "val": "Zona%"}
+ ], # Filter by adhoc column label
granularity=None,
is_timeseries=False,
)
- sql = database.compile_sqla_query(query.sqla_query)
-
- assert re.search(null_target, sql), sql
- assert "IS NULL" not in sql.upper()
+ assert result is not None
+ # Verify the WHERE predicate uses the resolved adhoc expression and value.
+ with database.get_sqla_engine() as engine:
+ sql = str(
+ result.sqla_query.compile(
+ dialect=engine.dialect,
+ compile_kwargs={"literal_binds": True},
+ )
+ )
-@pytest.fixture
-def advanced_data_types(mocker: MockerFixture) -> None:
- mocker.patch(
- "superset.models.helpers.feature_flag_manager.is_feature_enabled",
- side_effect=lambda feature: feature == "ENABLE_ADVANCED_DATA_TYPES",
- )
+ assert "real_name AS full_name" in sql
+ assert "WHERE" in sql
+ # The adhoc column resolved by label is parenthesized in the WHERE clause,
+ # consistent with inline adhoc columns, to guard operator precedence.
+ assert "lower((real_name)) LIKE lower('Zona%')" in sql
-@pytest.mark.usefixtures("advanced_data_types")
-@pytest.mark.parametrize(
- "flt",
- [
- pytest.param({"col": "ip", "op": "!=", "val": "1.1.1.1"}, id="ip-not-equals"),
- pytest.param({"col": "ip", "op": "NOT IN", "val": ["1.1.1.1"]}, id="ip-not-in"),
- pytest.param({"col": "port", "op": "!=", "val": "80"}, id="port-not-equals"),
- ],
-)
-def test_negated_advanced_data_type_filter_keeps_null_rows(
- database: Database, flt: QueryObjectFilterClause
+def test_get_query_result_wraps_post_processing_type_error(
+ database: "Database",
) -> None:
- """``!=`` / ``NOT IN`` on internet address and port columns keep NULL rows."""
- states = _states_matching_filter(database, flt)
- assert None in states
- assert "CA" not in states
+ """
+ A raw TypeError from pandas inside exec_post_processing (e.g. resample.mean()
+ on a DataFrame that contains object-dtype columns) must be surfaced as
+ QueryObjectValidationError (400) rather than propagating as a system 500.
+ """
+ from datetime import timedelta
+ from unittest.mock import patch
+ import pandas as pd
-@pytest.mark.usefixtures("advanced_data_types")
-@pytest.mark.parametrize(
- "flt, expected_with_null",
- [
- pytest.param(
- {"col": "ip", "op": "!=", "val": "192.168.1.0/24"},
- {"CA", "TX", None},
- id="ip-not-equals-cidr-range",
- ),
- pytest.param(
- {"col": "port", "op": "NOT IN", "val": ["80", "443"]},
- {"TX", None},
- id="port-not-in-multiple",
- ),
- ],
-)
-def test_negated_advanced_data_type_filter_keeps_null_rows_with_ranges_and_lists(
- database: Database,
- flt: QueryObjectFilterClause,
- expected_with_null: set[str | None],
-) -> None:
- """The CIDR-range ``!=`` and multi-value port ``NOT IN`` forms keep NULL rows."""
- assert _states_matching_filter(database, flt) == expected_with_null
+ from superset.common.query_object import QueryObject
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.exceptions import QueryObjectValidationError
+ from superset.models.helpers import QueryResult
+ table = SqlaTable(table_name="t", database=database)
-@pytest.mark.usefixtures("advanced_data_types")
-def test_ip_not_in_cidr_range_keeps_null_row(database: Database) -> None:
- """The NULL row survives a CIDR-range ``NOT IN``."""
- flt: QueryObjectFilterClause = {
- "col": "ip",
- "op": "NOT IN",
- "val": ["192.168.1.0/24"],
- }
- assert None in _states_matching_filter(database, flt)
+ # DatetimeIndex + object-dtype "category" column causes
+ # df.resample("1D").mean() to raise TypeError in pandas ≥ 2.x
+ df = pd.DataFrame(
+ {"metric": [1.0, 2.0], "category": ["a", "b"]},
+ index=pd.to_datetime(["2023-01-01", "2023-01-03"]),
+ )
+ query_object = QueryObject(
+ row_limit=10,
+ post_processing=[
+ {"operation": "resample", "options": {"method": "mean", "rule": "1D"}}
+ ],
+ )
-@pytest.mark.usefixtures("advanced_data_types")
-@pytest.mark.parametrize(
- "flt, expected",
- [
- pytest.param(
- {"col": "ip", "op": "==", "val": "1.1.1.1"}, {"CA"}, id="ip-equals"
- ),
- pytest.param({"col": "ip", "op": "IN", "val": ["1.1.1.1"]}, {"CA"}, id="ip-in"),
- pytest.param(
- {"col": "ip", "op": "==", "val": "192.168.1.0/24"},
- {"NY"},
- id="ip-equals-cidr-range",
- ),
- pytest.param(
- {"col": "port", "op": "==", "val": "80"}, {"CA"}, id="port-equals"
- ),
- pytest.param(
- {"col": "port", "op": "IN", "val": ["80", "443"]},
- {"CA", "NY"},
- id="port-in",
+ with (
+ patch.object(
+ table,
+ "query",
+ return_value=QueryResult(
+ df=df,
+ query="SELECT 1",
+ duration=timedelta(0),
+ sql_shifted_temporal_labels=set(),
+ ),
),
- ],
-)
-def test_advanced_data_type_filters_with_unchanged_null_semantics(
- database: Database, flt: QueryObjectFilterClause, expected: set[str | None]
-) -> None:
- """Non-negated operators on advanced data type columns are unchanged."""
- assert _states_matching_filter(database, flt) == expected
-
-
-@pytest.mark.usefixtures("advanced_data_types")
-def test_ip_not_in_cidr_range_non_null_rows_unchanged(database: Database) -> None:
- """
- Pins the current behavior for the non-NULL rows of a CIDR-range ``NOT IN``:
- the range conjunction ``(col > end) AND (col < start)`` can never hold, so no
- non-NULL row matches. Only the NULL row handling is meant to change.
- """
- flt: QueryObjectFilterClause = {
- "col": "ip",
- "op": "NOT IN",
- "val": ["192.168.1.0/24"],
- }
- assert _states_matching_filter(database, flt) - {None} == set()
+ patch.object(table, "normalize_df", return_value=df),
+ pytest.raises(QueryObjectValidationError),
+ ):
+ table.get_query_result(query_object)
diff --git a/tests/unit_tests/models/test_multivalue_filter.py b/tests/unit_tests/models/test_multivalue_filter.py
index ee40619a601..d665d1284a4 100644
--- a/tests/unit_tests/models/test_multivalue_filter.py
+++ b/tests/unit_tests/models/test_multivalue_filter.py
@@ -322,6 +322,44 @@ def test_in_on_array_parses_literals(mocker: MockerFixture, app: Flask) -> None:
"skills not in (array('driver'), array('cook'))",
id="not-in",
),
+ # Raw (non-string) values: the array literal comes from ``val`` itself,
+ # never from the scalar operand, which would collapse ``[]`` to NULL and
+ # ``[0]`` to the falsy ``0``. Every one of these is a real array.
+ pytest.param(
+ {"col": "skills", "op": FilterOperator.NOT_EQUALS.value, "val": []},
+ "skills != array()",
+ id="not-equals-raw-empty-list",
+ ),
+ pytest.param(
+ {"col": "scores", "op": FilterOperator.NOT_EQUALS.value, "val": [0]},
+ "scores != array(0)",
+ id="not-equals-raw-zero-list",
+ ),
+ pytest.param(
+ {"col": "skills", "op": FilterOperator.NOT_EQUALS.value, "val": [None]},
+ "skills != array(null)",
+ id="not-equals-raw-null-element",
+ ),
+ pytest.param(
+ {"col": "skills", "op": FilterOperator.NOT_IN.value, "val": [[]]},
+ "skills not in (array())",
+ id="not-in-raw-empty-array",
+ ),
+ pytest.param(
+ {"col": "scores", "op": FilterOperator.NOT_IN.value, "val": [[0]]},
+ "scores not in (array(0))",
+ id="not-in-raw-zero-array",
+ ),
+ pytest.param(
+ {"col": "scores", "op": FilterOperator.NOT_IN.value, "val": [0]},
+ "scores not in (array(0))",
+ id="not-in-raw-zero-candidate",
+ ),
+ pytest.param(
+ {"col": "skills", "op": FilterOperator.NOT_IN.value, "val": []},
+ "skills not in (",
+ id="not-in-raw-empty-candidate-list",
+ ),
],
)
def test_negated_whole_array_filter_keeps_null_arrays(
(superset) 02/04: test(models): extend NULL-handling guards to arrays, HAVING, engine hooks and advanced types
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-negated-filters-keep-null-rows
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 6e40fe22e522d432dc43a6cc334d413520525d64
Author: Claude <no...@anthropic.com>
AuthorDate: Thu Oct 15 01:35:12 2026 +0000
test(models): extend NULL-handling guards to arrays, HAVING, engine hooks and advanced types
Add guards for the remaining negated predicates: whole-array != and NOT IN
(compile level), != on a metric in HAVING, internet address and port
!= / NOT IN, the dashboard-scope != clause, and falsy or list operands.
Add a guard that the NULL check is built through the engine spec's
handle_null_filter hook and applied to the column before any string cast.
Pin the behavior that must not change (empty list operand, != NULL, ==,
IN, and the non-NULL rows of a CIDR-range NOT IN).
Co-Authored-By: Claude Sonnet 5.5 <no...@anthropic.com>
---
.../mcp_service/test_dashboard_scope_sql.py | 24 ++
tests/unit_tests/models/helpers_test.py | 354 +++++++++++++++++++--
tests/unit_tests/models/test_multivalue_filter.py | 100 ++++++
3 files changed, 451 insertions(+), 27 deletions(-)
diff --git a/tests/unit_tests/mcp_service/test_dashboard_scope_sql.py b/tests/unit_tests/mcp_service/test_dashboard_scope_sql.py
index 3d43761e946..7b7e2776e19 100644
--- a/tests/unit_tests/mcp_service/test_dashboard_scope_sql.py
+++ b/tests/unit_tests/mcp_service/test_dashboard_scope_sql.py
@@ -338,6 +338,30 @@ def test_null_aware_in_list_matches_the_dataset_path(
) == [(1030,)]
+def test_not_equals_keeps_null_rows_like_the_dataset_path(
+ database: Any, warehouse: sa.engine.Engine
+) -> None:
+ """``client != 'A'`` excludes only A: B (300) and the NULL client (1000) stay."""
+ not_a = DashboardConstraints(
+ ({"col": "client", "op": "!=", "val": "A"},), None, None
+ )
+ assert run(
+ warehouse, scoped(database, "SELECT SUM(amount) FROM orders", not_a)
+ ) == [(1300,)]
+
+
+def test_not_equals_null_operand_stays_is_not_null(
+ database: Any, warehouse: sa.engine.Engine
+) -> None:
+ """A NULL operand still means IS NOT NULL: the NULL client row is excluded."""
+ not_null = DashboardConstraints(
+ ({"col": "client", "op": "!=", "val": None},), None, None
+ )
+ assert run(
+ warehouse, scoped(database, "SELECT SUM(amount) FROM orders", not_null)
+ ) == [(330,)]
+
+
def test_time_range_is_applied_to_the_resolved_datetime_column(
database: Any, warehouse: sa.engine.Engine
) -> None:
diff --git a/tests/unit_tests/models/helpers_test.py b/tests/unit_tests/models/helpers_test.py
index 04d8534f99a..470cc9a4b2d 100644
--- a/tests/unit_tests/models/helpers_test.py
+++ b/tests/unit_tests/models/helpers_test.py
@@ -43,6 +43,7 @@ from superset.utils.core import (
from tests.unit_tests.conftest import with_feature_flags
if TYPE_CHECKING:
+ from superset.connectors.sqla.models import SqlaTable
from superset.jinja_context import BaseTemplateProcessor
from superset.models.core import Database
@@ -6040,26 +6041,49 @@ def test_get_query_result_wraps_post_processing_type_error(
# Rows used by the NULL-handling tests for negated filters. ``state`` is the
# column that is selected back out; the other columns are the filter targets for
-# the column-type cases. The last row is NULL in every filtered column.
+# the column-type cases (``ip`` and ``port`` carry the advanced data types). The
+# last row is NULL in every filtered column.
+_NEGATION_COLUMNS = ("id", "state", "n", "flag", "ts", "uid", "ip", "port")
_NEGATION_ROWS = [
- (1, "CA", 1, 1, "2024-01-01 00:00:00", "aaaaaaaa-0000-0000-0000-000000000001"),
- (2, "NY", 2, 0, "2024-02-01 00:00:00", "bbbbbbbb-0000-0000-0000-000000000002"),
- (3, "TX", 3, 0, "2024-03-01 00:00:00", "cccccccc-0000-0000-0000-000000000003"),
- (4, None, None, None, None, None),
+ (
+ 1,
+ "CA",
+ 1,
+ 1,
+ "2024-01-01 00:00:00",
+ "aaaaaaaa-0000-0000-0000-000000000001",
+ 16843009, # 1.1.1.1
+ 80,
+ ),
+ (
+ 2,
+ "NY",
+ 2,
+ 0,
+ "2024-02-01 00:00:00",
+ "bbbbbbbb-0000-0000-0000-000000000002",
+ 3232235777, # 192.168.1.1
+ 443,
+ ),
+ (
+ 3,
+ "TX",
+ 3,
+ 0,
+ "2024-03-01 00:00:00",
+ "cccccccc-0000-0000-0000-000000000003",
+ 3232236033, # 192.168.2.1
+ 8080,
+ ),
+ (4, None, None, None, None, None, None, None),
]
-def _states_matching_filter(
- database: Database, flt: QueryObjectFilterClause
-) -> set[str | None]:
- """
- Build ``flt`` with the real ``get_sqla_query`` against a SQLite table holding
- the ``_NEGATION_ROWS``, run the compiled SQL, and return the ``state`` values
- of the rows that survive the filter (``None`` for the all-NULL row).
- """
+def _negation_table(database: Database) -> SqlaTable:
+ """Create ``negation_t`` holding the ``_NEGATION_ROWS`` and register it."""
from sqlalchemy import text
- from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn
with database.get_sqla_engine() as engine:
with engine.begin() as connection:
@@ -6067,21 +6091,19 @@ def _states_matching_filter(
connection.execute(
text(
"CREATE TABLE negation_t (id INTEGER, state TEXT, n INTEGER, "
- "flag BOOLEAN, ts TIMESTAMP, uid UUID)"
+ "flag BOOLEAN, ts TIMESTAMP, uid UUID, ip INTEGER, port INTEGER)"
)
)
for row in _NEGATION_ROWS:
connection.execute(
text(
"INSERT INTO negation_t VALUES "
- "(:id, :state, :n, :flag, :ts, :uid)"
- ),
- dict(
- zip(("id", "state", "n", "flag", "ts", "uid"), row, strict=True)
+ "(:id, :state, :n, :flag, :ts, :uid, :ip, :port)"
),
+ dict(zip(_NEGATION_COLUMNS, row, strict=True)),
)
- table = SqlaTable(
+ return SqlaTable(
database=database,
schema=None,
table_name="negation_t",
@@ -6092,23 +6114,48 @@ def _states_matching_filter(
TableColumn(column_name="flag", type="BOOLEAN"),
TableColumn(column_name="ts", type="TIMESTAMP", is_dttm=True),
TableColumn(column_name="uid", type="UUID"),
+ TableColumn(
+ column_name="ip", type="INTEGER", advanced_data_type="internet_address"
+ ),
+ TableColumn(column_name="port", type="INTEGER", advanced_data_type="port"),
TableColumn(
column_name="upper_state", type="TEXT", expression="UPPER(state)"
),
],
+ metrics=[SqlMetric(metric_name="sum_n", expression="SUM(n)")],
)
+
+
+def _run_negation_query(
+ database: Database, table: SqlaTable, flt: QueryObjectFilterClause, **kwargs: Any
+) -> tuple[str, set[str | None]]:
+ """
+ Build ``flt`` with the real ``get_sqla_query``, compile it and run it on
+ SQLite. Returns the compiled SQL and the ``state`` values of the surviving
+ rows (``None`` for the all-NULL row).
+ """
query = table.get_sqla_query(
- columns=["state"],
- metrics=[],
- extras={},
- filter=[flt],
- granularity=None,
- is_timeseries=False,
+ **{
+ "columns": ["state"],
+ "metrics": [],
+ "extras": {},
+ "filter": [flt],
+ "granularity": None,
+ "is_timeseries": False,
+ **kwargs,
+ }
)
sql = database.compile_sqla_query(query.sqla_query)
with database.get_sqla_engine() as engine:
with engine.connect() as connection:
- return {row[0] for row in connection.exec_driver_sql(sql).fetchall()}
+ return sql, {row[0] for row in connection.exec_driver_sql(sql).fetchall()}
+
+
+def _states_matching_filter(
+ database: Database, flt: QueryObjectFilterClause
+) -> set[str | None]:
+ """The ``state`` values of the ``_NEGATION_ROWS`` that survive ``flt``."""
+ return _run_negation_query(database, _negation_table(database), flt)[1]
@pytest.mark.parametrize(
@@ -6239,3 +6286,256 @@ def test_filters_with_unchanged_null_semantics(
``IS NOT NULL`` and ``NOT IN (..., NULL)`` still excludes the NULL row.
"""
assert _states_matching_filter(database, flt) == expected
+
+
+@pytest.mark.parametrize(
+ "flt",
+ [
+ pytest.param({"col": "state", "op": "!=", "val": ""}, id="empty-string"),
+ pytest.param({"col": "n", "op": "!=", "val": 0}, id="zero"),
+ pytest.param({"col": "flag", "op": "!=", "val": False}, id="false"),
+ pytest.param({"col": "state", "op": "!=", "val": ["XX"]}, id="one-item-list"),
+ pytest.param(
+ {"col": "state", "op": "!=", "val": ["XX", "CA"]}, id="multi-item-list"
+ ),
+ ],
+)
+def test_negated_filter_with_falsy_or_list_operand_keeps_null_rows(
+ database: Database, flt: QueryObjectFilterClause
+) -> None:
+ """
+ A falsy operand (``0``, ``False``, ``""``) is a real value, not an absent one,
+ so the NULL row is kept. A list operand for a scalar ``!=`` collapses to its
+ first item and is treated the same way.
+ """
+ states = _states_matching_filter(database, flt)
+ assert None in states
+
+
+@pytest.mark.parametrize("val", [[], [None]], ids=["empty-list", "list-of-null"])
+def test_not_equals_with_empty_operand_stays_is_not_null(
+ database: Database, val: list[None]
+) -> None:
+ """A list operand with no usable item collapses to NULL and stays IS NOT NULL."""
+ flt = cast(QueryObjectFilterClause, {"col": "state", "op": "!=", "val": val})
+ assert _states_matching_filter(database, flt) == {"CA", "NY", "TX"}
+
+
+def test_not_equals_on_metric_keeps_null_aggregates(database: Database) -> None:
+ """
+ A ``!=`` filter on a metric is applied in HAVING; a group whose aggregate is
+ NULL is kept alongside the groups with a different aggregate.
+ """
+ table = _negation_table(database)
+ # SUM(n) per state: CA=1, NY=2, TX=3, NULL state -> NULL
+ sql, states = _run_negation_query(
+ database,
+ table,
+ {"col": "sum_n", "op": "!=", "val": 1},
+ columns=[],
+ groupby=["state"],
+ metrics=["sum_n"],
+ )
+ assert "HAVING" in sql
+ assert states == {"NY", "TX", None}
+
+
+@pytest.mark.parametrize(
+ "flt, expected",
+ [
+ pytest.param(
+ {"col": "sum_n", "op": "==", "val": 1}, {"CA"}, id="equals-aggregate"
+ ),
+ pytest.param(
+ {"col": "sum_n", "op": "!=", "val": None},
+ {"CA", "NY", "TX"},
+ id="not-equals-null-operand",
+ ),
+ ],
+)
+def test_metric_filters_with_unchanged_null_semantics(
+ database: Database, flt: QueryObjectFilterClause, expected: set[str | None]
+) -> None:
+ """``==`` on a metric and ``!= NULL`` on a metric keep their behavior."""
+ _, states = _run_negation_query(
+ database,
+ _negation_table(database),
+ flt,
+ columns=[],
+ groupby=["state"],
+ metrics=["sum_n"],
+ )
+ assert states == expected
+
+
+@pytest.mark.parametrize(
+ "flt, null_target",
+ [
+ pytest.param(
+ {"col": "upper_state", "op": "!=", "val": "CA"},
+ r"my_isnull\(\(?UPPER\(state\)\)?\)",
+ id="not-equals-calculated-column",
+ ),
+ pytest.param(
+ {"col": "n", "op": "NOT LIKE", "val": "1%"},
+ r"my_isnull\(n\)",
+ id="not-like-checks-column-before-cast",
+ ),
+ pytest.param(
+ {"col": "n", "op": "NOT ILIKE", "val": "1%"},
+ r"my_isnull\(n\)",
+ id="not-ilike-checks-column-before-cast",
+ ),
+ pytest.param(
+ {"col": "state", "op": "!=", "val": "CA"},
+ r"my_isnull\(state\)",
+ id="not-equals-plain-column",
+ ),
+ ],
+)
+def test_negated_filter_null_check_uses_engine_spec_hook(
+ mocker: MockerFixture,
+ database: Database,
+ flt: QueryObjectFilterClause,
+ null_target: str,
+) -> None:
+ """
+ The NULL check comes from the engine spec's ``handle_null_filter`` hook, so
+ engines with their own NULL syntax (e.g. KQL ``isnull(col)``) get it, and it
+ is applied to the filtered column itself rather than to its string cast.
+ """
+ from sqlalchemy import func
+
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.db_engine_specs.sqlite import SqliteEngineSpec
+
+ class NullFunctionEngineSpec(SqliteEngineSpec):
+ @classmethod
+ def handle_null_filter(cls, sqla_col: Any, op: FilterOperator) -> Any:
+ if op == FilterOperator.IS_NULL:
+ return func.my_isnull(sqla_col)
+ return super().handle_null_filter(sqla_col, op)
+
+ mocker.patch.object(
+ SqlaTable, "db_engine_spec", new=property(lambda self: NullFunctionEngineSpec)
+ )
+ table = _negation_table(database)
+ query = table.get_sqla_query(
+ columns=["state"],
+ metrics=[],
+ extras={},
+ filter=[flt],
+ granularity=None,
+ is_timeseries=False,
+ )
+ sql = database.compile_sqla_query(query.sqla_query)
+
+ assert re.search(null_target, sql), sql
+ assert "IS NULL" not in sql.upper()
+
+
+@pytest.fixture
+def advanced_data_types(mocker: MockerFixture) -> None:
+ mocker.patch(
+ "superset.models.helpers.feature_flag_manager.is_feature_enabled",
+ side_effect=lambda feature: feature == "ENABLE_ADVANCED_DATA_TYPES",
+ )
+
+
+@pytest.mark.usefixtures("advanced_data_types")
+@pytest.mark.parametrize(
+ "flt",
+ [
+ pytest.param({"col": "ip", "op": "!=", "val": "1.1.1.1"}, id="ip-not-equals"),
+ pytest.param({"col": "ip", "op": "NOT IN", "val": ["1.1.1.1"]}, id="ip-not-in"),
+ pytest.param({"col": "port", "op": "!=", "val": "80"}, id="port-not-equals"),
+ ],
+)
+def test_negated_advanced_data_type_filter_keeps_null_rows(
+ database: Database, flt: QueryObjectFilterClause
+) -> None:
+ """``!=`` / ``NOT IN`` on internet address and port columns keep NULL rows."""
+ states = _states_matching_filter(database, flt)
+ assert None in states
+ assert "CA" not in states
+
+
+@pytest.mark.usefixtures("advanced_data_types")
+@pytest.mark.parametrize(
+ "flt, expected_with_null",
+ [
+ pytest.param(
+ {"col": "ip", "op": "!=", "val": "192.168.1.0/24"},
+ {"CA", "TX", None},
+ id="ip-not-equals-cidr-range",
+ ),
+ pytest.param(
+ {"col": "port", "op": "NOT IN", "val": ["80", "443"]},
+ {"TX", None},
+ id="port-not-in-multiple",
+ ),
+ ],
+)
+def test_negated_advanced_data_type_filter_keeps_null_rows_with_ranges_and_lists(
+ database: Database,
+ flt: QueryObjectFilterClause,
+ expected_with_null: set[str | None],
+) -> None:
+ """The CIDR-range ``!=`` and multi-value port ``NOT IN`` forms keep NULL rows."""
+ assert _states_matching_filter(database, flt) == expected_with_null
+
+
+@pytest.mark.usefixtures("advanced_data_types")
+def test_ip_not_in_cidr_range_keeps_null_row(database: Database) -> None:
+ """The NULL row survives a CIDR-range ``NOT IN``."""
+ flt: QueryObjectFilterClause = {
+ "col": "ip",
+ "op": "NOT IN",
+ "val": ["192.168.1.0/24"],
+ }
+ assert None in _states_matching_filter(database, flt)
+
+
+@pytest.mark.usefixtures("advanced_data_types")
+@pytest.mark.parametrize(
+ "flt, expected",
+ [
+ pytest.param(
+ {"col": "ip", "op": "==", "val": "1.1.1.1"}, {"CA"}, id="ip-equals"
+ ),
+ pytest.param({"col": "ip", "op": "IN", "val": ["1.1.1.1"]}, {"CA"}, id="ip-in"),
+ pytest.param(
+ {"col": "ip", "op": "==", "val": "192.168.1.0/24"},
+ {"NY"},
+ id="ip-equals-cidr-range",
+ ),
+ pytest.param(
+ {"col": "port", "op": "==", "val": "80"}, {"CA"}, id="port-equals"
+ ),
+ pytest.param(
+ {"col": "port", "op": "IN", "val": ["80", "443"]},
+ {"CA", "NY"},
+ id="port-in",
+ ),
+ ],
+)
+def test_advanced_data_type_filters_with_unchanged_null_semantics(
+ database: Database, flt: QueryObjectFilterClause, expected: set[str | None]
+) -> None:
+ """Non-negated operators on advanced data type columns are unchanged."""
+ assert _states_matching_filter(database, flt) == expected
+
+
+@pytest.mark.usefixtures("advanced_data_types")
+def test_ip_not_in_cidr_range_non_null_rows_unchanged(database: Database) -> None:
+ """
+ Pins the current behavior for the non-NULL rows of a CIDR-range ``NOT IN``:
+ the range conjunction ``(col > end) AND (col < start)`` can never hold, so no
+ non-NULL row matches. Only the NULL row handling is meant to change.
+ """
+ flt: QueryObjectFilterClause = {
+ "col": "ip",
+ "op": "NOT IN",
+ "val": ["192.168.1.0/24"],
+ }
+ assert _states_matching_filter(database, flt) - {None} == set()
diff --git a/tests/unit_tests/models/test_multivalue_filter.py b/tests/unit_tests/models/test_multivalue_filter.py
index 675ab9003e7..ee40619a601 100644
--- a/tests/unit_tests/models/test_multivalue_filter.py
+++ b/tests/unit_tests/models/test_multivalue_filter.py
@@ -272,3 +272,103 @@ def test_in_on_array_parses_literals(mocker: MockerFixture, app: Flask) -> None:
],
)
assert "skills in (array('driver'), array('cook'))" in sql
+
+
+@pytest.mark.parametrize(
+ "flt, negated_sql",
+ [
+ pytest.param(
+ {
+ "col": "skills",
+ "op": FilterOperator.NOT_EQUALS.value,
+ "val": "['Driver', 'Cook']",
+ },
+ "skills != array('driver', 'cook')",
+ id="not-equals",
+ ),
+ pytest.param(
+ {
+ "col": "skills",
+ "op": FilterOperator.NOT_EQUALS.value,
+ "val": "Driver",
+ },
+ "skills != array('driver')",
+ id="not-equals-plain-value",
+ ),
+ pytest.param(
+ {
+ "col": "skills",
+ "op": FilterOperator.NOT_EQUALS.value,
+ "val": "[]",
+ },
+ "skills != array()",
+ id="not-equals-empty-array",
+ ),
+ pytest.param(
+ {
+ "col": "scores",
+ "op": FilterOperator.NOT_EQUALS.value,
+ "val": "[0]",
+ },
+ "scores != array(0)",
+ id="not-equals-zero-element",
+ ),
+ pytest.param(
+ {
+ "col": "skills",
+ "op": FilterOperator.NOT_IN.value,
+ "val": ["['Driver']", "['Cook']"],
+ },
+ "skills not in (array('driver'), array('cook'))",
+ id="not-in",
+ ),
+ ],
+)
+def test_negated_whole_array_filter_keeps_null_arrays(
+ mocker: MockerFixture, app: Flask, flt: dict[str, Any], negated_sql: str
+) -> None:
+ """
+ Whole-array ``!=`` and ``NOT IN`` only exclude the arrays they name, so a
+ row whose array column is NULL is kept. These are compile-level checks: an
+ array comparison cannot run on SQLite, and array elements (not the arrays
+ themselves) are the only nullable part of a ClickHouse array, so this is
+ defensive coverage of the same policy as the scalar operators.
+ """
+ dataset = _make_dataset(mocker)
+ _clickhouse(mocker, dataset)
+ with app.test_request_context():
+ sql = _sql(dataset, [flt])
+ assert negated_sql in sql
+ assert f"{flt['col']} is null" in sql
+
+
+@pytest.mark.parametrize(
+ "flt",
+ [
+ pytest.param(
+ {"col": "skills", "op": FilterOperator.EQUALS.value, "val": "['Driver']"},
+ id="equals",
+ ),
+ pytest.param(
+ {"col": "skills", "op": FilterOperator.IN.value, "val": ["['Driver']"]},
+ id="in",
+ ),
+ pytest.param(
+ {
+ "col": "skills",
+ "op": FilterOperator.CONTAINS_ANY.value,
+ "val": ["Driver"],
+ },
+ id="contains-any",
+ ),
+ ],
+)
+def test_non_negated_whole_array_filter_has_no_null_check(
+ mocker: MockerFixture, app: Flask, flt: dict[str, Any]
+) -> None:
+ """Operators that are not negations do not gain a NULL check."""
+ dataset = _make_dataset(mocker)
+ _clickhouse(mocker, dataset)
+ with app.test_request_context():
+ sql = _sql(dataset, [flt])
+ assert "is null" not in sql
(superset) 01/04: test(models): guard NULL handling of !=, NOT LIKE and NOT ILIKE filters
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-negated-filters-keep-null-rows
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 7402f1b5db8d3156a4727a1c4619625c5f20e0f5
Author: Claude <no...@anthropic.com>
AuthorDate: Thu Oct 15 01:19:09 2026 +0000
test(models): guard NULL handling of !=, NOT LIKE and NOT ILIKE filters
Execute the real get_sqla_query output on SQLite for rows CA, NY, TX and
NULL. The negated comparison and pattern filters currently drop the NULL
row; these cases are expected to keep it. Also pin the operators whose
NULL handling must not change (=, IN, LIKE, ILIKE, IS NULL, explicit NULL
operands).
Co-Authored-By: Claude Sonnet 5.5 <no...@anthropic.com>
---
tests/unit_tests/models/helpers_test.py | 209 +++++++++++++++++++++++++++++++-
1 file changed, 208 insertions(+), 1 deletion(-)
diff --git a/tests/unit_tests/models/helpers_test.py b/tests/unit_tests/models/helpers_test.py
index 7346969e1ce..04d8534f99a 100644
--- a/tests/unit_tests/models/helpers_test.py
+++ b/tests/unit_tests/models/helpers_test.py
@@ -35,7 +35,11 @@ from sqlalchemy.sql.elements import Cast, ColumnElement
from sqlalchemy.sql.visitors import iterate
from superset.superset_typing import AdhocColumn, AdhocMetric, OrderBy
-from superset.utils.core import FilterOperator, GenericDataType
+from superset.utils.core import (
+ FilterOperator,
+ GenericDataType,
+ QueryObjectFilterClause,
+)
from tests.unit_tests.conftest import with_feature_flags
if TYPE_CHECKING:
@@ -6032,3 +6036,206 @@ def test_get_query_result_wraps_post_processing_type_error(
pytest.raises(QueryObjectValidationError),
):
table.get_query_result(query_object)
+
+
+# Rows used by the NULL-handling tests for negated filters. ``state`` is the
+# column that is selected back out; the other columns are the filter targets for
+# the column-type cases. The last row is NULL in every filtered column.
+_NEGATION_ROWS = [
+ (1, "CA", 1, 1, "2024-01-01 00:00:00", "aaaaaaaa-0000-0000-0000-000000000001"),
+ (2, "NY", 2, 0, "2024-02-01 00:00:00", "bbbbbbbb-0000-0000-0000-000000000002"),
+ (3, "TX", 3, 0, "2024-03-01 00:00:00", "cccccccc-0000-0000-0000-000000000003"),
+ (4, None, None, None, None, None),
+]
+
+
+def _states_matching_filter(
+ database: Database, flt: QueryObjectFilterClause
+) -> set[str | None]:
+ """
+ Build ``flt`` with the real ``get_sqla_query`` against a SQLite table holding
+ the ``_NEGATION_ROWS``, run the compiled SQL, and return the ``state`` values
+ of the rows that survive the filter (``None`` for the all-NULL row).
+ """
+ from sqlalchemy import text
+
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ with database.get_sqla_engine() as engine:
+ with engine.begin() as connection:
+ connection.execute(text("DROP TABLE IF EXISTS negation_t"))
+ connection.execute(
+ text(
+ "CREATE TABLE negation_t (id INTEGER, state TEXT, n INTEGER, "
+ "flag BOOLEAN, ts TIMESTAMP, uid UUID)"
+ )
+ )
+ for row in _NEGATION_ROWS:
+ connection.execute(
+ text(
+ "INSERT INTO negation_t VALUES "
+ "(:id, :state, :n, :flag, :ts, :uid)"
+ ),
+ dict(
+ zip(("id", "state", "n", "flag", "ts", "uid"), row, strict=True)
+ ),
+ )
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="negation_t",
+ columns=[
+ TableColumn(column_name="id", type="INTEGER"),
+ TableColumn(column_name="state", type="TEXT"),
+ TableColumn(column_name="n", type="INTEGER"),
+ TableColumn(column_name="flag", type="BOOLEAN"),
+ TableColumn(column_name="ts", type="TIMESTAMP", is_dttm=True),
+ TableColumn(column_name="uid", type="UUID"),
+ TableColumn(
+ column_name="upper_state", type="TEXT", expression="UPPER(state)"
+ ),
+ ],
+ )
+ query = table.get_sqla_query(
+ columns=["state"],
+ metrics=[],
+ extras={},
+ filter=[flt],
+ granularity=None,
+ is_timeseries=False,
+ )
+ sql = database.compile_sqla_query(query.sqla_query)
+ with database.get_sqla_engine() as engine:
+ with engine.connect() as connection:
+ return {row[0] for row in connection.exec_driver_sql(sql).fetchall()}
+
+
+@pytest.mark.parametrize(
+ "flt",
+ [
+ pytest.param({"col": "state", "op": "!=", "val": "CA"}, id="not-equals-text"),
+ pytest.param(
+ {"col": "state", "op": "NOT LIKE", "val": "%C%"}, id="not-like-text"
+ ),
+ pytest.param(
+ {"col": "state", "op": "NOT ILIKE", "val": "%c%"}, id="not-ilike-text"
+ ),
+ ],
+)
+def test_negated_filter_keeps_null_rows(
+ database: Database, flt: QueryObjectFilterClause
+) -> None:
+ """
+ ``!=``, ``NOT LIKE`` and ``NOT ILIKE`` only exclude the rows they name.
+
+ In SQL ``NULL != 'CA'`` is not true, so a bare ``state != 'CA'`` would also
+ drop the row whose ``state`` is NULL. A negated filter must keep it.
+ """
+ assert _states_matching_filter(database, flt) == {"NY", "TX", None}
+
+
+@pytest.mark.parametrize(
+ "flt",
+ [
+ pytest.param({"col": "n", "op": "!=", "val": 1}, id="not-equals-numeric"),
+ pytest.param({"col": "n", "op": "!=", "val": "1"}, id="not-equals-numeric-str"),
+ pytest.param({"col": "flag", "op": "!=", "val": True}, id="not-equals-boolean"),
+ pytest.param(
+ {"col": "ts", "op": "!=", "val": "2024-01-01 00:00:00"},
+ id="not-equals-temporal",
+ ),
+ pytest.param(
+ {"col": "upper_state", "op": "!=", "val": "CA"},
+ id="not-equals-calculated-column",
+ ),
+ pytest.param(
+ {
+ "col": {
+ "expressionType": "SQL",
+ "sqlExpression": "UPPER(state)",
+ "label": "upper_state_expr",
+ },
+ "op": "!=",
+ "val": "CA",
+ },
+ id="not-equals-adhoc-sql-column",
+ ),
+ pytest.param(
+ {"col": "n", "op": "NOT LIKE", "val": "1%"},
+ id="not-like-numeric-cast-to-string",
+ ),
+ pytest.param(
+ {"col": "n", "op": "NOT ILIKE", "val": "1%"},
+ id="not-ilike-numeric-cast-to-string",
+ ),
+ pytest.param(
+ {"col": "uid", "op": "NOT LIKE", "val": "aaaaaaaa%"},
+ id="not-like-uuid-cast-to-string",
+ ),
+ pytest.param(
+ {"col": "uid", "op": "NOT ILIKE", "val": "AAAAAAAA%"},
+ id="not-ilike-uuid-cast-to-string",
+ ),
+ ],
+)
+def test_negated_filter_keeps_null_rows_across_column_types(
+ database: Database, flt: QueryObjectFilterClause
+) -> None:
+ """
+ The NULL row is kept for numeric, boolean, temporal, UUID, calculated and
+ ad-hoc SQL expression columns, including the LIKE paths that cast the column
+ to a string first.
+ """
+ assert _states_matching_filter(database, flt) == {"NY", "TX", None}
+
+
+@pytest.mark.parametrize(
+ "flt, expected",
+ [
+ pytest.param({"col": "state", "op": "==", "val": "CA"}, {"CA"}, id="equals"),
+ pytest.param({"col": "state", "op": "IN", "val": ["CA"]}, {"CA"}, id="in"),
+ pytest.param({"col": "state", "op": "LIKE", "val": "%C%"}, {"CA"}, id="like"),
+ pytest.param({"col": "state", "op": "ILIKE", "val": "%c%"}, {"CA"}, id="ilike"),
+ pytest.param(
+ {"col": "state", "op": "IS NULL", "val": None}, {None}, id="is-null"
+ ),
+ pytest.param(
+ {"col": "state", "op": "IS NOT NULL", "val": None},
+ {"CA", "NY", "TX"},
+ id="is-not-null",
+ ),
+ pytest.param(
+ {"col": "n", "op": ">", "val": 1}, {"NY", "TX"}, id="greater-than"
+ ),
+ pytest.param(
+ {"col": "state", "op": "NOT IN", "val": ["CA", None]},
+ {"NY", "TX"},
+ id="not-in-with-explicit-null-operand",
+ ),
+ pytest.param(
+ {"col": "state", "op": "!=", "val": None},
+ {"CA", "NY", "TX"},
+ id="not-equals-null-operand",
+ ),
+ pytest.param(
+ {"col": "state", "op": "!=", "val": "<NULL>"},
+ {"CA", "NY", "TX"},
+ id="not-equals-null-marker-operand",
+ ),
+ pytest.param(
+ {"col": "state", "op": "==", "val": None},
+ {None},
+ id="equals-null-operand",
+ ),
+ ],
+)
+def test_filters_with_unchanged_null_semantics(
+ database: Database, flt: QueryObjectFilterClause, expected: set[str | None]
+) -> None:
+ """
+ Operators that are not negated comparisons, and negated operators given an
+ explicit NULL operand, keep their NULL handling: ``!= NULL`` still means
+ ``IS NOT NULL`` and ``NOT IN (..., NULL)`` still excludes the NULL row.
+ """
+ assert _states_matching_filter(database, flt) == expected
(superset) 04/04: fix(models): keep NULL rows in !=, NOT LIKE and NOT ILIKE filters
Posted by jl...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-negated-filters-keep-null-rows
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 5fe82a3f840c64b6d9d147923b518517a9ff46b8
Author: Claude <no...@anthropic.com>
AuthorDate: Thu Oct 15 02:08:47 2026 +0000
fix(models): keep NULL rows in !=, NOT LIKE and NOT ILIKE filters
SQL three-valued logic makes NULL != 'CA' (and NULL NOT LIKE x) not true, so a
filter that only names values to exclude also dropped rows whose column is
NULL. Negated predicates now also match NULL rows, via a shared helper that
builds the NULL check through the engine spec and applies it to the filtered
column rather than to its string cast.
Applies to scalar !=, NOT LIKE and NOT ILIKE, whole-array != and NOT IN,
!= and NOT IN on advanced data type columns, and != in the MCP dashboard
scope clause. An explicit NULL operand keeps its previous meaning.
Co-Authored-By: Claude Sonnet 5.5 <no...@anthropic.com>
---
UPDATING.md | 9 +++
superset/mcp_service/dashboard_scope_sql.py | 7 ++-
superset/models/helpers.py | 86 ++++++++++++++++++++++++-----
3 files changed, 87 insertions(+), 15 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index 5eddffc2f77..75be99af035 100644
--- a/UPDATING.md
+++ b/UPDATING.md
@@ -43,6 +43,15 @@ assists people when migrating to a new version.
the view's `metadata_cache_token`, so metadata refresh makes old entries miss. Enable
`SEMANTIC_LAYERS`; configure shared persistent `DATA_CACHE_CONFIG` and Redis-backed `DISTRIBUTED_COORDINATION_CONFIG` on every web/worker process.
+- `!=`, `NOT LIKE` and `NOT ILIKE` filters on SQLAlchemy-backed datasets now also
+ return rows where the column is NULL, as do whole-array `!=` / `NOT IN` filters
+ and `!=` / `NOT IN` filters on advanced data type columns (internet address,
+ port). Previously these rows were dropped by SQL three-valued logic
+ (`NULL != 'CA'` is not true), so a filter such as `state != 'CA'` removed NULL
+ rows in addition to `CA`. Dashboards, charts and reports that use these
+ operators may therefore show additional NULL rows. An explicit NULL operand
+ behaves as before (`!= NULL` still means `IS NOT NULL`).
+
- Address validation refuses additional non-global address forms. Addresses in
the well-known NAT64 prefix `64:ff9b::/96` are checked using their embedded IPv4
address and refused when that address is unsafe. Local-use translation
diff --git a/superset/mcp_service/dashboard_scope_sql.py b/superset/mcp_service/dashboard_scope_sql.py
index 9101d21c67b..0305972e561 100644
--- a/superset/mcp_service/dashboard_scope_sql.py
+++ b/superset/mcp_service/dashboard_scope_sql.py
@@ -382,6 +382,7 @@ def _filter_clause(
"""
import sqlalchemy as sa
+ from superset.models.helpers import keep_null_rows
from superset.utils.core import FilterOperator, GenericDataType
values = clause["val"] if isinstance(clause["val"], list) else [clause["val"]]
@@ -428,7 +429,11 @@ def _filter_clause(
"an empty value.",
_SQL_GUIDANCE,
)
- return db_engine_spec.handle_comparison_filter(sqla_col, op, value)
+ comparison = db_engine_spec.handle_comparison_filter(sqla_col, op, value)
+ if op == FilterOperator.NOT_EQUALS and value is not None:
+ # Same as the dataset path: ``!= x`` only excludes ``x``; NULL rows stay.
+ return keep_null_rows(sqla_col, comparison, db_engine_spec)
+ return comparison
def _in_list_clause(sqla_col: Any, op: Any, value: Any, name: str) -> Any:
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index 77a18702172..18cf648fe10 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -681,6 +681,32 @@ def parse_array_literal(value: Any) -> list[Any]:
return [value]
+def keep_null_rows(
+ sqla_col: Any,
+ negated_cond: Any,
+ db_engine_spec: builtins.type[BaseEngineSpec],
+) -> Any:
+ """
+ Make a negated predicate keep the rows where ``sqla_col`` is NULL.
+
+ SQL three-valued logic evaluates ``NULL != 'a'``, ``NULL NOT LIKE 'a%'`` and
+ similar negations to NULL, which a WHERE clause treats as false, so a bare
+ negated filter would also drop NULL rows although it only names the values to
+ exclude. The NULL check is built through the engine spec so engines with their
+ own NULL syntax are honored, and it must be given the column the filter names
+ (before any string cast applied for pattern matching).
+
+ :param sqla_col: the filtered column expression
+ :param negated_cond: the negated predicate built for ``sqla_col``
+ :param db_engine_spec: engine spec of the database being queried
+ :return: ``sqla_col IS NULL OR negated_cond``
+ """
+ return or_(
+ db_engine_spec.handle_null_filter(sqla_col, utils.FilterOperator.IS_NULL),
+ negated_cond,
+ )
+
+
def coerce_array_values(
values: list[Any], element_type: Optional[utils.GenericDataType]
) -> list[Any]:
@@ -5407,11 +5433,20 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
_(bus_resp["error_message"])
)
- target_clause_list.append(
- ADVANCED_DATA_TYPES[col_advanced_data_type].translate_filter(
- sqla_col, op, bus_resp["values"]
+ translated_filter = ADVANCED_DATA_TYPES[
+ col_advanced_data_type
+ ].translate_filter(sqla_col, op, bus_resp["values"])
+ # Negations keep NULL rows unless the operand itself is NULL,
+ # which a plugin may translate on its own (``IS NOT NULL``).
+ if (op == utils.FilterOperator.NOT_EQUALS and eq is not None) or (
+ op == utils.FilterOperator.NOT_IN
+ and isinstance(eq, (list, tuple))
+ and all(value is not None for value in eq)
+ ):
+ translated_filter = keep_null_rows(
+ sqla_col, translated_filter, db_engine_spec
)
- )
+ target_clause_list.append(translated_filter)
elif is_multivalue_col and op in {
utils.FilterOperator.EQUALS,
utils.FilterOperator.NOT_EQUALS,
@@ -5432,11 +5467,12 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
parse_array_literal(val), array_element_type
)
)
- cond = (
- sqla_col != literal
- if op == utils.FilterOperator.NOT_EQUALS
- else sqla_col == literal
- )
+ if op == utils.FilterOperator.NOT_EQUALS:
+ cond = keep_null_rows(
+ sqla_col, sqla_col != literal, db_engine_spec
+ )
+ else:
+ cond = sqla_col == literal
else:
candidates: list[Any] = (
list(val) if isinstance(val, (list, tuple)) else [val]
@@ -5453,7 +5489,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
]
)
if op == utils.FilterOperator.NOT_IN:
- cond = ~cond
+ cond = keep_null_rows(sqla_col, ~cond, db_engine_spec)
target_clause_list.append(cond)
elif op in {
utils.FilterOperator.IN,
@@ -5584,9 +5620,16 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
utils.FilterOperator.GREATER_THAN_OR_EQUALS,
utils.FilterOperator.LESS_THAN_OR_EQUALS,
}:
- target_clause_list.append(
- db_engine_spec.handle_comparison_filter(sqla_col, op, eq)
+ comparison = db_engine_spec.handle_comparison_filter(
+ sqla_col, op, eq
)
+ # ``!= NULL`` already means IS NOT NULL; any other operand
+ # only excludes its own value, so NULL rows stay.
+ if op == utils.FilterOperator.NOT_EQUALS and eq is not None:
+ comparison = keep_null_rows(
+ sqla_col, comparison, db_engine_spec
+ )
+ target_clause_list.append(comparison)
elif op in {
utils.FilterOperator.ILIKE,
utils.FilterOperator.LIKE,
@@ -5599,6 +5642,9 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
target_generic_type != GenericDataType.STRING
or is_uuid_native_type(col_type)
)
+ # NULL rows are identified on the column itself, not on
+ # its string cast.
+ null_check_col = sqla_col
if needs_string_cast_for_like:
sqla_col = sa.cast(sqla_col, sa.String)
@@ -5607,9 +5653,21 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
elif op == utils.FilterOperator.ILIKE:
target_clause_list.append(sqla_col.ilike(eq))
elif op == utils.FilterOperator.NOT_LIKE:
- target_clause_list.append(sqla_col.not_like(eq))
+ target_clause_list.append(
+ keep_null_rows(
+ null_check_col,
+ sqla_col.not_like(eq),
+ db_engine_spec,
+ )
+ )
else:
- target_clause_list.append(sqla_col.not_ilike(eq))
+ target_clause_list.append(
+ keep_null_rows(
+ null_check_col,
+ sqla_col.not_ilike(eq),
+ db_engine_spec,
+ )
+ )
elif op in {
utils.FilterOperator.CONTAINS_ANY,
utils.FilterOperator.CONTAINS_ALL,