Repository navigation
fix(query-context): resolve legacy granularity_sqla for stored queries - #44460
Conversation
Code Review Agent Run #5a3714Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The current implementation of superset/common/query_context_factory.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44460 +/- ##
==========================================
- Coverage 81.10% 81.10% -0.01%
==========================================
Files 2955 2955
Lines 178385 178389 +4
Branches 41336 41337 +1
==========================================
+ Hits 144680 144682 +2
- Misses 30996 30998 +2
Partials 2709 2709
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Nice diagnosis on this one. The granularity_sqla fallback makes sense, and the temporal_columns guard keeps it from picking up a stale column. Codeant's filter-removal concern looks addressed in cafc5bb too. LGTM! We'll get this merged.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The Translations failure is reproducible on the CI base. I ran The Codecov report currently has no unit-test upload; Python-Unit is still awaiting workflow approval. Locally, all 52 query-context tests pass with coverage and execute all four added statements, including the early return. No translation changes added to this PR. |
Code Review Agent Run #52b24dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Heads up on the red CI here — the
None of those strings appear in this diff (this change is confined to Happy to regenerate it here if you'd rather unblock this PR that way, but it would add unrelated translation churn to the diff, so I've left it alone. Let me know which you prefer. |
650a43a to
6cd1291
Compare
|
Heads up on the red The OAuth2 reword landed in This branch touches no translatable strings, so I have left |
Code Review Agent Run #7f5a76Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Addressed the precedence finding in e967242: |
Code Review Agent Run #6934b3Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
i18n
A chart saved before the x-axis control existed carries its time column in form_data as the legacy granularity_sqla key, and its stored query_context has is_timeseries true with granularity null. _apply_granularity only infers a granularity when form_data has an adhoc x_axis plus a temporal range filter, so that shape falls through with granularity still unset and lands on the "not granularity and is_timeseries" guard in superset/models/helpers.py, which raises "Datetime column not provided as part table configuration and is required by this type of chart". The chart renders in Explore because the paths that rebuild a query from form data already resolve the legacy key (extractExtras.ts for Explore, form_data_query_context.py for the Excel export and the MCP tools). Every consumer that replays the stored query_context verbatim through /api/v1/chart/<id>/data/ fails instead, which covers alerts and reports, thumbnails, cache warm-up and data export. When a stored query object is a timeseries, has no granularity and carries no x-axis, fall back to form_data's granularity_sqla and only then to the dataset's main datetime column. Candidates are matched against the dataset's temporal columns, so a column that has since been dropped or is no longer temporal is ignored. Queries that already carry a granularity, non timeseries queries and x-axis queries are untouched. Fixes apache#42926
9c95ec7 to
717be3b
Compare
github-actions
Bot
removed
i18n
Code Review Agent Run #4a5ce9Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
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
paramsas the legacygranularity_sqla, and its storedquery_contexthasis_timeseries: truewithgranularity: null:{"is_timeseries": true, "columns": ["ds"], "metrics": ["count"], "filters": [], "granularity": null}QueryContextFactory._apply_granularity(superset/common/query_context_factory.py:245-276on master) only infers a granularity whenform_datacarries an adhocx_axisplus a temporal range bound. With nox_axis,is_adhoc_column(None)is false,should_infer_filter_granularityis false, theif granularity := query_object.granularity:block at line 278 is skipped because the granularity isNone, and the method returns having set nothing.superset/models/helpers.py:4715then hitsand the request fails with a 400 out of
ChartDataQueryFailedError. The backward-compatibility coercion just above it (helpers.py:4704) is explicitly gated ongranularity is not None, so a null granularity is not rescued there, and thegranularity_sqlatogranularityrename atquery_object.py:69is query-object level only and never readsform_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-80for Explore, andform_data_query_context.py:349for the Excel export and the MCP tools. Only the consumers that replay the storedquery_contextverbatim throughGET /api/v1/chart/<id>/data/fail, which is alerts and reports, thumbnails, cache warm-up and data export.When the query object is a timeseries with no granularity, check
form_data'sgranularity, then legacygranularity_sqla, then the dataset's main datetime column. This matches the precedence used by Explore and the form-data query builder. Returning after inference also preserves independent temporal filters.Two deliberate narrowings, both to avoid disturbing existing behavior:
temporal_columnsset, so a column that oldparamsstill names but the dataset has since dropped or made non temporal is ignored rather than injected into the query.form_datacarries anx_axis. Setting a granularity there would fall into the block below and rewrite the x-axis column to the granularity, which is exactly whattest_apply_granularity_preserves_physical_temporal_axisguards against. Anx_axisquery is also not the reported shape:is_timeseriesdefaults toDTTM_ALIAS in columns(query_object.py:202), which is the pre-x-axis form. A stored query that somehow has both an explicitis_timeseries: trueand anx_axisstill 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
Current validation:
pytest tests/unit_tests/common -qpasses 343 tests. A five-case regression covers distinct valid modern/legacy columns and missing, empty, dropped, or non-temporal modern values; the distinct-column case fails on the previous head and passes with the fix. All applicable staged-file pre-commit hooks pass, including mypy, Ruff and Pylint.Original fallback validation, retained for context:
Six unit tests were added to
tests/unit_tests/common/test_query_context_factory.py: thegranularity_sqlafallback, themain_dttm_colfallback, 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.Red, with the three new fallback tests against unmodified
query_context_factory.py:Green, same file with the fix applied (45 pre-existing plus the 6 new):
The rest of the directory is unaffected:
tests/unit_tests/chartsandtests/unit_tests/modelswere also run: 1154 passed with this branch against 1148 on master, with the same 20 failures inmodels/core_test.pyandmodels/test_hours_offset_bound_truncation.pybefore and after, so they are pre-existing in my environment and unrelated.pre-commit runon the two staged files passes, including mypy (main), ruff, ruff-format and pylint.Manual check: save a chart whose
query_contexthasis_timeseries: trueand nogranularitywhile itsparamscarry onlygranularity_sqla, then callGET /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.txtthenpip 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_granularitycases. Happy to add an integration test that stores a legacy-shapedquery_contextand asserts a 200 from the data endpoint if a reviewer would rather see it pinned there too.ADDITIONAL INFORMATION