rusackas commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r3658014108
##########
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:
generate_cache_key isn't unused, it's called in cache_key() further down in
this file. Also wasn't newly imported here, it was already on master, just
set_and_log_cache got swapped for set_and_log_cache_with_outcome in that same
import block.
##########
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:
project_query_timing() already checks CHART_DATA_INCLUDE_TIMING internally
before it touches the payload, so the gating this is asking for is already
there, just inside the helper rather than at the call site.
--
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]
