bito-code-review[bot] commented on code in PR #44835:
URL: https://github.com/apache/superset/pull/44835#discussion_r4170664967
##########
superset/common/query_context_processor.py:
##########
@@ -318,8 +330,9 @@ def get_df_payload_result(
)
if cache.status != QueryStatus.FAILED:
+ assert cache_key is not None
cache.set_query_result(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Empty-string sentinel cache key</b></div>
<div id="fix">
Skipping persistence now relies on a sentinel: `set_query_result` only
writes when `self.is_loaded and key` is truthy (`query_cache_manager.py:155`),
so `""` means "do not store". That contract is implicit and two modules away;
any future change to that truthiness check silently re-enables writes under an
empty key. Prefer an explicit `cache: bool` parameter or a named sentinel.
</div>
</div>
<small><i>Code Review Run #5e86a2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/semantic_layers/metadata_cache.py:
##########
@@ -0,0 +1,109 @@
+# 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.
+
+"""Captured catalog identity for derived caches and read-only inspection."""
+
+from __future__ import annotations
+
+import hashlib
+from dataclasses import dataclass, field
+from typing import TYPE_CHECKING
+from uuid import uuid4
+
+from superset_core.semantic_layers.metadata import CatalogSnapshot,
MetadataRefreshError
+
+from superset.semantic_layers import metadata_binding
+from superset.semantic_layers.metadata import ScopedMetadataStore
+from superset.semantic_layers.metadata_binding import connection_store
+from superset.utils import json
+
+if TYPE_CHECKING:
+ from superset.semantic_layers.models import SemanticView
+
+
+@dataclass(frozen=True)
+class CompatibilityIdentity:
+ key: str = field(repr=False)
+ source_observed_at: str | None
+
+
+def view_cache_token(view: SemanticView, token: str) -> str:
+ """Include host view configuration without revealing it in cache keys."""
+ identity: str = json.dumps(
+ [token, str(view.uuid), view.name, json.loads(view.configuration)],
+ sort_keys=True,
+ separators=(",", ":"),
+ allow_nan=False,
+ )
+ return hashlib.sha256(identity.encode()).hexdigest()
+
+
+def annotation_cache_token(view: SemanticView) -> str | None:
+ """Key a host chart without discovering its annotation source's
metadata."""
+ if not metadata_binding.participates(view.semantic_layer):
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>MetadataRefreshError escapes fallback</b></div>
<div id="fix">
`metadata_binding.participates()` (metadata_binding.py:151-159) raises
`MetadataRefreshError` when stored layer configuration is unparseable or
`layer.type` is absent from `registry`. This call sits outside the try at line
59, so the exception escapes the documented soft fallback and propagates
uncaught through
`_annotation_cache_context`/`_query_cache_key`/`get_df_payload_result` (no
handler in that path), failing the whole chart request instead of forcing a
cache miss.
</div>
</div>
<small><i>Code Review Run #5e86a2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/semantic_layers/metadata_cache.py:
##########
@@ -0,0 +1,109 @@
+# 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.
+
+"""Captured catalog identity for derived caches and read-only inspection."""
+
+from __future__ import annotations
+
+import hashlib
+from dataclasses import dataclass, field
+from typing import TYPE_CHECKING
+from uuid import uuid4
+
+from superset_core.semantic_layers.metadata import CatalogSnapshot,
MetadataRefreshError
+
+from superset.semantic_layers import metadata_binding
+from superset.semantic_layers.metadata import ScopedMetadataStore
+from superset.semantic_layers.metadata_binding import connection_store
+from superset.utils import json
+
+if TYPE_CHECKING:
+ from superset.semantic_layers.models import SemanticView
+
+
+@dataclass(frozen=True)
+class CompatibilityIdentity:
+ key: str = field(repr=False)
+ source_observed_at: str | None
+
+
+def view_cache_token(view: SemanticView, token: str) -> str:
+ """Include host view configuration without revealing it in cache keys."""
+ identity: str = json.dumps(
+ [token, str(view.uuid), view.name, json.loads(view.configuration)],
+ sort_keys=True,
+ separators=(",", ":"),
+ allow_nan=False,
+ )
+ return hashlib.sha256(identity.encode()).hexdigest()
+
+
+def annotation_cache_token(view: SemanticView) -> str | None:
+ """Key a host chart without discovering its annotation source's
metadata."""
+ if not metadata_binding.participates(view.semantic_layer):
+ return None
+ try:
+ token: str | None = metadata_binding.peek_view_metadata_token(view)
+ if token:
+ return view_cache_token(view, token)
+ except MetadataRefreshError:
+ pass
+ # Missing/expired/unreadable metadata cannot establish that cached
annotation
+ # data is current. A unique key forces a miss, including across failed
reads.
+ return "uncaptured:" + uuid4().hex
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated cacheability sentinel</b></div>
<div id="fix">
The `"uncaptured:"` sentinel is duplicated here and in `_query_cache_key`'s
`token.startswith("uncaptured:")` check (query_context_processor.py:467) with
no shared constant. If the copies diverge, uncaptured tokens are treated as
cacheable and each request stores under a fresh uuid4 key, flooding the cache.
Extract a shared constant.
</div>
</div>
<small><i>Code Review Run #5e86a2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
