codeant-ai-for-open-source[bot] commented on code in PR #37516: URL: https://github.com/apache/superset/pull/37516#discussion_r3657744134
########## superset/common/query_context_processor.py: ########## @@ -41,7 +70,10 @@ from superset.models.helpers import QueryResult from superset.superset_typing import AdhocColumn, AdhocMetric from superset.utils import csv, excel -from superset.utils.cache import generate_cache_key, set_and_log_cache +from superset.utils.cache import ( + generate_cache_key, + set_and_log_cache_with_outcome, +) Review Comment: **Suggestion:** The newly imported `generate_cache_key` is never referenced in this file. Remove the unused import to avoid dead code and prevent lint/type-check failures for unused imports. [possible bug] <details> <summary><b>Severity Level:</b> Minor ๐งน</summary> ```mdx - โ ๏ธ Python lint may fail for `query_context_processor.py`. - โ ๏ธ CI validation can block chart-data changes. - โ ๏ธ Unused import adds dead code to the cache refactor. ``` </details> <details> <summary><b>Steps of Reproduction โ </b></summary> ```mdx 1. Load `superset/common/query_context_processor.py`, which is imported when chart query processing constructs or invokes `QueryContextProcessor`. 2. During module analysis, inspect the import block at `superset/common/query_context_processor.py:73-76`; it imports `generate_cache_key` and `set_and_log_cache_with_outcome`. 3. Trace the cache-related methods in the same file, including `query_cache_key()` at line 379 and `get_payload_result()` at lines 452-482; these use the processor's own cache-key logic and `set_and_log_cache_with_outcome`, but never reference `generate_cache_key`. 4. Run the repository's Python lint/type-check pipeline against this module; an unused-import rule reports `generate_cache_key` as `F401`/unused, potentially failing CI even though normal chart execution does not raise a runtime exception. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=be42ac7af0b64268a7fca46d9eff168a&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=be42ac7af0b64268a7fca46d9eff168a&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/query_context_processor.py **Line:** 73:76 **Comment:** *Possible Bug: The newly imported `generate_cache_key` is never referenced in this file. Remove the unused import to avoid dead code and prevent lint/type-check failures for unused imports. 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=e8e961d7fc9dbdc25d30816304ffcd194ff05dc8728e06080f699156e37ef429&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=e8e961d7fc9dbdc25d30816304ffcd194ff05dc8728e06080f699156e37ef429&reaction=dislike'>๐</a> ########## superset/charts/data/api.py: ########## @@ -434,44 +471,63 @@ def _run_async( # First, look for the chart query results in the cache, # but only if we're not forcing a refresh. if not form_data.get("force"): - with contextlib.suppress(ChartDataCacheLoadError): - result = command.run(force_cached=True) - if result is not None: - # Log is_cached if extra payload callback is provided. - # This indicates no async job was triggered - data was already - # cached and a synchronous response is being returned immediately. - self._log_is_cached(result, add_extra_log_payload) - return self._send_chart_response(result) + result = None + try: + with chart_timing_phase("query"): + result = command.execute( + ChartDataExecutionOptions(force_cached=True) + ) + except ChartDataCacheLoadError: + pass + except ChartDataQueryFailedError as exc: + # The cached query result recorded an error; surface it the same + # way the synchronous path does instead of letting it escape as + # an unhandled server error. + return self.response_400(message=exc.message) + if result is not None: + # Log is_cached if extra payload callback is provided. + # This indicates no async job was triggered - data was already + # cached and a synchronous response is being returned immediately. + self._log_is_cached(result, add_extra_log_payload) + return self._send_chart_response(result) # Otherwise, kick off a background job to run the chart query. # Clients will either poll or be notified of query completion, # at which point they will call the /data/<cache_key> endpoint # to retrieve the results. - async_command = CreateAsyncChartDataJobCommand() - try: - async_command.validate(request) - except AsyncQueryTokenException: - return self.response_401() + with chart_timing_phase("enqueue"): + async_command = CreateAsyncChartDataJobCommand() + try: + async_command.validate(request) + except AsyncQueryTokenException: + return self.response_401() - result = async_command.run(form_data, get_user_id()) - return self.response(202, **result) + async_result = async_command.run(form_data, get_user_id()) + return self.response(202, **async_result) def _send_chart_response( # noqa: C901 self, - result: dict[Any, Any], + execution: ChartDataExecutionResult, form_data: dict[str, Any] | None = None, datasource: BaseDatasource | Query | None = None, filename: str | None = None, expected_rows: int | None = None, dashboard_filter_context: DashboardFilterContext | None = None, ) -> Response: + result = execution.materialize() result_type = result["query_context"].result_type result_format = result["query_context"].result_format # Post-process the data so it matches the data presented in the chart. # This is needed for sending reports based on text charts that do the # post-processing of data, eg, the pivot table. if result_type == ChartDataResultType.POST_PROCESSED: - result = apply_client_processing(result, form_data, datasource) + with chart_timing_phase("client"): + result = apply_client_processing(result, form_data, datasource) + + for query, query_result in zip( + result["queries"], execution.queries, strict=True + ): + project_query_timing(query, query_result.timing) Review Comment: **Suggestion:** The query timing is projected into every response unconditionally, so timing data will be exposed even when the documented `CHART_DATA_INCLUDE_TIMING` configuration is disabled. Gate this projection on the configuration setting, or omit it from the payload when timing output is disabled. [logic error] <details> <summary><b>Severity Level:</b> Major โ ๏ธ</summary> ```mdx - โ Chart API responses expose timing despite configuration. - โ ๏ธ Clients cannot disable response timing as documented. - โ ๏ธ Response shape changes for all JSON chart queries. ``` </details> <details> <summary><b>Steps of Reproduction โ </b></summary> ```mdx 1. Enable the chart data API and send an authenticated JSON request to `POST /api/v1/chart/data` implemented by `ChartDataRestApi.data()` at `superset/charts/data/api.py:287`, with a valid chart query. 2. The request reaches `_get_data_response()` at `superset/charts/data/api.py:627`, which executes the query and passes the resulting `ChartDataExecutionResult` to `_send_chart_response()`. 3. `_send_chart_response()` iterates over every result query at `superset/charts/data/api.py:527-530` and unconditionally calls `project_query_timing(query, query_result.timing)`. 4. The projected timing fields are therefore placed into the JSON query payload before serialization at `superset/charts/data/api.py:592-598`, even when `CHART_DATA_INCLUDE_TIMING` is disabled. The same path is used by `GET /api/v1/chart/<pk>/data/` through `ChartDataRestApi.get_data()` at `superset/charts/data/api.py:102-111`. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=35d6738d7f53411f8d9927422db4a2c4&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=35d6738d7f53411f8d9927422db4a2c4&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/charts/data/api.py **Line:** 527:530 **Comment:** *Logic Error: The query timing is projected into every response unconditionally, so timing data will be exposed even when the documented `CHART_DATA_INCLUDE_TIMING` configuration is disabled. Gate this projection on the configuration setting, or omit it from the payload when timing output is disabled. 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=d07069e7c21b0a4fedd5c275652823b6248a0702f5faef5e5b7b95b48b59cc21&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=d07069e7c21b0a4fedd5c275652823b6248a0702f5faef5e5b7b95b48b59cc21&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]
