shoemoney opened a new pull request, #44460:
URL: https://github.com/apache/superset/pull/44460
### SUMMARY
Fixes #42926 (the second symptom, the one left open after #42927).
A chart saved before the x-axis control existed keeps its time column in
`params` as the legacy `granularity_sqla`, and its stored `query_context` has
`is_timeseries: true` with `granularity: null`:
```json
{"is_timeseries": true, "columns": ["ds"], "metrics": ["count"], "filters":
[], "granularity": null}
```
`QueryContextFactory._apply_granularity`
(`superset/common/query_context_factory.py:245-276` on master) only infers a
granularity when `form_data` carries an adhoc `x_axis` plus a temporal range
bound. With no `x_axis`, `is_adhoc_column(None)` is false,
`should_infer_filter_granularity` is false, the `if granularity :=
query_object.granularity:` block at line 278 is skipped because the granularity
is `None`, and the method returns having set nothing.
`superset/models/helpers.py:4715` then hits
```python
if not granularity and is_timeseries:
raise QueryObjectValidationError(
_("Datetime column not provided as part table configuration "
"and is required by this type of chart")
)
```
and the request fails with a 400 out of `ChartDataQueryFailedError`. The
backward-compatibility coercion just above it (`helpers.py:4704`) is explicitly
gated on `granularity is not None`, so a null granularity is not rescued there,
and the `granularity_sqla` to `granularity` rename at `query_object.py:69` is
query-object level only and never reads `form_data`.
The chart still renders in Explore, because the paths that rebuild a query
from form data already resolve the legacy key: `extractExtras.ts:77-80` for
Explore, and `form_data_query_context.py:349` for the Excel export and the MCP
tools. Only the consumers that replay the stored `query_context` verbatim
through `GET /api/v1/chart/<id>/data/` fail, which is alerts and reports,
thumbnails, cache warm-up and data export.
This is the fix @rusackas described on the issue: when the query object is a
timeseries with no granularity, fall back to `form_data`'s `granularity_sqla`,
and only then to the dataset's main datetime column.
Two deliberate narrowings, both to avoid disturbing existing behavior:
- Candidates are matched against the already computed `temporal_columns`
set, so a column that old `params` still names but the dataset has since
dropped or made non temporal is ignored rather than injected into the query.
- The fallback is skipped when `form_data` carries an `x_axis`. Setting a
granularity there would fall into the block below and rewrite the x-axis column
to the granularity, which is exactly what
`test_apply_granularity_preserves_physical_temporal_axis` guards against. An
`x_axis` query is also not the reported shape: `is_timeseries` defaults to
`DTTM_ALIAS in columns` (`query_object.py:202`), which is the pre-x-axis form.
A stored query that somehow has both an explicit `is_timeseries: true` and an
`x_axis` still reaches the raise, and is left for a separate change rather than
risking the column rewrite here.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable, backend only.
### TESTING INSTRUCTIONS
Six unit tests were added to
`tests/unit_tests/common/test_query_context_factory.py`: the `granularity_sqla`
fallback, the `main_dttm_col` fallback, a stale legacy column being ignored, an
explicit granularity not being overridden, a non timeseries query being left
alone, and an x-axis query being left alone.
```
pytest tests/unit_tests/common/test_query_context_factory.py
```
Red, with the three new fallback tests against unmodified
`query_context_factory.py`:
```
FFF...
[100%]
> assert query_object.granularity == "ds"
E AssertionError: assert None == 'ds'
E + where None = <Mock spec='QueryObject'
id='4709837200'>.granularity
tests/unit_tests/common/test_query_context_factory.py:567: AssertionError
...
FAILED
tests/unit_tests/common/test_query_context_factory.py::TestQueryContextFactory::test_apply_granularity_falls_back_to_legacy_granularity_sqla
FAILED
tests/unit_tests/common/test_query_context_factory.py::TestQueryContextFactory::test_apply_granularity_falls_back_to_main_dttm_col
FAILED
tests/unit_tests/common/test_query_context_factory.py::TestQueryContextFactory::test_apply_granularity_ignores_stale_legacy_granularity_sqla
3 failed, 3 passed, 45 deselected in 0.55s
```
Green, same file with the fix applied (45 pre-existing plus the 6 new):
```
...................................................
[100%]
51 passed in 0.37s
```
The rest of the directory is unaffected:
```
$ pytest tests/unit_tests/common -q
337 passed in 6.90s
```
`tests/unit_tests/charts` and `tests/unit_tests/models` were also run: 1154
passed with this branch against 1148 on master, with the same 20 failures in
`models/core_test.py` and `models/test_hours_offset_bound_truncation.py` before
and after, so they are pre-existing in my environment and unrelated.
`pre-commit run` on the two staged files passes, including mypy (main),
ruff, ruff-format and pylint.
Manual check: save a chart whose `query_context` has `is_timeseries: true`
and no `granularity` while its `params` carry only `granularity_sqla`, then
call `GET /api/v1/chart/<pk>/data/`. It returns the data instead of "Datetime
column not provided as part table configuration".
Environment used: Python 3.11 venv, `pip install -r requirements/base.txt`
then `pip install -e . --no-deps`, plus pytest. No end-to-end API test is
included in this PR, since the change is confined to one method and the unit
file already owns the `_apply_granularity` cases. Happy to add an integration
test that stores a legacy-shaped `query_context` and asserts a 200 from the
data endpoint if a reviewer would rather see it pinned there too.
### 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 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]
