aminghadersohi commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4179235170
##########
superset/datasets/api.py:
##########
@@ -323,6 +401,17 @@ class DatasetRestApi(SoftDeleteApiMixin,
BaseSupersetModelRestApi):
]
show_columns = show_select_columns + [
"columns.type_generic",
+ # Engine-supplied pre-fill for the editor's value transform input.
+ "partition_value_transform_default",
+ # The resolved mapping summary, which is what Explore's pruning
+ # indicator reads. Saving a dataset from Explore reloads this endpoint
+ # and replaces the chart's datasource with the response wholesale -- it
+ # is a replacement, not a merge -- so anything the summary carries and
+ # this payload does not is lost on an unrelated save and the glyphs
+ # vanish until the page is reloaded. Recomputing it client-side from
+ # `columns` is not an option: `data_for_slices` prunes the partition
+ # column on dashboards, which is why the summary is self-contained.
+ "partition_filter_mapping",
Review Comment:
`check-openapi-spec-drift` fails on this line: it publishes a new readOnly
`partition_filter_mapping` field, so `docs/static/resources/openapi.json` needs
regenerating here (no suggestion: the fix is in the generated file).
##########
tests/unit_tests/connectors/sqla/partition_mapping_test.py:
##########
@@ -0,0 +1,1434 @@
+# 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 typing import Any, cast
+from importlib import import_module
Review Comment:
`pre-commit` stays red even after the mypy fix: ruff I001 wants `importlib`
before `typing`.
```suggestion
from importlib import import_module
from typing import Any, cast
```
##########
tests/unit_tests/connectors/sqla/partition_mapping_test.py:
##########
@@ -0,0 +1,1434 @@
+# 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 typing import Any, cast
+from importlib import import_module
+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.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,
+ MappingValidationIssue,
+ MIRRORABLE_ALWAYS,
+ MIRRORABLE_IF_MONOTONIC,
+ mirrorable_operators,
+ parse_error_detail,
+ 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
+ )
+
+
+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:
Review Comment:
`ruff-format` also rewrites this signature onto one line, which is the other
modified file in the failing pre-commit run.
```suggestion
def test_a_case_insensitive_engine_does_not_opt_in(module: str, spec_name:
str) -> None:
```
--
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]
