bito-code-review[bot] commented on code in PR #44406:
URL: https://github.com/apache/superset/pull/44406#discussion_r4133634486
##########
tests/unit_tests/common/test_query_context_processor.py:
##########
@@ -2226,6 +2396,180 @@ def
test_get_df_payload_no_warning_when_not_memory_limited() -> None:
assert result["warning"] is None
+def
test_get_df_payload_result_decouples_annotation_cache_from_dataframe_cache():
+ """
+ The dataframe cache entry must stay shareable across viewers, and
+ annotation-layer data must be resolved through its own (per-user) cache
+ path -- not stored on the dataframe's cache entry -- so that two viewers
+ of the same annotated chart share one dataframe cache hit while each
+ still gets their own annotation-security-scoped payload.
+ """
+ from superset.common.query_object import QueryObject
+
+ mock_query_context = MagicMock()
+ mock_query_context.force = False
+ mock_datasource = MagicMock()
+ mock_datasource.column_names = ["col1"]
+
+ processor = QueryContextProcessor(mock_query_context)
+ processor._qc_datasource = mock_datasource
+
+ query_obj = QueryObject(
+ datasource=mock_datasource,
+ columns=["col1"],
+ annotation_layers=[
+ {
+ "annotationType": "EVENT",
+ "sourceType": "NATIVE",
+ "name": "a",
+ "value": 1,
+ }
+ ],
+ )
+
+ class MockCache:
+ def __init__(self):
+ self.is_loaded = True
+ self.applied_filter_columns = ["col1"]
+ self.df = pd.DataFrame({"col1": [1, 2, 3]})
+ self.query = ""
+ self.status = "success"
+ self.cache_dttm = "2024-01-01T00:00:00"
+ self.queried_dttm = "2024-01-01T00:00:00"
+ self.stacktrace = None
+ self.error_message = None
+ self.is_cached = True
+ self.sql_rowcount = 0
+ self.cache_value = None
+ self.applied_template_filters = []
+ self.rejected_filter_columns = []
+ self.annotation_data = {"stale": "should not be used"}
+ self.bq_memory_limited = False
+ self.bq_memory_limited_row_count = 0
+ self.result_persisted = False
+ self.set_query_result = MagicMock()
+
+ mock_cache = MockCache()
+
+ with (
+ patch(
+ "superset.common.query_context_processor.QueryCacheManager"
+ ) as mock_cache_manager,
+ patch.object(query_obj, "validate", return_value=None),
+ patch.object(processor, "query_cache_key", return_value="df-key"),
+ patch.object(processor, "annotation_cache_key",
return_value="ann-key"),
+ patch.object(
+ processor, "_get_annotation_data_cached", return_value={"a": [1,
2]}
+ ) as mock_get_annotation,
+ patch.object(processor, "get_cache_timeout", return_value=3600),
+ ):
+ mock_cache_manager.get.return_value = mock_cache
+ result = processor.get_df_payload(query_obj, force_cached=False)
+
+ # The dataframe cache is a hit, so the (expensive) query is never re-run,
+ # and its cache entry is never rewritten.
+ mock_cache.set_query_result.assert_not_called()
+
+ # Annotation data is resolved through the separate, per-user cache path,
+ # keyed by the dedicated annotation cache key -- not the dataframe's key.
+ mock_get_annotation.assert_called_once()
+ _, kwargs = mock_get_annotation.call_args
+ assert kwargs["cache_key"] == "ann-key"
+
+ # The payload serves the freshly-resolved annotation data, not whatever
+ # (stale) value happened to sit on the dataframe's cache object.
+ assert result["annotation_data"] == {"a": [1, 2]}
+
+
+def test_get_df_payload_result_annotation_refresh_independent_of_df_marker():
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing type annotations</b></div>
<div id="fix">
BITO.md adaptive rules 7819/12587/12787 require explicit type annotations on
new test code, and 12147 requires docstrings on helpers. This test lacks `->
None`; nested `marker_lookup` lacks a return hint and docstring;
`MockCache.__init__` and the mock variables lack annotations. Adding them
satisfies the org typing standard for new Python code.
</div>
</div>
<small><i>Code Review Run #df4733</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
##########
tests/unit_tests/common/test_query_context_processor.py:
##########
@@ -2226,6 +2396,180 @@ def
test_get_df_payload_no_warning_when_not_memory_limited() -> None:
assert result["warning"] is None
+def
test_get_df_payload_result_decouples_annotation_cache_from_dataframe_cache():
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing Return Type Hint</b></div>
<div id="fix">
BITO.md adaptive rule 7819 mandates explicit return type hints on all
functions, explicitly including test methods. This new test omits `-> None:`,
while sibling `test_get_df_payload_bq_memory_limited_warning` (line 2283)
already annotates it. Adding the one-line hint keeps the mandated standard
uniform and enables static type checking.
</div>
</div>
<small><i>Code Review Run #df4733</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]
