sadpandajoe commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4202688638
##########
superset/models/helpers.py:
##########
@@ -4292,36 +4389,621 @@ def dttm_sql_literal(self, dttm: datetime, col:
"TableColumn") -> str:
return f"""'{dttm.strftime("%Y-%m-%d %H:%M:%S.%f")}'"""
- def get_time_filter( # pylint: disable=too-many-arguments # noqa: C901
+ def _mirror_probe_value(self, dttm: datetime, col:
Optional["TableColumn"]) -> Any:
+ """
+ ``dttm`` in the mapped column's own stored representation.
+
+ The predicate this mirror stands in for compares the column against
+ `dttm_sql_literal(dttm, col)`, so the transform has to be probed with
+ the same value. On a column stored as an epoch integer, ``T`` is a
+ function of an integer: probing it with a `datetime` either returns
+ something unrelated to the partition keys or raises, and a raise costs
+ the dataset its pruning silently.
+
+ Only the ``python_date_format`` branches of `dttm_sql_literal` are
+ reproduced, because only those produce a *value*. `convert_dttm` yields
+ engine-specific SQL text, which cannot be bound as a parameter; its own
+ divergence from the probe is handled by widening the bounds instead.
+ """
+ if col is None:
+ return dttm
+
+ tf = col.python_date_format
Review Comment:
The mirror is probed with `dttm.strftime(tf)` whenever a
`python_date_format` is set, but `dttm_sql_literal` hands the filter's own
bound to `convert_dttm` first and only falls back to the format when the engine
returns nothing. For a TIMESTAMP mapped column that also carries a `%Y%m%d`
format (via the column or `python_date_format_by_column_name`), `event_time >=
'2026-01-01 00:00:00'` is mirrored as `partition_col >= '20260101'`, so a
January 15 row whose key is `2026-01-15` compares below it and is dropped from
a chart the filter would keep. Should this use the same precedence as
`dttm_sql_literal`, so the format is applied only when `convert_dttm` yields
nothing?
##########
tests/unit_tests/connectors/sqla/partition_mapping_test.py:
##########
@@ -0,0 +1,1973 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``superset.connectors.sqla.partition_mapping``."""
+
+from __future__ import annotations
+
+from datetime import datetime
+from decimal import Decimal
+from importlib import import_module
+from typing import Any, cast
+from unittest.mock import MagicMock, patch, PropertyMock
+
+import pandas as pd
+import pytest
+from dateutil.relativedelta import relativedelta
+from flask import Flask
+from sqlalchemy.dialects import mysql, postgresql
+
+from superset.config import DISALLOWED_SQL_FUNCTIONS
+from superset.connectors.sqla.models import SqlaTable, TableColumn
+from superset.connectors.sqla.partition_mapping import (
+ _probe_cache_key,
+ _render_literal,
+ build_probe_sql,
+ contains_jinja,
+ contains_value_placeholder,
+ equality_mirrors_safely,
+ evaluate_transform,
+ find_non_deterministic_functions,
+ grain_bucket_width,
+ GRAIN_BUCKET_WIDTHS,
+ is_bare_expression,
+ is_parseable,
+ is_transform_active,
+ is_unfinished,
+ known_mirror_verdict,
+ MappingValidationIssue,
+ mirror_operator,
+ MIRRORABLE_ALWAYS,
+ MIRRORABLE_IF_MONOTONIC,
+ mirrorable_operators,
+ parse_error_detail,
+ placeholder_is_executable,
+ probe_sql_is_evaluable,
+ probed_value_type_error,
+ RawProbeValue,
+ resolve_partition_mapping,
+ stored_expression_error,
+ validate_partition_mapping,
+ validate_transform,
+)
+from superset.constants import TimeGrain
+from superset.db_engine_specs.base import BaseEngineSpec
+from superset.db_engine_specs.oracle import OracleEngineSpec
+from superset.models.core import Database
+from superset.utils.core import FilterOperator
+
+
[email protected](autouse=True)
+def real_probe_cache(app: Flask) -> Any:
+ """
+ The test app runs a null cache, which would make every cache assertion here
+ vacuously pass. Swap in a real in-memory one for the duration.
+ """
+ from flask_caching import Cache
+
+ from superset.extensions import cache_manager
+
+ cache = Cache(config={"CACHE_TYPE": "SimpleCache",
"CACHE_DEFAULT_TIMEOUT": 300})
+ cache.init_app(app)
+ original = cache_manager._cache # noqa: SLF001
+ cache_manager._cache = cache # noqa: SLF001
+ yield
+ cache_manager._cache = original # noqa: SLF001
+
+
[email protected](autouse=True)
+def enable_partition_filter_mapping(app: Flask) -> Any:
+ """The feature ships off; every test here exercises it on."""
+ original =
app.config["DEFAULT_FEATURE_FLAGS"].get("PARTITION_FILTER_MAPPING")
+ app.config["DEFAULT_FEATURE_FLAGS"]["PARTITION_FILTER_MAPPING"] = True
+ yield
+ if original is None:
+ del app.config["DEFAULT_FEATURE_FLAGS"]["PARTITION_FILTER_MAPPING"]
+ else:
+ app.config["DEFAULT_FEATURE_FLAGS"]["PARTITION_FILTER_MAPPING"] =
original
+
+
+def _table(**kwargs: Any) -> SqlaTable:
+ database = Database(database_name="test_db", sqlalchemy_uri="sqlite://")
+ defaults: dict[str, Any] = {
+ "table_name": "web_events",
+ "database": database,
+ "main_dttm_col": "event_time",
+ "columns": [
+ TableColumn(column_name="event_time", is_dttm=True,
type="TIMESTAMP"),
+ TableColumn(
+ column_name="dt_epoch",
+ type="BIGINT",
+ partition_value_transform=None,
+ ),
+ TableColumn(column_name="country", type="VARCHAR"),
+ TableColumn(column_name="region_key", type="VARCHAR"),
+ ],
+ }
+ defaults.update(kwargs)
+ return SqlaTable(**defaults)
+
+
+def _mapped_table(
+ *,
+ partition_column: str = "dt_epoch",
+ mapped_column: str = "event_time",
+ transform: str | None = "unix_timestamp(:value)",
+ monotonic: bool = True,
+ partition_mapped_column: str | None = None,
+ main_dttm_col: str | None = "event_time",
+) -> SqlaTable:
+ table = _table(main_dttm_col=main_dttm_col)
+ table.partition_column = partition_column
+ table.partition_mapped_column = partition_mapped_column
+ for column in table.columns:
+ if column.column_name == mapped_column:
+ column.partition_value_transform = transform
+ column.partition_transform_is_monotonic = monotonic
+ return table
+
+
+# ---------------------------------------------------------------------------
+# §2 — operator safety matrix
+# ---------------------------------------------------------------------------
+
+
+def test_equality_and_in_are_always_mirrorable() -> None:
+ """``=`` and ``IN`` are safe for any function ``T``."""
+ assert MIRRORABLE_ALWAYS == {FilterOperator.EQUALS, FilterOperator.IN}
+
+
+def test_range_operators_require_a_monotonic_transform() -> None:
+ assert MIRRORABLE_IF_MONOTONIC == {
+ FilterOperator.GREATER_THAN,
+ FilterOperator.GREATER_THAN_OR_EQUALS,
+ FilterOperator.LESS_THAN,
+ FilterOperator.LESS_THAN_OR_EQUALS,
+ FilterOperator.TEMPORAL_RANGE,
+ }
+
+
+def test_mirrorable_operators_excludes_ranges_when_not_monotonic() -> None:
+ assert mirrorable_operators(is_monotonic=False) == MIRRORABLE_ALWAYS
+
+
+def test_mirrorable_operators_includes_ranges_when_monotonic() -> None:
+ assert mirrorable_operators(is_monotonic=True) == (
+ MIRRORABLE_ALWAYS | MIRRORABLE_IF_MONOTONIC
+ )
+
+
[email protected](
+ "operator",
+ [
+ FilterOperator.NOT_EQUALS,
+ FilterOperator.NOT_IN,
+ FilterOperator.LIKE,
+ FilterOperator.ILIKE,
+ FilterOperator.NOT_LIKE,
+ FilterOperator.NOT_ILIKE,
+ FilterOperator.IS_NULL,
+ FilterOperator.IS_NOT_NULL,
+ FilterOperator.IS_TRUE,
+ FilterOperator.IS_FALSE,
+ ],
+)
+def test_negations_and_pattern_matches_are_never_mirrorable(
+ operator: FilterOperator,
+) -> None:
+ """
+ ``T`` is not injective, so ``col != v`` does **not** imply ``T(col) !=
T(v)``:
+ mirroring it would drop rows the original filter keeps.
+ """
+ assert operator not in mirrorable_operators(is_monotonic=True)
+
+
+def test_equality_is_dropped_when_the_engine_does_not_compare_byte_exactly()
-> None:
+ """
+ "Safe for any function ``T``" reasons about value equality: ``T`` is a
+ function, so ``col = v`` gives ``T(col) = T(v)``. The engine reasons about
+ *SQL* equality, and the two part company under a case-insensitive
+ collation -- a stored ``'us'`` satisfies a filter for ``'US'`` while the
+ mirror ``hex('US')`` excludes the row, and the chart loses it silently.
+ """
+ assert mirrorable_operators(is_monotonic=False, equality_is_safe=False) ==
set()
+
+
+def test_ranges_survive_an_unsafe_equality() -> None:
+ """
+ Range mirroring already rests on the owner declaring ``T``
+ order-preserving with respect to the column's own order -- an assertion
+ about the very comparison semantics this gate checks for. Equality has no
+ such declaration behind it, which is why only equality is withdrawn.
+ """
+ assert (
+ mirrorable_operators(is_monotonic=True, equality_is_safe=False)
+ == MIRRORABLE_IF_MONOTONIC
+ )
+
+
[email protected](
+ "operator,expected",
+ [
+ (FilterOperator.GREATER_THAN, FilterOperator.GREATER_THAN_OR_EQUALS),
+ (FilterOperator.LESS_THAN, FilterOperator.LESS_THAN_OR_EQUALS),
+ (
+ FilterOperator.GREATER_THAN_OR_EQUALS,
+ FilterOperator.GREATER_THAN_OR_EQUALS,
+ ),
+ (FilterOperator.LESS_THAN_OR_EQUALS,
FilterOperator.LESS_THAN_OR_EQUALS),
+ (FilterOperator.EQUALS, FilterOperator.EQUALS),
+ (FilterOperator.IN, FilterOperator.IN),
+ ],
+)
+def test_a_strict_bound_mirrors_non_strictly(
+ operator: FilterOperator, expected: FilterOperator
+) -> None:
+ """
+ Order-preserving means non-decreasing, not injective, so ``col < v``
+ implies only ``T(col) <= T(v)``. A day-key transform maps a whole day onto
+ one value and a strict mirror drops the boundary bucket. The non-strict
+ cases are already safe and pass through, which is what makes applying this
+ twice harmless.
+ """
+ assert mirror_operator(operator) == expected
+
+
+def test_relaxing_a_bound_stays_inside_the_mirrorable_set() -> None:
+ """
+ The substitution is about strictness, not about which filters mirror. The
+ Explore indicator reads `mirrorable_operators`, so leaving that set alone
is
+ what keeps the glyph from promising pruning the SQL does not do -- and it
is
+ also what guarantees `handle_comparison_filter` is never handed an operator
+ it has no case for.
+ """
+ assert {
+ mirror_operator(operator) for operator in MIRRORABLE_IF_MONOTONIC
+ } <= MIRRORABLE_IF_MONOTONIC
+
+
+def _column(type_: str) -> TableColumn:
+ database = Database(database_name="t", sqlalchemy_uri="sqlite://")
+ table = SqlaTable(table_name="t", database=database)
+ return TableColumn(column_name="c", type=type_, table=table)
+
+
[email protected](
+ "type_, binary, expected",
+ [
+ ("VARCHAR", True, True),
+ ("VARCHAR", False, False),
+ # Numeric and temporal comparison is exact everywhere.
+ ("BIGINT", False, True),
+ ("TIMESTAMP", False, True),
+ ("DOUBLE", False, True),
+ ],
+)
+def test_equality_mirrors_safely_only_guards_string_columns(
+ app: Flask, type_: str, binary: bool, expected: bool
+) -> None:
+ class _Spec(BaseEngineSpec):
+ binary_string_comparison = binary
+
+ with app.app_context():
+ assert equality_mirrors_safely(_column(type_), _Spec) is expected
+
+
+def test_equality_mirrors_safely_fails_closed_on_an_unresolvable_type(
+ app: Flask,
+) -> None:
+ """
+ The gate exists to stop a silent wrong answer, so a column whose type it
+ cannot read is treated as the risky case rather than waved through.
+ """
+ column = MagicMock()
+ type(column).type_generic = PropertyMock(side_effect=ValueError("no type"))
+
+ with app.app_context():
+ assert equality_mirrors_safely(column, BaseEngineSpec) is False
+
+
+def test_no_mapped_column_leaves_the_matrix_alone(app: Flask) -> None:
+ """Nothing to reason about, and the resolver bails out on it anyway."""
+ with app.app_context():
+ assert equality_mirrors_safely(None, BaseEngineSpec) is True
+
+
[email protected](
+ "module, spec_name",
+ [
+ ("presto", "PrestoEngineSpec"),
+ ("hive", "HiveEngineSpec"),
+ ("trino", "TrinoEngineSpec"),
+ ("impala", "ImpalaEngineSpec"),
+ ("postgres", "PostgresEngineSpec"),
+ ("bigquery", "BigQueryEngineSpec"),
+ ("sqlite", "SqliteEngineSpec"),
+ ],
+)
+def test_the_byte_exact_engines_opt_in(module: str, spec_name: str) -> None:
+ """
+ The engines this feature targets compare text byte-exactly, so the gate
+ must not cost them the second canonical mapping (`lower(:value)` onto a
+ lowercased key).
+ """
+ spec = getattr(import_module(f"superset.db_engine_specs.{module}"),
spec_name)
+ assert spec.binary_string_comparison is True
+
+
[email protected]("module, spec_name", [("mysql", "MySQLEngineSpec")])
+def test_a_case_insensitive_engine_does_not_opt_in(module: str, spec_name:
str) -> None:
+ """
+ MySQL's default collation is `utf8mb4_0900_ai_ci`, which is exactly the
+ case the reviewer raised.
+ """
+ spec = getattr(import_module(f"superset.db_engine_specs.{module}"),
spec_name)
+ assert spec.binary_string_comparison is False
+
+
+def test_the_base_spec_does_not_opt_in() -> None:
+ """A spec that has not said so does not mirror string equality."""
+ assert BaseEngineSpec.binary_string_comparison is False
+
+
+# ---------------------------------------------------------------------------
+# Time grain bucket widths
+# ---------------------------------------------------------------------------
+
+
[email protected]("grain", list(TimeGrain))
+def test_every_builtin_grain_has_a_known_bucket_width(
+ app: Flask, grain: TimeGrain
+) -> None:
+ """
+ The widening is only sound for a grain whose bucket width we know, so a
+ grain added upstream without a width here would silently stop mirroring.
+ This is the test that notices.
+ """
+ with app.app_context():
+ assert grain_bucket_width(grain.value, "sqlite") is not None
+
+
+def test_a_calendar_grain_widens_by_a_calendar_month(app: Flask) -> None:
+ """A month is not thirty days, and the month it lands in decides its
width."""
+ with app.app_context():
+ width = grain_bucket_width(TimeGrain.MONTH.value, "sqlite")
+
+ assert width is not None
+ assert datetime(2026, 1, 31) + width == datetime(2026, 2, 28)
+ assert datetime(2026, 2, 1) + width == datetime(2026, 3, 1)
+
+
+def test_a_fractional_grain_has_a_width(app: Flask) -> None:
+ """
+ ``PT0.5H`` and ``P0.25Y`` are why the widths are written out rather than
+ parsed -- and the anchored week grains are intervals ``parse_duration``
+ rejects outright.
+ """
+ with app.app_context():
+ assert grain_bucket_width(TimeGrain.HALF_HOUR.value, "sqlite") ==
relativedelta(
+ minutes=30
+ )
+ assert grain_bucket_width(
+ TimeGrain.WEEK_ENDING_SATURDAY.value, "sqlite"
+ ) == relativedelta(days=7)
+
+
+def test_an_unknown_grain_has_no_known_width(app: Flask) -> None:
+ with app.app_context():
+ assert grain_bucket_width("P13X", "sqlite") is None
+ assert grain_bucket_width(None, "sqlite") is None
+
+
+def test_an_addon_grain_has_no_known_width(app: Flask) -> None:
+ """An operator-declared grain carries whatever duration string they
typed."""
+ with app.app_context():
+ app.config["TIME_GRAIN_ADDONS"] = {"PT2S": "2 second"}
+ try:
+ assert grain_bucket_width("PT2S", "sqlite") is None
+ finally:
+ app.config["TIME_GRAIN_ADDONS"] = {}
+
+
+def test_a_builtin_grain_an_operator_redefined_has_no_known_width(
+ app: Flask,
+) -> None:
+ """
+ ``TIME_GRAIN_ADDON_EXPRESSIONS`` replaces a built-in grain's SQL for one
+ engine, so its duration no longer bounds the displacement *there* -- but it
+ still does on every other engine.
+ """
+ with app.app_context():
+ app.config["TIME_GRAIN_ADDON_EXPRESSIONS"] = {
+ "sqlite": {"P1D": "DATETIME({col}, 'start of year')"}
+ }
+ try:
+ assert grain_bucket_width(TimeGrain.DAY.value, "sqlite") is None
+ assert grain_bucket_width(TimeGrain.DAY.value, "hive") is not None
+ finally:
+ app.config["TIME_GRAIN_ADDON_EXPRESSIONS"] = {}
+
+
+def test_the_width_table_is_keyed_on_grain_durations(app: Flask) -> None:
+ """Keys are the ISO duration a filter carries, not the enum member name."""
+ assert GRAIN_BUCKET_WIDTHS[TimeGrain.DAY] == relativedelta(days=1)
+ assert set(GRAIN_BUCKET_WIDTHS) == {grain.value for grain in TimeGrain}
+
+
+# ---------------------------------------------------------------------------
+# §4.1 — resolving the effective mapping, and the defensive bail-outs
+# ---------------------------------------------------------------------------
+
+
+def test_resolve_returns_the_mapping_when_everything_lines_up(app: Flask) ->
None:
+ with app.app_context():
+ mapping = resolve_partition_mapping(_mapped_table())
+
+ assert mapping is not None
+ assert mapping.partition_column == "dt_epoch"
+ assert mapping.mapped_column == "event_time"
+ assert mapping.value_transform == "unix_timestamp(:value)"
+ assert mapping.is_monotonic is True
+
+
+def test_effective_mapped_column_follows_main_dttm_col(app: Flask) -> None:
+ """No explicit override: the mapping follows the default datetime
column."""
+ with app.app_context():
+ mapping = resolve_partition_mapping(_mapped_table())
+
+ assert mapping is not None
+ assert mapping.mapped_column == "event_time"
+
+
+def test_explicit_override_wins_over_main_dttm_col(app: Flask) -> None:
+ table = _mapped_table(
+ mapped_column="country",
+ transform="lower(:value)",
+ monotonic=False,
+ partition_mapped_column="country",
+ )
+ table.partition_column = "region_key"
+
+ with app.app_context():
+ mapping = resolve_partition_mapping(table)
+
+ assert mapping is not None
+ assert mapping.mapped_column == "country"
+ assert mapping.partition_column == "region_key"
+ assert mapping.is_monotonic is False
+
+
+def test_resolve_returns_none_when_the_feature_flag_is_off(app: Flask) -> None:
+ table = _mapped_table()
+ with app.app_context():
+ with patch(
+ "superset.connectors.sqla.partition_mapping.feature_flag_manager."
+ "is_feature_enabled",
+ return_value=False,
+ ):
+ assert resolve_partition_mapping(table) is None
+
+
+def test_resolve_returns_none_without_a_partition_column(app: Flask) -> None:
+ table = _mapped_table()
+ table.partition_column = None
+ with app.app_context():
+ assert resolve_partition_mapping(table) is None
+
+
+def test_resolve_returns_none_when_partition_column_no_longer_exists(
+ app: Flask,
+) -> None:
+ """A column sync can drop the physical partition column out from under
us."""
+ table = _mapped_table()
+ table.partition_column = "dt_epoch_gone"
+ with app.app_context():
+ assert resolve_partition_mapping(table) is None
+
+
+def test_resolve_returns_none_when_the_mapped_column_no_longer_exists(
+ app: Flask,
+) -> None:
+ table = _mapped_table(main_dttm_col="vanished")
+ with app.app_context():
+ assert resolve_partition_mapping(table) is None
+
+
+def test_resolve_returns_none_on_self_mapping(app: Flask) -> None:
+ """
+ ``partition_column == effective mapped column`` mirrors a column onto
itself.
+ Save-time validation rejects it, but rows predating that validation exist.
+ """
+ table = _mapped_table(
+ partition_column="event_time",
+ mapped_column="event_time",
+ )
+ with app.app_context():
+ assert resolve_partition_mapping(table) is None
+
+
[email protected]("transform", [None, "", " "])
+def test_resolve_returns_none_without_a_transform(
+ app: Flask, transform: str | None
+) -> None:
+ table = _mapped_table(transform=transform)
+ with app.app_context():
+ assert resolve_partition_mapping(table) is None
+
+
+def test_resolve_returns_none_when_the_transform_lacks_the_placeholder(
+ app: Flask,
+) -> None:
+ table = _mapped_table(transform="unix_timestamp(event_time)")
+ with app.app_context():
+ assert resolve_partition_mapping(table) is None
+
+
+def test_resolve_returns_none_for_a_non_deterministic_transform(app: Flask) ->
None:
+ """
+ The query-time gate is `is_transform_active`, the same one the Explore
+ indicator reads. Anything narrower would let a transform that reached
+ storage without passing `UpdateDatasetCommand` -- through import, or a
+ hand-written bundle -- mirror a filter with a value frozen at probe time,
+ while the editor reported the mapping as inactive and nothing on screen
+ said otherwise.
+ """
+ table = _mapped_table(transform="unix_timestamp(:value) + rand()")
+ with app.app_context():
+ assert resolve_partition_mapping(table) is None
+
+
+def test_resolve_returns_none_for_a_multi_expression_transform(app: Flask) ->
None:
+ """A select list is not an expression; the probe cannot align its
columns."""
+ table = _mapped_table(transform="lower(:value), 'x'")
+ with app.app_context():
+ assert resolve_partition_mapping(table) is None
+
+
+def test_resolve_returns_none_for_an_unparseable_transform(app: Flask) -> None:
+ table = _mapped_table(transform="unix_timestamp(:value")
+ with app.app_context():
+ assert resolve_partition_mapping(table) is None
+
+
+def test_resolve_returns_none_when_the_mapped_column_has_an_advanced_data_type(
+ app: Flask,
+) -> None:
+ """
+ ``translate_filter`` builds its own predicate shape from *translated*
values,
+ so the ``(operator, value)`` pair the operator matrix reasons about does
not
+ exist. Mirroring anyway would silently apply the wrong values (§4.1).
+ """
+ table = _mapped_table()
+ for column in table.columns:
+ if column.column_name == "event_time":
+ column.advanced_data_type = "port"
+
+ with app.app_context():
+ with patch.dict(app.config["ADVANCED_DATA_TYPES"], {"port":
MagicMock()}):
+ with patch(
+
"superset.connectors.sqla.partition_mapping.feature_flag_manager."
+ "is_feature_enabled",
+ side_effect=lambda flag: (
+ flag in {"PARTITION_FILTER_MAPPING",
"ENABLE_ADVANCED_DATA_TYPES"}
+ ),
+ ):
+ assert resolve_partition_mapping(table) is None
+
+
+def test_advanced_data_type_does_not_block_when_the_flag_is_off(app: Flask) ->
None:
+ """An inert ``advanced_data_type`` is not a reason to skip mirroring."""
+ table = _mapped_table()
+ for column in table.columns:
+ if column.column_name == "event_time":
+ column.advanced_data_type = "port"
+
+ with app.app_context():
+ with patch(
+ "superset.connectors.sqla.partition_mapping.feature_flag_manager."
+ "is_feature_enabled",
+ side_effect=lambda flag: flag == "PARTITION_FILTER_MAPPING",
+ ):
+ assert resolve_partition_mapping(table) is not None
+
+
+# ---------------------------------------------------------------------------
+# §5 — transform inspection helpers
+# ---------------------------------------------------------------------------
+
+
[email protected](
+ "transform,expected",
+ [
+ ("unix_timestamp(:value)", True),
+ ("lower(:value)", True),
+ ("CAST(:value AS BIGINT)", True),
+ ("unix_timestamp(event_time)", False),
+ (":values", False),
+ ("", False),
+ (None, False),
+ ],
+)
+def test_contains_value_placeholder(transform: str | None, expected: bool) ->
None:
+ assert contains_value_placeholder(transform) is expected
+
+
[email protected](
+ "transform,expected",
+ [
+ ("unix_timestamp(:value)", False),
+ ("{{ current_username() }}", True),
+ ("lower({% if x %}:value{% endif %})", True),
+ ("lower(:value) -- {# comment #}", True),
+ ],
+)
+def test_contains_jinja(transform: str, expected: bool) -> None:
+ assert contains_jinja(transform) is expected
+
+
[email protected](
+ "transform",
+ [
+ "unix_timestamp(:value)",
+ "lower(:value)",
+ "CAST(:value AS BIGINT)",
+ "date_format(:value, 'yyyyMMdd')",
+ ],
+)
+def test_pure_transforms_report_no_non_deterministic_functions(
+ transform: str,
+) -> None:
+ assert find_non_deterministic_functions(transform, "hive") == set()
+
+
[email protected](
+ "transform,expected_name",
+ [
+ ("date_diff(:value, now())", "NOW"),
+ ("CAST(:value AS BIGINT) + rand()", "RAND"),
+ ("CAST(:value AS DATE) - current_date", "CURRENT_DATE"),
+ ],
+)
+def test_non_deterministic_functions_are_reported(
+ transform: str, expected_name: str
+) -> None:
+ """
+ The probe runs at a different moment and in a different session from the
+ chart query, and its result is cached, so anything time- or
+ randomness-dependent freezes a snapshot of probe time into the predicate.
+ """
+ assert expected_name in find_non_deterministic_functions(transform, "hive")
+
+
+def test_niladic_unix_timestamp_is_rejected_but_the_unary_form_is_not() ->
None:
+ """
+ On Hive/Impala ``unix_timestamp()`` means "now" while ``unix_timestamp(x)``
+ -- the canonical temporal transform -- is pure. The distinction is the
whole
+ reason this check inspects arity rather than just the name.
+ """
+ assert find_non_deterministic_functions("unix_timestamp(:value)", "hive")
== set()
+ assert find_non_deterministic_functions(
+ "unix_timestamp(:value) - unix_timestamp()", "hive"
+ )
+
+
+# ---------------------------------------------------------------------------
+# §4.2 — probe evaluation
+# ---------------------------------------------------------------------------
+
+
+def _database_returning(values: list[Any]) -> Database:
+ """A real ``Database`` (so the dialect is real) with only ``get_df``
stubbed."""
+ database = Database(database_name="probe_db", sqlalchemy_uri="sqlite://")
+ database.get_df = MagicMock( # type: ignore[method-assign]
+ return_value=pd.DataFrame(
+ [values], columns=[f"v{i}" for i in range(len(values))]
+ )
+ )
+ return database
+
+
+def _database_returning_frame(frame: pd.DataFrame) -> Database:
+ """
+ ``_database_returning`` for a probe result whose column dtypes matter.
+
+ The flat-list sibling builds its frame from one Python list, so pandas
+ infers a single dtype and the row-versus-cell distinction cannot be seen.
+ """
+ database = Database(database_name="probe_db", sqlalchemy_uri="sqlite://")
+ database.get_df = MagicMock( # type: ignore[method-assign]
+ return_value=frame
+ )
+ return database
+
+
+def _probe(database: Database) -> MagicMock:
+ """
+ The stubbed ``get_df``, typed for call assertions.
+
+ ``Database.get_df`` is a real method, so mypy reads its declared signature
+ rather than the mock that replaced it and rejects ``.call_args`` and
+ friends.
+ """
+ return cast(MagicMock, database.get_df)
+
+
+def test_evaluate_transform_returns_one_value_per_input(app: Flask) -> None:
+ database = _database_returning([1767225600, 1769904000])
+
+ with app.app_context():
+ result = evaluate_transform(
+ database,
+ None,
+ "default",
+ "unix_timestamp(:value)",
+ ["2026-01-01 00:00:00", "2026-02-01 00:00:00"],
+ )
+
+ assert result == [1767225600, 1769904000]
+ _probe(database).assert_called_once()
+
+
+def test_evaluate_transform_binds_values_rather_than_interpolating(
+ app: Flask,
+) -> None:
+ """
+ Filter values are attacker-controlled (a Gamma user picks them), so they
must
+ be bound and escaped, never pasted into the probe SQL.
+ """
+ database = _database_returning(["o''brien"])
+
+ with app.app_context():
+ evaluate_transform(database, None, None, "lower(:value)", ["O'Brien"])
+
+ sql = _probe(database).call_args.kwargs["sql"]
+ assert "O'Brien" not in sql
+ assert "O''Brien" in sql
+
+
+def test_evaluate_transform_dedupes_repeated_values(app: Flask) -> None:
+ """Three inputs, two distinct: the probe evaluates the transform twice."""
+ database = _database_returning(["us", "us"])
+
+ with app.app_context():
+ result = evaluate_transform(
+ database, None, None, "lower(:value)", ["US", "US", "us"]
+ )
+
+ assert result == ["us", "us", "us"]
+ sql = _probe(database).call_args.kwargs["sql"]
+ assert sql.count("lower") == 2
+
+
+def test_evaluate_transform_fails_open_on_a_short_result_row(app: Flask) ->
None:
+ """A row narrower than the probe asked for means the results cannot be
+ aligned back to their inputs; pruning is skipped rather than guessed at."""
+ database = _database_returning(["us"])
+
+ with app.app_context():
+ assert (
+ evaluate_transform(database, None, None, "lower(:value)", ["US",
"CA"])
+ is None
+ )
+
+
+def test_evaluate_transform_fails_open_on_a_wide_result_row(app: Flask) ->
None:
+ """
+ Too many columns is the dangerous direction. A transform whose select list
+ holds two expressions returns 2N columns for N inputs, and reading the
first
+ N interleaves the expressions instead of taking one per value -- a
predicate
+ built from the wrong values rather than one that is merely short. Here
+ `lower('US'), 'x', lower('CA'), 'x'` would have yielded `('us', 'x')` and
+ dropped every CA row.
+ """
+ database = _database_returning(["us", "x", "ca", "x"])
+
+ with app.app_context():
+ assert (
+ evaluate_transform(database, None, None, "lower(:value), 'x'",
["US", "CA"])
Review Comment:
`lower(:value), 'x'` is refused by `stored_expression_error` (not a single
expression) before `get_df` is ever called, so the four-column frame this mock
returns is never consumed and `None` comes back for a different reason.
Changing the width guard in `_probe` from `!=` to `<` would leave this test
green, even though that is the interleaving case the docstring describes. Could
this use a valid transform such as `lower(:value)` with the same oversized
frame, and assert that `get_df` ran once and the result is `None`?
##########
superset/views/datasource/views.py:
##########
@@ -157,6 +203,18 @@ def save(self) -> FlaskResponse:
),
status=409,
)
+ # `partition_value_transform` rides in on `update_from_object`, which
+ # writes every field in `update_from_object_fields` straight onto the
+ # column. That bypasses `UpdateDatasetCommand`, and with it the gate
+ # the REST PUT and the mapping preview both apply -- so without this
+ # the deprecated endpoint is a way to store arbitrary SQL that the
+ # partition probe later runs. Refused rather than dropped, unlike the
+ # importer: this is one interactive edit whose author is present to
+ # correct it, not a bundle where failing the whole dataset over one
+ # expression is the worse trade.
+ if error := _partition_transform_error(orm_datasource,
datasource_dict):
Review Comment:
The legacy save now refuses an unsafe transform, but `partition_column` and
`partition_mapped_column` ride in through `update_from_object` with no
reference check. With the flag on, a payload carrying `partition_column:
"missing_column"` is stored and returns 200, and every later `PUT
/api/v1/dataset/<pk>` (even a description-only one) then fails
`_validate_partition_mapping` with a column-not-found error until someone
repairs the reference. Should this endpoint run the same mapping-reference
validation as `UpdateDatasetCommand` before writing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
