Repository navigation
fix(examples): replace deprecated granularity_sqla with granularity - #43432
Conversation
Superset.common.query_object.DEPRECATED_FIELDS logs a warning and performs a runtime migration whenever granularity_sqla is present in form_data. The birth_names and world_bank example datasets still seeded slices using the old field name, triggering this warning on every load. get_slice_json() is a plain dict merge with no schema validation, so the rename is a straight drop-in with no behavior change once the examples are (re)loaded.
Self-review on the prior commit caught that the same slices also ship as YAML fixtures under examples/usa_births_names/charts and examples/world_health/charts, loaded unconditionally by load_examples_from_configs() on every `superset load_examples` run. Those still used the old granularity_sqla key, so the deprecation warning would keep firing via that path even after the .py rename. Mirrors the scope of the merged precedent (#43422), which covered both the .py and .yaml sides of the same pattern.
Code Review Agent Run #48770eActionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43432 +/- ##
==========================================
- Coverage 78.85% 78.85% -0.01%
==========================================
Files 2876 2876
Lines 164581 164581
Branches 38011 38011
==========================================
- Hits 129786 129784 -2
- Misses 32348 32350 +2
Partials 2447 2447
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| "compare_suffix": "o10Y", | ||
| "limit": "25", | ||
| "granularity_sqla": "ds", | ||
| "granularity": "ds", |
There was a problem hiding this comment.
Suggestion: The modern granularity key is not handled when rebuilding a big_number query from saved form data: form_data_query_context._columns_and_metrics only promotes granularity_sqla to the time column. Consequently, exports or fallback query paths for the Participants trendline produce an aggregate without the ds column and lose the trendline time series. Update the rebuild logic to recognize granularity as well, or retain the legacy key until that consumer is updated. [api mismatch]
Severity Level: Major ⚠️
- ❌ Birth names Big Number exports lose trendline time columns.
- ⚠️ Excel fallback queries return one aggregate instead of a series.
- ⚠️ Other modern-key Big Number fallback charts share this mismatch.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/examples/birth_names.py
**Line:** 179:179
**Comment:**
*Api Mismatch: The modern `granularity` key is not handled when rebuilding a `big_number` query from saved form data: `form_data_query_context._columns_and_metrics` only promotes `granularity_sqla` to the time column. Consequently, exports or fallback query paths for the Participants trendline produce an aggregate without the `ds` column and lose the trendline time series. Update the rebuild logic to recognize `granularity` as well, or retain the legacy key until that consumer is updated.
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 fix
|
The flagged issue is correct. The PR replaces To resolve this, you should update the query rebuilding logic to check for # Example of the required logic update
granularity = form_data.get("granularity") or form_data.get("granularity_sqla")There are no other comments in the provided PR review data to address. superset/examples/birth_names.py |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Review: fix(examples): replace deprecated granularity_sqla with granularity (head 271cdd45)
Reviewed as a behavior change, not a rename. Verdict on the one open codeant-ai thread at birth_names.py:179: CONFIRMED — but the blast radius is much narrower than "the charts lose their time column."
The codeant claim is structurally correct
superset/common/form_data_query_context.py:256-257 promotes the time column into columns for a Big Number trendline using only granularity_sqla:
if not columns and viz_type == "big_number" and form_data.get("granularity_sqla"):
return [form_data["granularity_sqla"]], metricsContrast :349, which correctly accepts both keys for the primary granularity job:
if granularity := form_data.get("granularity") or form_data.get("granularity_sqla"):Executable proof at head (RAN — build_query_context_from_form_data, viz_type big_number):
OLD granularity_sqla -> columns: ['ds'] | granularity: ds
NEW granularity -> columns: [] | granularity: ds
So after the rename, a big_number trendline no longer groups by its time column when its query context is rebuilt from form data — it returns a single aggregate instead of the trend series. This hits Participants in both birth_names.py and usa_births_names/charts/Participants.yaml (viz_type: big_number, granularity: ds).
…but normal chart rendering is NOT affected
build_query_context_from_form_data is called from only two places (traced, grep over superset/): the MCP chart tools (mcp_service/chart/tool/get_chart_preview.py, get_chart_sql.py) and Excel dashboard export (tasks/export_dashboard_excel.py). Standard /chart/data rendering does not use it — it relies on the stored query context / frontend buildQuery, where granularity is the canonical modern key (query_object.py:69 DeprecatedField(old_name="granularity_sqla", new_name="granularity") renames the legacy key, and extractExtras.ts reads granularity). The headline Big Number value is still correct; only the rebuilt-trendline groupby is dropped, and only in those two auxiliary paths.
Net: the rename direction is right (granularity is the officially deprecated-away key). The PR just exposes a pre-existing gap in _columns_and_metrics, which should mirror :349:
if not columns and viz_type == "big_number" and (
gran := form_data.get("granularity") or form_data.get("granularity_sqla")
):
return [gran], metricsWorth bundling here (one line) or filing as a fast follow, since this PR is what makes the modern-key big_number reach that branch.
Test adequacy (Rule 26) — the useful part
No test in this diff, and no existing test would catch this. tests/unit_tests/common/test_form_data_query_context.py covers the promotion (test_big_number_trendline_promotes_granularity_sqla_column, :142) but exclusively with the granularity_sqla key — there is no case exercising the modern granularity key for the big_number branch. I RAN the file at head: 40 passed — and they pass precisely because they never test the path this PR moves the examples onto. CI green here is evidence of absent coverage, not of correctness. Adding a granularity-key variant of that test (or fixing :256 and asserting it) would close the hole.
Supporting notes (non-blocking)
- Partial rename. Many example dashboards still carry
granularity_sqla(video_game_sales/*,fcc_new_coder_survey/*,sales_dashboard/*,slack_dashboard/*,deckgl_demo/*,misc_charts/*). This PR scopes to the birth-names and world-health/world-bank dashboards; each is internally consistent, and both keys render fine, so this is acceptable — but the repo now runs both keys in parallel, and (ironically) the un-migratedgranularity_sqlabig_numbers still promote correctly in the rebuild path while the migrated ones don't, until:256is fixed. - "Deprecated" is partly aspirational. Besides
:256,utils/core.py's native-filter path also reads the chart's own key exclusively (chart_has_granularity_sqla = bool(form_data.get("granularity_sqla")),:1183). Not exercised by simply loading examples, but it confirmsgranularity_sqlais not uniformly shimmed in code.
Gate status
CI re-derived at post time via statusCheckRollup: 63 CheckRun SUCCESS + 1 StatusContext SUCCESS, 3 NEUTRAL, 14 SKIPPED — no IN_PROGRESS, zero failures. This is a COMMENT round, not an approval or a change request. The codeant thread is a real (if narrow) finding; I'll leave it for a committer to resolve or action.
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
agent left non blocking comments i will leave to your call. approving as all else checks out
Summary
superset/common/query_object.py'sDEPRECATED_FIELDSlist warns and performs a runtime migration whenever legacy form_data keygranularity_sqlais present (current name:granularity). Thebirth_namesandworld_bankexample datasets — both the Python seed scripts and their YAML chart-config siblings — still constructed slice params using the old key, so everysuperset load_examplesrun (and every render of those charts) logged this deprecation warning.This mirrors the scope of #43422, which did the identical rename for the sibling deprecated field
timeseries_limit_metricacross the same.py+.yamlpairs.What changed
superset/examples/birth_names.py,superset/examples/world_bank.py: renamedgranularity_sqla→granularity(8 occurrences).superset/examples/usa_births_names/charts/*.yaml(11 files),superset/examples/world_health/charts/*.yaml(9 files): same rename, sinceload_examples_from_configs()loads these unconditionally alongside the Python loaders.Decisions made that were not in the instructions
None.
No-behavior-change note
get_slice_json()(superset/examples/helpers.py) is a plain dict merge with no schema validation, and the YAML configs are loaded as plain dicts too.granularityis the fieldDEPRECATED_FIELDSmigratesgranularity_sqlainto at read time, so this is a pure key rename — the resulting slice/chart params are identical once loaded, just constructed via the current field name instead of triggering the legacy-field warning + migration path.Test plan
ruff check/ruff format --checkon both changed.pyfiles: clean..yamlfiles parse successfully viayaml.safe_load.granularity_sqlaoccurrences in the changed files, and confirmed the unrelatedgranularity_sqla_overridevariable insuperset/utils/core.py(different dict,extra_form_data) was untouched.tests/for any assertion hardcodinggranularity_sqlaagainstbirth_names.create_slices/world_bank.create_slicesoutput specifically — none found.