codeant-ai-for-open-source[bot] commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r3595462498
##########
superset/common/utils/query_cache_manager.py:
##########
@@ -192,6 +283,9 @@ def get(
query_cache.applied_filter_columns = cache_value.get(
"applied_filter_columns", []
)
+ query_cache.has_applied_filter_columns = (
+ "applied_filter_columns" in cache_value
+ )
Review Comment:
**Suggestion:** Marking legacy cache entries as missing
`applied_filter_columns` causes them to be treated as cache misses later, but
this happens after the `force_cached` check has already passed; as a result,
cache-only requests can silently fall through to live DB execution instead of
raising a cache-load error. Preserve force-cached semantics by failing fast (or
raising) when this legacy-miss condition is detected under cache-only mode.
[api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ /api/v1/chart/data/<cache_key> can hit database.
- ⚠️ Async chart prefetch may execute queries despite cache-only mode.
- ⚠️ force_cached option no longer guarantees no database execution.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Ensure the DATA cache contains a legacy entry whose value dict lacks the
"applied_filter_columns" key but does contain "df" and "query" for some
filtered chart
query (so `_cache[CacheRegion.DATA].get(key)` returns a truthy `cache_value`
without that
key when `QueryCacheManager.get()` is called at
`superset/common/utils/query_cache_manager.py:253-268`).
2. Call the cache-only chart-data endpoint `GET
/api/v1/chart/data/<cache_key>`, which is
implemented by `ChartDataRestApi.data_from_cache` in
`superset/charts/data/api.py:15-79`;
this loads the saved form data, builds a `QueryContext`, constructs a
`ChartDataCommand`,
and then calls `_get_data_response(command, True)` (line 79), passing
`force_cached=True`.
3. Inside `_get_data_response`,
`command.execute(ChartDataExecutionOptions(force_cached=force_cached))` is
invoked at
`superset/charts/data/api.py:176-182`, which flows into
`ChartDataCommand.execute`
(`superset/commands/chart/data/get_data_command.py:35-47`); there,
`QueryContext.get_payload_result(cache_query_context=..., force_cached=True,
materialize=True)` is called (lines 42-46), which delegates to
`QueryContextProcessor.get_payload_result` and then to `_execute_query_plan`
(`superset/common/query_context_processor.py:128-136, 468-93`), which
acquires data via
`acquire_query_data(..., force_cached=True, ...)` and finally reaches
`QueryContextProcessor.get_df_payload_result`
(`superset/common/query_context_processor.py:19-45`).
4. In `get_df_payload_result`, `QueryCacheManager.get` is called with
`force_cached=True`
at `superset/common/query_context_processor.py:39-45`; because the legacy
cache entry
exists, `QueryCacheManager.get` loads it, sets `query_cache.is_loaded =
True` and
`query_cache.has_applied_filter_columns = ("applied_filter_columns" in
cache_value)`
(False for legacy entries) at
`superset/common/utils/query_cache_manager.py:277-288`, and
does not raise in the `if force_cached and not query_cache.is_loaded` block
at lines
325-329. Back in `get_df_payload_result`, the legacy-miss check at
`superset/common/query_context_processor.py:48-57` sees a non-empty
`query_obj.filter`,
`cache.is_loaded` True, and `has_applied_filter_columns` False, so it calls
`cache.discard_loaded_value()` (line 58), which resets `is_loaded` to False
in
`discard_loaded_value` at
`superset/common/utils/query_cache_manager.py:102-124`. The
subsequent block `if query_obj and cache_key and not cache.is_loaded:` at
`superset/common/query_context_processor.py:66-92` then executes
`self.get_query_result(query_obj)`, issuing a live database query even though
`force_cached=True`. No `CacheLoadError` is raised anywhere, so
`ChartDataCommand.execute`
never converts this into a `ChartDataCacheLoadError` (its `except
CacheLoadError` at
`superset/commands/chart/data/get_data_command.py:47-48` is not triggered),
and
`ChartDataRestApi.data_from_cache` returns a 200 response backed by a fresh
DB query
instead of treating the situation as a cache load failure.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8fbfd18f0372412c8f824013b1f2b788&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8fbfd18f0372412c8f824013b1f2b788&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/utils/query_cache_manager.py
**Line:** 286:288
**Comment:**
*Api Mismatch: Marking legacy cache entries as missing
`applied_filter_columns` causes them to be treated as cache misses later, but
this happens after the `force_cached` check has already passed; as a result,
cache-only requests can silently fall through to live DB execution instead of
raising a cache-load error. Preserve force-cached semantics by failing fast (or
raising) when this legacy-miss condition is detected under cache-only mode.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=dd8ae0efd8dc80a20913701b55629a917298a18651ce26da4e3d6f8b28f87e0c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=dd8ae0efd8dc80a20913701b55629a917298a18651ce26da4e3d6f8b28f87e0c&reaction=dislike'>👎</a>
--
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]
