fix(schemas): rename deprecated query fields regardless of falsy values

[eschutho]

What's the change?

ChartDataQueryObjectSchema.rename_deprecated_fields() is a Marshmallow @post_load hook that renames legacy query field names to their modern equivalents:

Old (deprecated)	New
timeseries_limit	series_limit
timeseries_limit_metric	series_limit_metric
granularity_sqla	granularity
groupby	columns

The original implementation used the walrus operator with a truthiness check:

if value := data.pop(old, None):
    data[new] = value

Bug: data.pop(old, None) always removes the old key, but when the value is falsy, the new key is never set. Real-world cases silently affected:

• timeseries_limit=0 — the default for most charts meaning "no series limit"
• groupby=[] — explicit empty groupby (fields.List(...), empty list is falsy)
• timeseries_limit_metric=None or granularity_sqla=None (all four deprecated fields carry allow_none=True)

Result: the renamed field (series_limit) is silently absent from the data dict passed to QueryObject. QueryObject.__init__ falls back to its series_limit: int = 0 default — numerically the same but semantically wrong; the caller's explicit 0 or None is discarded.

Fix:

for old, new in _renames:
    if old in data:
        data.setdefault(new, data.pop(old))

• old in data — checks key presence, not value truthiness
• data.pop(old) — always removes the deprecated key
• data.setdefault(new, ...) — sets the new key only if it isn't already present (an explicit series_limit from the caller is never silently clobbered by a coexisting deprecated timeseries_limit)

No behavior change for truthy values

For any truthy value the behavior is identical to the old code.

Test plan

• Unit tests for ChartDataQueryObjectSchema: verify that timeseries_limit=0 is renamed to series_limit=0 after deserialization (was silently discarded before).
• Integration test: run the existing query_context_tests.py tests that set deprecated fields to ensure they still pass.

Follow-up (non-blocking)

QueryObject._rename_deprecated_fields (query_object.py:232) has the same if value: guard for the setattr path. The deprecation warning still fires there regardless (the log line precedes the if), and the functional outcome is identical since series_limit defaults to 0. A follow-up fix there would make both rename paths consistent.

[bito-code-review]

Code Review Agent Run #9098f1

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: 0be66d4..0be66d4

  • superset/charts/schemas.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - 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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	0be66d4
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a36be27aaa634000844286d
😎 Deploy Preview	https://deploy-preview-41263--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.66%. Comparing base (382a094) to head (0be66d4).
⚠️ Report is 8 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (382a094) and HEAD (0be66d4). Click for more details.

HEAD has 36 uploads less than BASE

Flag	BASE (382a094)	HEAD (0be66d4)
hive	5	1
python	30	2
presto	5	1

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #41263      +/-   ##
==========================================
- Coverage   64.36%   55.66%   -8.70%     
==========================================
  Files        2653     2653              
  Lines      144864   144867       +3     
  Branches    33421    33421              
==========================================
- Hits        93242    80647   -12595     
- Misses      49949    63486   +13537     
+ Partials     1673      734     -939     

Flag	Coverage Δ
hive	39.33% <0.00%> (+<0.01%)	⬆️
mysql	?
postgres	?
presto	40.92% <100.00%> (+<0.01%)	⬆️
python	40.99% <100.00%> (-18.58%)	⬇️
sqlite	?
unit	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[durgaprasadml]

Thanks for the fix. The previous implementation relied on truthiness checks and could unintentionally drop valid falsy values such as 0, [], or None during field renaming. Checking key presence instead of value truthiness looks correct and preserves the intended behavior.

One thing that may be worth adding is a test covering the case where both the deprecated and modern field names are supplied simultaneously (e.g. timeseries_limit and series_limit) to explicitly document the expected precedence behavior.