codeant-ai-for-open-source[bot] commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r3610269659
##########
superset/common/utils/query_cache_manager.py:
##########
@@ -116,13 +166,17 @@ def set_query_result(
self.queried_dttm = (
datetime.now(tz=timezone.utc).replace(microsecond=0).isoformat()
)
+ self.source_trace = query_result.source_trace
if self.status != QueryStatus.FAILED:
- current_app.config["STATS_LOGGER"].incr("loaded_from_source")
- if not force_query:
- current_app.config["STATS_LOGGER"].incr(
- "loaded_from_source_without_force"
- )
+ try:
+
current_app.config["STATS_LOGGER"].incr("loaded_from_source")
+ if not force_query:
+ current_app.config["STATS_LOGGER"].incr(
+ "loaded_from_source_without_force"
+ )
+ except Exception: # pylint: disable=broad-except
+ logger.exception("Unable to emit source load metric")
self.is_loaded = True
Review Comment:
**Suggestion:** After loading a fresh source result, the cache state never
sets `is_cached` to `False`, so downstream payloads can return `null` for cache
status instead of a boolean. This breaks cache-hit/miss semantics for API
consumers and any logic that expects a strict bool; set the field explicitly
when loading from source. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chart data API returns null cache status for source.
- ⚠️ Cache-hit diagnostics in timing object become unreliable.
- ⚠️ Any strict-boolean consumers must defensively handle None.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Issue a chart data request to `GET /api/v1/chart/data` (handler in
`superset/charts/data/api.py`, which uses `ChartDataCommand` in
`superset/commands/chart/data/get_data_command.py`).
2. `ChartDataCommand` builds a `QueryContext` that attempts to use the cache
via
`QueryCacheManager.get(...)` in
`superset/common/utils/query_cache_manager.py:253`; for a
new query or expired key, this returns a fresh `QueryCacheManager` with
`is_cached`
initialized to `None` in `__init__` at
`superset/common/utils/query_cache_manager.py:69-74`.
3. The query is executed against the database, producing a `QueryResult`
(from
`superset/models/helpers.py:QueryResult`), and
`QueryCacheManager.set_query_result(...)`
at `superset/common/utils/query_cache_manager.py:127-146` is called; this
method delegates
to `load_query_result(...)` where the snippet at lines 171-180 runs, setting
`self.is_loaded = True` but never setting `self.is_cached` to `False` for
this fresh
source result.
4. `QueryContextProcessor.get_df_payload()` in
`superset/common/query_context_processor.py` (identified in the PR
description as the main
lifecycle method) builds the response payload, including cache
metadata/timing; it reads
the `QueryCacheManager.is_cached` field, which is still `None`, causing the
`/api/v1/chart/data` JSON response to expose `null` (or an undefined
boolean) for cache
status instead of a strict `true`/`false` value for a non-cached source
result.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=91d4b6eb3b89477fb25b614133e9d428&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=91d4b6eb3b89477fb25b614133e9d428&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:** 171:180
**Comment:**
*Logic Error: After loading a fresh source result, the cache state
never sets `is_cached` to `False`, so downstream payloads can return `null` for
cache status instead of a boolean. This breaks cache-hit/miss semantics for API
consumers and any logic that expects a strict bool; set the field explicitly
when loading from source.
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=8dc3b360eb16fe538c9c1a55cfeb8258b0caf43b3151e54fc6140362ed273925&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=8dc3b360eb16fe538c9c1a55cfeb8258b0caf43b3151e54fc6140362ed273925&reaction=dislike'>👎</a>
##########
superset/commands/chart/warm_up_cache.py:
##########
@@ -40,10 +45,10 @@
class ChartWarmUpCacheCommand(BaseCommand):
def __init__(
self,
- chart_or_id: Union[int, Slice],
- dashboard_id: Optional[int],
- extra_filters: Optional[str],
- ):
+ chart_or_id: int | Slice,
+ dashboard_id: int | None,
+ extra_filters: str | None,
+ ) -> None:
Review Comment:
**Suggestion:** The constructor now requires `dashboard_id` and
`extra_filters` without defaults, which will raise a `TypeError` at runtime for
existing call sites that instantiate this command with only a chart identifier.
Restore `None` defaults for these optional arguments to keep
backward-compatible invocation behavior. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard warm_up_cache endpoint crashes due to constructor TypeError.
- ❌ Async explore_json cache preloading fails for all charts.
- ⚠️ Server-side dashboard rendering relying on warm cache is broken.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with this PR code deployed and configured normally.
2. Call the warm-up cache endpoint `warm_up_cache()` defined in
`superset/views/core.py:723` (e.g., via dashboard initial load or direct
HTTP call), which
instantiates `ChartWarmUpCacheCommand(chart_id)` with only the chart
identifier.
3. The constructor at `superset/commands/chart/warm_up_cache.py:46-51` now
requires
`dashboard_id` and `extra_filters` positional arguments without defaults, so
the call from
`warm_up_cache()` provides fewer arguments than required.
4. Python raises a `TypeError: __init__() missing 2 required positional
arguments:
'dashboard_id' and 'extra_filters'`, causing the warm-up cache request to
fail and
breaking any feature relying on `ChartWarmUpCacheCommand` with the legacy
one-argument
invocation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4d768a315791445ab841d9c86c5a664c&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=4d768a315791445ab841d9c86c5a664c&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/commands/chart/warm_up_cache.py
**Line:** 46:51
**Comment:**
*Possible Bug: The constructor now requires `dashboard_id` and
`extra_filters` without defaults, which will raise a `TypeError` at runtime for
existing call sites that instantiate this command with only a chart identifier.
Restore `None` defaults for these optional arguments to keep
backward-compatible invocation behavior.
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=f12c0f37aba27b3d18e3c93847cb0ae2fdc8854d61039afea3cee98a3ee045b4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=f12c0f37aba27b3d18e3c93847cb0ae2fdc8854d61039afea3cee98a3ee045b4&reaction=dislike'>👎</a>
##########
superset/commands/chart/warm_up_cache.py:
##########
@@ -96,12 +101,14 @@ def _warm_up_non_legacy_cache(self, chart: Slice) ->
tuple[Any, Any]:
query_context.force = True
command = ChartDataCommand(query_context)
command.validate()
- payload = command.run()
+ execution: ChartDataExecutionResult = command.execute(
+ ChartDataExecutionOptions(mode=ChartDataExecutionMode.CACHE_ONLY)
+ )
Review Comment:
**Suggestion:** Using cache-only execution mode in a cache warm-up path
prevents database execution on cache miss, so this code path cannot actually
populate missing cache entries. Use an execution mode that allows computing and
writing results when the cache is cold. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Warm_up_cache endpoint fails to populate missing cache entries.
- ⚠️ Initial dashboard loads do not benefit from pre-caching.
- ⚠️ ChartDataCommand timing metrics do not reflect warm-up behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Deploy Superset with this PR code and ensure a chart has no existing
cached result (for
example, by creating a new chart or clearing its cache through normal
operations).
2. Trigger the cache warm-up path via the `warm_up_cache()` view in
`superset/views/core.py:723`, which creates a `ChartWarmUpCacheCommand` and
eventually
calls `_warm_up_non_legacy_cache()` in
`superset/commands/chart/warm_up_cache.py:87`.
3. Inside `_warm_up_non_legacy_cache`, at lines 101-106,
`ChartDataCommand.execute()` is
invoked with
`ChartDataExecutionOptions(mode=ChartDataExecutionMode.CACHE_ONLY)`, meaning
the execution path is restricted to cache reads and will not perform
database query
execution on a cache miss.
4. On a cold cache, `execution.queries[*].payload` will report a cache-load
error or
failure status, the loop at lines 108-113 will return that error, and the
cache entry
remains unpopulated, so subsequent chart requests still incur full DB
execution instead of
benefiting from the warm-up step.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=36b68f30569f43a2821a31234b858dac&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=36b68f30569f43a2821a31234b858dac&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/commands/chart/warm_up_cache.py
**Line:** 104:106
**Comment:**
*Logic Error: Using cache-only execution mode in a cache warm-up path
prevents database execution on cache miss, so this code path cannot actually
populate missing cache entries. Use an execution mode that allows computing and
writing results when the cache is cold.
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=3d435e70c138bd4bf198624ec2d2d2d8f0b708cbd1d3cc7ced7766b7d8b4d116&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=3d435e70c138bd4bf198624ec2d2d2d8f0b708cbd1d3cc7ced7766b7d8b4d116&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]
