sadpandajoe commented on code in PR #44835:
URL: https://github.com/apache/superset/pull/44835#discussion_r4201115672
##########
superset/initialization/__init__.py:
##########
@@ -163,10 +163,23 @@ class AppContextTask(task_base): # type: ignore
# nested context here would silently hand the task a second,
# blind session unable to see the caller's uncommitted work.
def __call__(self, *args: Any, **kwargs: Any) -> Any:
- if has_app_context():
- return task_base.__call__(self, *args, **kwargs)
- with superset_app.app_context():
- return task_base.__call__(self, *args, **kwargs)
+ # Avoid circular import through superset.app during
initialization.
+ from superset.semantic_layers.metadata_binding import
metadata_operation
+
+ with (
+ contextlib.nullcontext()
+ if has_app_context()
+ else superset_app.app_context()
+ ):
+ with (
+ metadata_operation()
Review Comment:
This gives each Celery task a single metadata budget that starts when the
task starts. A task like the dashboard Excel export runs every chart in one
task, so if earlier charts' warehouse queries take longer than the budget, a
later chart on a semantic view fails with a `deadline` error on its first
metadata access, even with a warm snapshot and healthy Redis, and its sheet is
dropped. Would it make sense to capture the views a task needs up front, or
scope the budget to each chart's metadata acquisition?
##########
superset/common/query_context_processor.py:
##########
@@ -478,7 +557,18 @@ def _annotation_cache_context(self, query_obj:
QueryObject) -> dict[str, Any]:
if annotation_datasource
else None
)
- return {"user_id": get_user_id(), "source_rls": source_rls}
+ metadata_datasource: Datasource | None = (
+ chart.resolved_datasource if chart else None
Review Comment:
The annotation part of the cache key takes its metadata token from
`chart.resolved_datasource`, while the capture above and the annotation
executor use `chart.get_query_context().datasource`. If a chart's datasource
was changed through the API and its saved query context was left pointing at
the old view, annotation data fetched from view B gets cached under view A's
token (or no token if A doesn't participate), so a refresh of B never
invalidates it. Should the key use the same view the annotation query actually
runs against?
##########
superset/semantic_layers/api.py:
##########
@@ -731,10 +736,19 @@ def runtime_schema(self, uuid: str) -> FlaskResponse:
return self.response_400(message=f"Unknown type: {layer.type}")
try:
- schema = cls.get_runtime_schema(
- layer.implementation.configuration, # type:
ignore[attr-defined]
- runtime_data,
- )
+ schema: dict[str, Any]
+ if metadata_binding.participates(layer):
+ adapter: MetadataRefreshAdapter | None = (
+ layer.implementation.metadata_refresh
+ )
+ if adapter is None:
+ raise MetadataRefreshError("configuration")
Review Comment:
When shared metadata storage is down or discovery runs out of time, this
path raises a typed `MetadataRefreshError`, but the broad `except Exception`
just below turns it into HTTP 400 with only `unavailable`/`deadline` as the
message. The same happens in `/compatible`, where the typed error falls through
`@safe` to a generic 500. Clients get none of the 503/504 mapping or localized
messages from `metadata_errors.py`, and the docs say storage failures are 503
and deadline expiry is 504. Should `metadata_api_errors` (or an explicit
`except MetadataRefreshError`) be applied to these REST endpoints in this PR,
or is that deliberately left for a follow-up?
##########
tests/unit_tests/semantic_layers/metadata_identity_test.py:
##########
@@ -0,0 +1,715 @@
+# 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.
+
+"""Exercise metadata identity through the real QueryContext result cache."""
+
+from __future__ import annotations
+
+from collections.abc import Callable
+from datetime import datetime, timedelta
+from typing import Any, cast
+from unittest.mock import Mock, patch
+from uuid import UUID
+
+import pyarrow as pa
+import pytest
+from flask import Flask
+from flask_caching import Cache
+from superset_core.semantic_layers.metadata import MetadataRefreshErrorCategory
+from superset_core.semantic_layers.types import Metric, SemanticQuery,
SemanticResult
+
+from superset.common.chart_data import ChartDataResultFormat,
ChartDataResultType
+from superset.common.query_context import QueryContext
+from superset.common.query_object import QueryObject
+from superset.common.utils import query_cache_manager
+from superset.connectors.sqla.models import BaseDatasource
+from superset.constants import CacheRegion
+from superset.explorables.base import Explorable
+from superset.semantic_layers.models import SemanticLayer, SemanticView
+from superset.semantic_layers.registry import registry
+from tests.unit_tests.semantic_layers.metadata_contract_test import (
+ LegacyLayer,
+ LegacyView,
+)
+
+
+class ResultView(LegacyView):
+ def __init__(self, token: str, value: int) -> None:
+ self.token: str = token
+ self.value: int = value
+ self.calls: int = 0
+
+ @property
+ def metadata_cache_token(self) -> str:
+ return self.token
+
+ def get_metrics(self) -> set[Metric]:
+ return {Metric("orders", "orders", pa.int64(), "orders")}
+
+ def get_table(self, query: SemanticQuery) -> SemanticResult:
+ self.calls += 1
+ return SemanticResult([], pa.table({"orders": [self.value]}))
+
+
+class RefreshLayer(LegacyLayer):
+ @classmethod
+ def supports_metadata_refresh(cls, configuration: dict[str, Any]) -> bool:
+ return True
+
+
+def view_for(provider: ResultView) -> SemanticView:
+ layer: SemanticLayer = SemanticLayer(
+ uuid=UUID("00000000-0000-0000-0000-000000000011"),
+ type="cache-test",
+ configuration="{}",
+ )
+ view: SemanticView = SemanticView(
+ id=11,
+ uuid=UUID("00000000-0000-0000-0000-000000000012"),
+ name="orders",
+ configuration="{}",
+ semantic_layer=layer,
+ changed_on=datetime(2026, 1, 1),
+ cache_timeout=300,
+ )
+ view.__dict__["_fixture_implementation"] = provider
+ return view
+
+
+def context_for(view: Explorable) -> tuple[QueryContext, QueryObject]:
+ query: QueryObject = QueryObject(
+ datasource=cast(BaseDatasource, view), metrics=["orders"], row_limit=10
+ )
+ context: QueryContext = QueryContext(
+ datasource=view,
+ queries=[query],
+ slice_=None,
+ form_data=None,
+ result_type=ChartDataResultType.FULL,
+ result_format=ChartDataResultFormat.JSON,
+ cache_values={},
+ )
+ return context, query
+
+
+def test_same_name_and_discovery_after_refresh_cannot_reuse_old_query_result(
Review Comment:
`view_cache_token` mixes the view's stored configuration into the cache key,
but the `view_for` fixture here pins `configuration="{}"` (and uuid/name) for
every view, so nothing exercises it. If someone dropped the configuration from
that hash, two views that share a catalog token and name but have different
configurations would share compatibility and query-result cache entries and all
of these tests would still pass. Could you add a case with two views differing
only in configuration, with the same selection and `changed_on`, asserting the
second one gets its own compatibility answer and an uncached query result?
--
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]
