Repository navigation
fix(query-context): resolve legacy charts' temporal column from granularity_sqla - #44526
trakshan-mishra wants to merge 3 commits into
Conversation
Code Review Agent Run #134f50Actionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44526 +/- ##
==========================================
+ Coverage 81.07% 81.16% +0.09%
==========================================
Files 2955 2961 +6
Lines 178331 178932 +601
Branches 41313 41402 +89
==========================================
+ Hits 144580 145234 +654
+ Misses 31041 30980 -61
- Partials 2710 2718 +8
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:
|
…larity_sqla A query_context stored by an older version carries no granularity of its own and no x-axis at all; its temporal column survives only in form_data as granularity_sqla. Explore rebuilds the query from form_data on every render and so never notices, but every other consumer replays the stored query verbatim and fails the `not granularity and is_timeseries` check in models/helpers, returning a 500. _apply_granularity only infers a granularity when should_infer_filter_ granularity passes, and that is gated on is_adhoc_column(x_axis), so a legacy chart never reaches the inference at all. Recover 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, falling back to the dataset's main_dttm_col, and accept it only if it is a known temporal column. The absent x-axis is what distinguishes this shape from a modern chart, so it is tested first and charts that already carry a granularity are left untouched.
4fb3325 to
fa0814a
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if ( | ||
| not x_axis | ||
| and query_object.granularity is None | ||
| and query_object.is_timeseries | ||
| ): | ||
| legacy_granularity = (form_data or {}).get("granularity_sqla") or getattr( | ||
| datasource, "main_dttm_col", None | ||
| ) | ||
| if legacy_granularity in temporal_columns: | ||
| query_object.granularity = legacy_granularity |
There was a problem hiding this comment.
Suggestion: When a legacy query has a temporal filter but no time_range, this assignment activates later filter removal without adding a replacement, silently dropping the only temporal restriction.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Logic error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/query_context_factory.py
**Line:** 288:297
**Comment:**
*Logic Error: When a legacy query has a temporal filter but no `time_range`, this assignment activates later filter removal without adding a replacement, silently dropping the only temporal restriction.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
I checked this against the full query path, and the restriction isn't dropped.
When a query has no time_range, QueryObjectFactory._process_time_range takes the range from the query's TEMPORAL_RANGE filter, so from_dttm/to_dttm already carry that filter's bounds before _apply_granularity runs. Once granularity is set, models/helpers.py builds the time filter on the granularity column from those same bounds. So the filter removed here is replaced, not lost. This is the same "replace the default temporal filter" behaviour that queries with an explicit granularity already go through.
To pin it, b16ee7f adds test_apply_granularity_legacy_query_context_keeps_temporal_filter_bounds. It builds the query object with the real QueryObjectFactory (no time_range, a TEMPORAL_RANGE filter of 2024-01-01 : 2024-02-01), runs _apply_granularity with a legacy granularity_sqla, and asserts that the filter is removed while from_dttm/to_dttm keep those bounds and granularity resolves to ds.
|
The flagged issue is correct. The logic added in To resolve this, you should ensure that the filter removal logic only proceeds if the temporal filter is redundant or if a valid replacement is guaranteed. I recommend adding a check to verify if a There are no other comments on this PR to address. superset/common/query_context_factory.py |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #37a027
Actionable Suggestions - 1
-
superset/common/query_context_factory.py - 1
- unhashable dict crash · Line 293-297
Additional Suggestions - 1
-
tests/unit_tests/common/test_query_context_factory.py - 1
-
Duplicated test setup · Line 749-757The three new tests repeat an identical 8-line `Mock(spec=QueryObject)` setup (lines 749-757, 778-786, 799-807). Extracting a shared helper would remove the duplication and make the per-test intent (granularity_sqla precedence vs main_dttm_col fallback vs explicit granularity) clearer.
-
Review Details
-
Files reviewed - 2 · Commit Range:
fa0814a..fa0814a- superset/common/query_context_factory.py
- tests/unit_tests/common/test_query_context_factory.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| legacy_granularity = (form_data or {}).get("granularity_sqla") or getattr( | ||
| datasource, "main_dttm_col", None | ||
| ) | ||
| if legacy_granularity in temporal_columns: | ||
| query_object.granularity = legacy_granularity |
There was a problem hiding this comment.
granularity_sqla can be an adhoc dict (see migrations/shared/migrate_viz/base.py:135), so legacy_granularity in temporal_columns (a set of str) raises TypeError: unhashable type: 'dict' on legacy charts replayed through this path. Guard with isinstance(legacy_granularity, str) before the membership test.
Code Review Run #37a027
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Fixed in 6216ac9. An adhoc granularity_sqla is now reduced to its sqlExpression before the membership test against temporal_columns, so it resolves instead of raising "unhashable type: 'dict'". Covered by test_apply_granularity_legacy_adhoc_granularity_sqla.
There was a problem hiding this comment.
The suggestion to guard the membership test with isinstance(legacy_granularity, str) is appropriate. It prevents a TypeError when granularity_sqla is an adhoc dictionary, ensuring the code safely handles legacy chart configurations.
superset/common/query_context_factory.py
legacy_granularity = (form_data or {}).get("granularity_sqla") or getattr(
datasource, "main_dttm_col", None
)
if isinstance(legacy_granularity, str) and legacy_granularity in temporal_columns:
query_object.granularity = legacy_granularity
temporal_columns is a set, so testing membership with the raw granularity_sqla value raises "unhashable type: 'dict'" when a stored form_data carries an adhoc column there. The chart API types the field as a string, but saved form_data is not re-validated on read, and the adjacent x_axis handling already unwraps dicts before its own membership test for the same reason. Unwrap sqlExpression first. A dict carrying neither key yields None, which is hashable and matches no temporal column, so the legacy fallback is simply skipped.
Code Review Agent Run #496fd3Actionable 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 |
…recovery With no time_range, QueryObjectFactory derives from_dttm and to_dttm from the query's TEMPORAL_RANGE filter. Recovering the legacy granularity makes _apply_granularity remove that filter, and models.helpers rebuilds the time filter on the granularity from those bounds. Build the query object with the real factory and assert the bounds survive the removal, so the restriction is replaced rather than dropped.
Code Review Agent Run #f23dd2Actionable 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 |
1 participant
SUMMARY
Fixes one symptom of #42926: a chart whose
query_contextwas stored by an older version 500s on every non-Explore path.Those stored payloads carry no
granularityof their own and no x-axis at all — the temporal column survives only inform_dataasgranularity_sqla. Explore rebuilds the query fromform_dataon 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 thenot granularity and is_timeseriescheck inmodels/helpers.py, and raises._apply_granularityalready has inference for a missing granularity, but it only runs whenshould_infer_filter_granularitypasses, and that is gated onis_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, andis_timeseriesis set, takegranularity_sqlafromform_data, fall back to the dataset'smain_dttm_col, and accept the result only if it is a known temporal column.Two deliberate choices:
form_datais preferred overmain_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
granularityare 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:They cover the three branches:
granularity_sqlainform_datais used, and takes precedence over a differentmain_dttm_col— so the test asserts precedence rather than merely that some temporal column was picked.granularity_sqla, the dataset'smain_dttm_colis used.granularityis not overwritten.To reproduce manually: take a chart saved by an older version whose
query_contexthas"granularity": nulland nox_axis, withgranularity_sqlaset inparams, 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
This addresses the symptom I described in my comment on #42926. 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.