YuriyKrasilnikov commented on PR #37516:
URL: https://github.com/apache/superset/pull/37516#issuecomment-4958513312
@rusackas, thanks for catching this and for jumping in with `78f15f3`. Your
change was the right call for the sentinel-based implementation. I followed the
issue one level deeper and removed the sentinel itself, so the
endpoint-specific cleanup was absorbed by the typed execution-result rewrite.
Your fix directly exposed the architectural boundary that needed to change.
I also want to answer the earlier `CHART_DATA_INCLUDE_TIMING` question
explicitly. This PR introduces three independent operator settings:
- `CHART_DATA_INCLUDE_TIMING = False` opts in to the versioned per-query
timing object in the JSON response body;
- `CHART_DATA_SLOW_QUERY_THRESHOLD_MS = None` opts in to server-side warning
logs for queries above the configured threshold;
- `CHART_DATA_INCLUDE_SERVER_TIMING = False` opts in to request-level timing
in the `Server-Timing` HTTP header.
I used configuration rather than a temporary feature flag because this is a
deployment-level observability and API-contract policy, not a temporary UI
rollout. Response timing exposes operational details such as cache behavior and
source timing, so both public response surfaces remain disabled unless the
operator opts in. `STATS_LOGGER` metrics and slow-query logging are independent
of those response options.
That means `#37507` is an opt-in diagnostic capability in this
implementation rather than a default response-contract change. If the project
prefers default `True`, or a feature flag instead of a config, I'll switch it -
just say which.
On the two public surfaces, `CHART_DATA_INCLUDE_TIMING` adds structured
lifecycle and source timing to each query in the JSON body, while
`CHART_DATA_INCLUDE_SERVER_TIMING` adds request phases such as context
creation, authorization, execution, server-side client-response processing,
serialization, enqueueing, and total request time to the standard HTTP header
on successful JSON responses. Either can be enabled independently. The prior
`timing: null` versus omitted inconsistency also disappears: the field is
absent unless JSON timing is enabled.
## What changed in the implementation
Timing no longer travels inside query payloads as the private
`_chart_data_timing` sentinel. Acquisition returns a typed, out-of-band
execution result (`QueryAcquisitionTiming` -> `QueryTiming`), and only the
chart-data API projects it onto the public response. `/api/v1/query/`,
datasource samples, MCP, warm-up, async workers, and other legacy consumers
therefore cannot leak internal timing state, regardless of configuration.
The public JSON object has an explicit wire-schema version. Its current
shape begins with `{"version": 1, "query": {...}, "sources": [...]}` and
separates cache-key planning, cache reads, source acquisition, cache-write
outcome, materialization, and bounded nested source timing. `Server-Timing` is
a separate HTTP header, not part of that object.
## Included in this PR because the execution contract changed
The typed contract separates dependency acquisition from public rendering
and makes materialized, cache-only, mixed-result, preview, and streaming paths
share the same execution plan. While validating those paths, I fixed the
contract violations that would otherwise make the timing rewrite behaviorally
incorrect:
- helper queries have a stable `main -> rowcount -> display totals ->
contribution dependency` order, with producer indexes computed after pagination
helpers are inserted; otherwise totals can bind to the wrong result;
- display totals and contribution totals retain distinct producers;
otherwise the display-total payload can be replaced by contribution data;
- `QUERY`, MCP, and View Query paths no longer require a dataframe that they
do not render, and mixed result modes separate internal acquisition from public
rendering;
- contribution totals use the same dependency-aware plan in materialized and
cache-only modes; otherwise cache-only execution can store page-local
percentages as complete results;
- totals are derived before CSV/XLSX conversion; otherwise dependency
processing receives bytes or strings instead of a dataframe;
- cache writes distinguish `succeeded`, `failed`, `skipped`, and `not
attempted`, and a backend returning `None` is treated as a successful write;
otherwise valid cache backends are reported as failures and strict async
caching cannot determine the real outcome;
- failed results are not written, and cache-write timing excludes
`load_query_result`; otherwise cache state and timing phases describe work that
belongs to another lifecycle stage;
- AUTO currency detection is an acquisition dependency over grouped
currencies and stops after a failed primary query; otherwise arbitrary sample
rows can report a false single currency and dependency work can run after
failure;
- series-limit prequeries remain deferred during cache-key calculation,
including `force_cached`; otherwise database work can happen before cache
lookup or during a cache-only read;
- native and legacy annotations participate in planning, execution, and
processing timing, while nested source traces remain bounded and parent phases
exclude child execution;
- executable SQL previews are preserved instead of being replaced by
deferred placeholders;
- the existing `StreamingCSVExportCommand` compiles executable SQL under the
deferred-source-query contract; this is a compatibility fix in this PR, not the
broader direct-export path prepared separately;
- obsolete timing cache metadata and its unused helper were removed after
timing ownership moved to the execution result.
Regression coverage after merging the latest `master` is `108 passed` across
the affected `tests/unit_tests/common`, `tests/unit_tests/charts`,
`tests/unit_tests/commands/chart`, `tests/unit_tests/models`,
`tests/unit_tests/tasks`, and `tests/unit_tests/utils` selections plus the
affected chart, model, and task integration tests. The complete
`superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts` and
`TableChart.test.tsx` suites are `111 passed`. The full frontend TypeScript
check passes, as do focused MyPy, Ruff, Pylint, Prettier, and Oxlint checks for
the changed files.
--
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]
