codeant-ai-for-open-source[bot] commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r3571145897
##########
superset/commands/chart/data/get_data_command.py:
##########
@@ -25,49 +27,111 @@
ChartDataQueryFailedError,
)
from superset.common.chart_data import ChartDataResultType
+from superset.common.chart_data_timing import (
+ CacheWriteOutcome,
+ chart_data_execution_scope,
+ ChartDataExecutionResult,
+ emit_query_timing,
+)
from superset.common.query_context import QueryContext
-from superset.exceptions import CacheLoadError
+from superset.exceptions import CacheLoadError, QueryObjectValidationError
logger = logging.getLogger(__name__)
+class ChartDataExecutionMode(str, Enum):
+ """Controls whether query data is returned or only persisted to cache."""
+
+ MATERIALIZE = "materialize"
+ CACHE_ONLY = "cache_only"
+
+
+@dataclass(frozen=True)
+class ChartDataExecutionOptions:
+ """Immutable options for one chart-data execution."""
+
+ mode: ChartDataExecutionMode = ChartDataExecutionMode.MATERIALIZE
+ force_cached: bool = False
+ cache_query_context: bool = False
+ require_cache_writes: bool = False
+
+
class ChartDataCommand(BaseCommand):
_query_context: QueryContext
def __init__(self, query_context: QueryContext):
self._query_context = query_context
def run(self, **kwargs: Any) -> dict[str, Any]:
- # caching is handled in query_context.get_df_payload
- # (also evals `force` property)
- cache_query_context = kwargs.get("cache", False)
- force_cached = kwargs.get("force_cached", False)
- try:
- payload = self._query_context.get_payload(
- cache_query_context=cache_query_context,
force_cached=force_cached
- )
- except CacheLoadError as ex:
- raise ChartDataCacheLoadError(ex.message) from ex
-
- # Skip error check for query-only requests - errors are returned in
payload
- # This allows View Query modal to display validation errors
- for query in payload["queries"]:
- if (
- query.get("error")
- and self._query_context.result_type !=
ChartDataResultType.QUERY
- ):
- raise ChartDataQueryFailedError(
- _("Error: %(error)s", error=query["error"])
+ """Execute and return the historical payload shape."""
+ options = ChartDataExecutionOptions(
+ force_cached=kwargs.get("force_cached", False),
+ cache_query_context=kwargs.get("cache", False),
+ )
+ return self.execute(options).materialize()
+
+ def execute(
+ self, options: ChartDataExecutionOptions | None = None
+ ) -> ChartDataExecutionResult:
+ """Execute using a typed result with timing outside query payloads."""
+ options = options or ChartDataExecutionOptions()
+ with chart_data_execution_scope() as owns_telemetry:
+ try:
+ result = self._query_context.get_payload_result(
+ cache_query_context=options.cache_query_context,
+ force_cached=options.force_cached,
+ materialize=options.mode ==
ChartDataExecutionMode.MATERIALIZE,
)
+ except CacheLoadError as ex:
+ raise ChartDataCacheLoadError(ex.message) from ex
+ except QueryObjectValidationError as ex:
+ raise ChartDataQueryFailedError(ex.message) from ex
+
+ execution = ChartDataExecutionResult(
+ query_context=self._query_context,
+ queries=result.queries,
+ cache_key=result.cache_key,
+ )
+
+ if owns_telemetry:
+ for query in result.queries:
+ emit_query_timing(query.timing)
- return_value = {
- "query_context": self._query_context,
- "queries": payload["queries"],
- }
- if cache_query_context:
- return_value.update(cache_key=payload["cache_key"])
+ # Query-only errors remain in the payload for the View Query modal.
+ for query in result.queries:
+ if (
+ query.payload.get("error")
+ and self._query_context.result_type !=
ChartDataResultType.QUERY
+ ):
+ raise ChartDataQueryFailedError(
+ _("Error: %(error)s", error=query.payload["error"])
+ )
+
+ if options.mode == ChartDataExecutionMode.CACHE_ONLY:
+ query_cache_failed = any(
+ query.timing.cache_write_outcome ==
CacheWriteOutcome.FAILED
+ or (
+ options.require_cache_writes
+ and query.timing.cache_hit is False
+ and query.timing.cache_write_outcome
+ != CacheWriteOutcome.SUCCEEDED
Review Comment:
**Suggestion:** When `require_cache_writes` is enabled, this condition only
treats query cache writes as missing if `cache_hit is False`. If cache writing
was required but never attempted (`cache_hit` remains `None`, e.g. no cache
key/path), the command incorrectly succeeds. Update the check to also fail on
non-`SUCCEEDED` outcomes when write-required mode is active, regardless of
`cache_hit`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Async load_chart_data_into_cache may succeed without query cache.
- ⚠️ /api/v1/chart/data/<cache_key> may hit uncached queries.
- ⚠️ Strict cache-only semantics inconsistent for metadata-only query types.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the chart data API `/api/v1/chart/data` with a JSON body whose
top-level
`result_type` is `FULL` but where one entry in `queries` sets its own
`result_type` to
`ChartDataResultType.QUERY` (per-query `result_type` is allowed by
`ChartDataQueryObjectSchema` in `superset/charts/schemas.py:1233-1239`).
2. With `GLOBAL_ASYNC_QUERIES` enabled and cache timeout not disabled, the
handler
`ChartDataRestApi.data` in `superset/charts/data/api.py:86-107` sets
`use_async` and
enqueues an async job via `_run_async`, which ultimately calls
`CreateAsyncChartDataJobCommand.run` and schedules
`load_chart_data_into_cache` with this
form data.
3. When the Celery worker executes `load_chart_data_into_cache` in
`superset/tasks/async_queries.py:87-110`, it constructs a `ChartDataCommand`
and calls
`execute(ChartDataExecutionOptions(mode=ChartDataExecutionMode.CACHE_ONLY,
cache_query_context=True, require_cache_writes=True))`, turning on strict
cache-write mode
for all queries in the context.
4. Inside `QueryContextProcessor._execute_query_plan` at
`superset/common/query_context_processor.py:183-199`, the metadata-only query
(result_type=`QUERY`) is processed via `get_query_results_cache_only`, which
for non-data
result types returns `_metadata_result` from
`superset/common/query_actions.py:259-268`.
That function constructs a `QueryDataResult` whose `timing.cache_hit` is
`None` and
`timing.cache_write_outcome` is `CacheWriteOutcome.NOT_ATTEMPTED`. Back in
`ChartDataCommand.execute` at
`superset/commands/chart/data/get_data_command.py:110-119`,
`query_cache_failed` is computed using the condition at lines 114-117: the
inner branch
requires `options.require_cache_writes` and `query.timing.cache_hit is
False` before
checking `cache_write_outcome != CacheWriteOutcome.SUCCEEDED`. Because
`cache_hit` is
`None` for this metadata query, the branch does not fire,
`query_cache_failed` remains
`False`, and no `ChartDataQueryFailedError` is raised even though strict
mode was
requested and no cache write was attempted for that query.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=80b2c5a418ef44b28e5d159fdb0e83a9&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=80b2c5a418ef44b28e5d159fdb0e83a9&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/data/get_data_command.py
**Line:** 115:117
**Comment:**
*Logic Error: When `require_cache_writes` is enabled, this condition
only treats query cache writes as missing if `cache_hit is False`. If cache
writing was required but never attempted (`cache_hit` remains `None`, e.g. no
cache key/path), the command incorrectly succeeds. Update the check to also
fail on non-`SUCCEEDED` outcomes when write-required mode is active, regardless
of `cache_hit`.
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=02df5ca8b91f7dc893726548b130731cffd59249497f0b85853c7535233658ae&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=02df5ca8b91f7dc893726548b130731cffd59249497f0b85853c7535233658ae&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]
