mikebridge commented on code in PR #44454: URL: https://github.com/apache/superset/pull/44454#discussion_r4209085778
########## superset/semantic_layers/dimension_resolution.py: ########## @@ -0,0 +1,81 @@ +# 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. + +"""Default preference for immutable semantic dimension variants.""" + +from collections.abc import Iterable + +from flask_babel import gettext as _ +from superset_core.semantic_layers.types import Dimension, Grain + +from superset.exceptions import QueryObjectValidationError + +# Known grains rank finest to coarsest, ahead of unknown representations. +_GRAIN_FINENESS: dict[str, int] = { + "PT1S": 0, + "PT1M": 1, + "PT1H": 2, + "P1D": 3, + "P1W": 4, + "P1M": 5, + "P3M": 6, + "P1Y": 7, +} + + +def grain_preference(dimension: Dimension) -> tuple[int, int, str]: + """Prefer raw, then finest known grain, then lexical representation.""" + grain: Grain | None = dimension.grain + if grain is None: + return (0, 0, "") + representation: str = grain.representation + return ( + 1, + _GRAIN_FINENESS.get(representation, len(_GRAIN_FINENESS)), + representation, + ) + + +def resolve_dimension_defaults( + dimensions: Iterable[Dimension], +) -> dict[str, Dimension]: + """Resolve names without hiding conflicting identities for any grain. + + Return the original provider objects. Even non-preferred grains must be + unambiguous: explicit grouping can select them independently of defaults. + """ + defaults: dict[str, Dimension] = {} + identities: dict[tuple[str, Grain | None], str] = {} + for dimension in dimensions: + key: tuple[str, Grain | None] = (dimension.name, dimension.grain) + previous_id: str | None = identities.get(key) + if previous_id is not None and previous_id != dimension.id: + raise QueryObjectValidationError( + _( + "Semantic dimension '%(name)s' has ambiguous variants for " + "grain '%(grain)s'. Use one ID per name and grain.", + name=dimension.name, + grain=dimension.grain.representation if dimension.grain else "raw", + ) + ) + identities[key] = dimension.id + preferred: Dimension | None = defaults.get(dimension.name) + if preferred is None or grain_preference(dimension) < grain_preference( + preferred + ): Review Comment: Good catch; the merge now retains each winning preference tuple within the resolver call, so every dimension's preference is evaluated once. The regression failed with six calls before the change and passes with four afterward; the algorithm remains linear. ########## tests/unit_tests/datasource/test_compatible_api.py: ########## @@ -0,0 +1,81 @@ +# 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. + +"""Compatibility API preserves semantic provider identities before caching.""" + +from unittest.mock import MagicMock + +import pyarrow as pa +import pytest +from flask.testing import FlaskClient +from pytest_mock import MockerFixture +from superset_core.semantic_layers.types import Dimension, Grains, Metric +from werkzeug.test import TestResponse + +from superset.semantic_layers.models import SemanticView + + [email protected]("ambiguous", [False, True]) +def test_compatible_endpoint_resolves_semantic_variants( + client: FlaskClient, full_api_access: None, mocker: MockerFixture, ambiguous: bool +) -> None: + """The public endpoint uses the real model resolver on cache misses.""" + raw: Dimension = Dimension("raw", "event_time", pa.timestamp("us")) + month: Dimension = Dimension( + "month", "event_time", pa.timestamp("us"), grain=Grains.MONTH + ) + other: Dimension = Dimension( + "other-month", "event_time", pa.timestamp("us"), grain=Grains.MONTH + ) + metric: Metric = Metric("count", "count", pa.int64(), "COUNT(*)") + provider: MagicMock = MagicMock() + provider.uid.return_value = "semantic-view-73" + provider.get_metrics.return_value = {metric} + provider.get_dimensions.return_value = ( + [raw, month, other] if ambiguous else [raw, month] + ) + provider.get_compatible_metrics.return_value = {metric} + provider.get_compatible_dimensions.return_value = {raw, month} + view: SemanticView = SemanticView(id=73, name="events") + view.implementation = provider + mocker.patch( + "superset.datasource.api.DatasourceDAO.get_datasource", return_value=view + ) + mocker.patch.object(SemanticView, "raise_for_access") + cache: MagicMock = mocker.patch("superset.datasource.api.cache_manager").data_cache + cache.get.return_value = None + response: TestResponse = client.post( + "/api/v1/datasource/semantic_view/73/compatible", + json={"selected_metrics": ["count"], "selected_dimensions": ["event_time"]}, + ) + if ambiguous: + assert response.status_code == 400 + assert response.json is not None + assert "ambiguous" in response.json["message"] + provider.get_compatible_metrics.assert_not_called() + provider.get_compatible_dimensions.assert_not_called() + cache.set.assert_not_called() + else: + assert response.status_code == 200 + assert response.json is not None + assert response.json["result"] == { + "compatible_metrics": ["count"], + "compatible_dimensions": ["event_time"], Review Comment: Could you take another look at the current test? Its dimension mock already returns `{raw, month}`; replacing that with `{metric}` makes the existing assertion fail, so the current test does detect the proposed mistake. ########## superset/semantic_layers/models.py: ########## @@ -795,7 +759,9 @@ def get_compatible_metrics( view implementation, and translates the result back to names. """ metric_map = {m.name: m for m in self.implementation.get_metrics()} - dim_map = {d.name: d for d in self.implementation.get_dimensions()} + dim_map: dict[str, Dimension] = resolve_dimensions( + self.implementation.get_dimensions(), usage=DimensionUsage.COMPATIBILITY + ) Review Comment: Good catch; both compatibility projections now use a shared typed selection helper while retaining their distinct provider callbacks and original provider identities. The focused compatibility and semantic suites pass. ########## superset/common/utils/time_range_utils.py: ########## @@ -16,16 +16,45 @@ # under the License. from __future__ import annotations +from collections.abc import Mapping, Sequence from datetime import datetime from typing import Any, cast from flask import current_app from superset.common.query_object import QueryObject -from superset.utils.core import FilterOperator +from superset.constants import NO_TIME_RANGE +from superset.superset_typing import Column +from superset.utils.core import ( + FilterOperator, + get_x_axis_label, +) from superset.utils.date_parser import get_since_until +def get_time_range_from_filters( + time_range: str | None, + filters: Sequence[Mapping[str, object]] | None, + columns: list[Column] | None, +) -> str: + """Select the original range expression without evaluating relative dates.""" + if time_range is not None: + return time_range + temporal_filters: list[Mapping[str, object]] = [ + filter_ + for filter_ in filters or [] + if filter_.get("op") == FilterOperator.TEMPORAL_RANGE + ] Review Comment: Good catch; non-string values are now excluded before selecting a temporal range, with the same rule applied to fallback time-axis selection. The red-first tests also cover an invalid filter preceding a valid filter on another column, so the valid bounds cannot be applied to the wrong column. -- 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]
