Repository navigation
fix(examples): replace deprecated timeseries_limit_metric with series_limit_metric - #43422
Conversation
…es_limit_metric birth_names.py's bundled example slices were still constructing chart params with the deprecated timeseries_limit_metric field, which QueryObject silently renames to series_limit_metric with a deprecation warning on every request. Use the current field name directly - no behavior change, since QueryObject treats the two as equivalent.
… form_data fallback Self-review found two gaps in the birth_names.py rename: - The newer YAML-fixture twin of the same example (usa_births_names/charts/ Boys.yaml, Girls.yaml) still used timeseries_limit_metric, so it would keep firing the same deprecation warning on every load. Renamed both. - orderby_from_form_data() in form_data_query_context.py only ever checked the deprecated timeseries_limit_metric key when reconstructing a query context straight from a chart's persisted params (used by dashboard Excel export and the MCP chart tools). Charts saved with the current series_limit_metric field name - including the just-fixed birth_names examples - would silently lose their sort-by-metric through this path. Added a series_limit_metric-first fallback, matching the pattern already used in query_object.py and models/helpers.py.
Code Review Agent Run #5e9c60Actionable 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43422 +/- ##
==========================================
- 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:
|
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Reviewed the diff — the core fix is correct and well-tested (fail-before/pass-after tests confirm the orderby_from_form_data fallback, and the example renames are consistent across birth_names.py and the YAML fixtures). Approving, with two non-blocking notes for a possible follow-up:
superset/common/form_data_query_context.py:216— this is now a third independent reimplementation of thetimeseries_limit_metric→series_limit_metricfallback, alongsidequery_object.py'sDEPRECATED_FIELDS/_rename_deprecated_fields(old field wins if both present) andmodels/helpers.py'sseries_limit_metric or timeseries_limit_metric(new field wins). This new code also matchesmodels/helpers.py's precedence but is the opposite ofquery_object.py's, and it drops the deprecation-usagelogger.warningthatquery_object.pyemits. Worth a follow-up to consolidate into a shared helper so future deprecated-field renames don't need a 4th copy.tests/unit_tests/common/test_form_data_query_context.py:218-234— the pre-existingtest_orderby_uses_timeseries_limit_metric_and_order_descand the two new tests differ only in which key(s) populateform_data, all asserting the sameorderbyresult. Could be collapsed into one@pytest.mark.parametrizetest.
Neither blocks this PR — both are pre-existing patterns/observations rather than regressions introduced here.
Decisions made that were not in the instructions
None.
What the warning was
Production logs showed a recurring
WARNINGon every request that touched the bundledbirth_namesexample data:QueryObject._rename_deprecated_fields(superset/common/query_object.py) fires this warning whenever it sees the old field name in incoming kwargs, then silently copies the value ontoseries_limit_metric. The example data shipped with Superset itself was still using the old name, so every load of the example dashboard/charts triggered the warning for no reason.What changed
superset/examples/birth_names.py— renamed 4timeseries_limit_metric=call sites toseries_limit_metric=(the "Girls"/"Boys" table slices, oneecharts_timeseries_lineslice, and the "Names Sorted by Num in California" slice).superset/examples/usa_births_names/charts/{Boys,Girls}.yaml— same rename in the newer YAML-fixture version of the same example dashboard, which was hit by the same warning via a separateload_examples_from_configscode path.superset/common/form_data_query_context.py—orderby_from_form_data()only ever read the deprecatedtimeseries_limit_metrickey when reconstructing a query straight from a chart's persistedparams(used by dashboard Excel export and the MCP chart tools). That meant a chart saved with the currentseries_limit_metricfield — like the examples fixed here — would silently lose its sort-by-metric through that specific path. Added aseries_limit_metric-first fallback, matching the pattern already used inquery_object.pyandmodels/helpers.py.No behavior change
series_limit_metricandtimeseries_limit_metricare treated as fully equivalent everywhere in the normal query/chart-data path (QueryObject,get_sqla_query,ChartDataQueryObjectSchema) — the rename is a no-op there other than dropping the warning. Item 3 above is a genuine bug fix for the one path that previously checked only the old key, but restores intended behavior (existing sort-by-metric) rather than changing it.Test plan
superset/examples/usa_births_names/charts/{Boys,Girls}.yaml— validated as parseable YAML.tests/unit_tests/common/test_form_data_query_context.py— added two tests:series_limit_metricalone produces the sameorderbyas the deprecated field, andseries_limit_metrictakes precedence when both are present. Confirmed fail-before/pass-after against the pre-fix code (both new tests fail onmain, pass after theform_data_query_context.pychange).ruff check/ruff format --checkclean on all changed Python files.pytest tests/unit_tests/common/test_form_data_query_context.py→ 40 passed.