trakshan-mishra opened a new pull request, #44526: URL: https://github.com/apache/superset/pull/44526
### SUMMARY Fixes one symptom of #42926: a chart whose `query_context` was stored by an older version 500s on every non-Explore path. Those stored payloads carry no `granularity` of their own and no x-axis at all — the temporal column survives only in `form_data` as `granularity_sqla`. Explore rebuilds the query from `form_data` on every render and so never notices. Every other consumer (alerts & reports, the chart data API, dashboards rendering from the stored context) replays the stored query verbatim, reaches the `not granularity and is_timeseries` check in `models/helpers.py`, and raises. `_apply_granularity` already has inference for a missing granularity, but it only runs when `should_infer_filter_granularity` passes, and that is gated on `is_adhoc_column(x_axis)`. A legacy chart has no x-axis at all, so it never reaches the inference. This recovers the column the way Explore effectively does: when there is no x-axis, no `granularity`, and `is_timeseries` is set, take `granularity_sqla` from `form_data`, fall back to the dataset's `main_dttm_col`, and accept the result only if it is a known temporal column. Two deliberate choices: - The **absent x-axis** is what distinguishes this legacy shape from a modern chart, so it is tested first. A modern chart always has one. - `form_data` is preferred over `main_dttm_col`, because what the chart saved should win over the dataset default. A chart explicitly built on a non-default time column keeps that column. Charts that already carry a `granularity` are left untouched, so this is inert for everything except the broken shape. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF Not applicable — backend only, no UI change. ### TESTING INSTRUCTIONS Three unit tests are included in `tests/unit_tests/common/test_query_context_factory.py`: ``` pytest tests/unit_tests/common/test_query_context_factory.py -q ``` They cover the three branches: 1. `granularity_sqla` in `form_data` is used, and takes precedence over a *different* `main_dttm_col` — so the test asserts precedence rather than merely that some temporal column was picked. 2. With no `granularity_sqla`, the dataset's `main_dttm_col` is used. 3. A chart that already has a `granularity` is not overwritten. To reproduce manually: take a chart saved by an older version whose `query_context` has `"granularity": null` and no `x_axis`, with `granularity_sqla` set in `params`, then request it through the chart data API or attach it to an alert. Before this change it returns a 500; after, it renders. ### ADDITIONAL INFORMATION - [x] Has associated issue: #42926 - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API This addresses the symptom I described in [my comment on #42926](https://github.com/apache/superset/issues/42926#issuecomment-5708931274). It fixes the read path at query-build time rather than migrating stored payloads, so it needs no migration and helps charts that are never re-saved. @AryaKetanShCt had mentioned a separate symptom on that issue; this does not overlap with it. -- 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]
