[SEPURI-SAI-KRISHNA]

SUMMARY

Fixes #43356.

ChartDataProphetOptionsSchema.time_grain validates against
get_time_grain_choices(), which merges builtin_time_grains with the
operator-configured TIME_GRAIN_ADDONS. prophet() then resolves the grain
through PROPHET_TIME_GRAIN_MAP, a static map that cannot contain an
operator's arbitrary addon key. So a configured custom grain passes schema
validation and then fails at the lookup, the API advertises a forecast it
cannot serve.

Reproducing on master with TIME_GRAIN_ADDONS = {"PT7M": "7 minute"}:

PT7M is a built-in mapped grain? False
schema.load('PT7M')  : ACCEPTED
prophet('PT7M')      : InvalidPostProcessingError -> Unsupported time grain: PT7M

The addon keys are arbitrary ISO-8601 durations, so the static map cannot
simply be extended to cover them, and converting them to pandas frequencies at
runtime is not total, P0.25Y being the obvious case. That leaves the other
option the issue identifies: publish only what the operation can resolve.

This adds get_prophet_time_grain_choices(), returning the keys of
PROPHET_TIME_GRAIN_MAP, and points the Prophet field's validator at it.
get_time_grain_choices() is deliberately left alone, it is shared with
time_grain_sqla on ChartDataExtrasSchema, where addon grains are
legitimately supported, and narrowing it there would be a regression.

This widens the published set as well as narrowing it. PT0.5H
(HALF_HOUR) and P0.25Y (QUARTER_YEAR) are in the map but not in
builtin_time_grains, so they were previously rejected by the schema despite
resolving fine in prophet(). They are now accepted. Net effect on a default
deployment is 19 advertised grains -> 21, with every one of them resolvable.

No working request changes behaviour: the grains this removes from the spec
never produced a forecast.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A, backend-only fix.

TESTING INSTRUCTIONS

pytest tests/unit_tests/charts/test_schemas.py -q
pytest tests/unit_tests/pandas_postprocessing/test_prophet.py -q

Three tests are added:

• test_prophet_schema_advertises_only_resolvable_grains asserts the field's
  own OneOf choices equal the map's keys. It reads the validator rather than
  the helper, so re-pointing the field back at get_time_grain_choices() fails
  here rather than silently reintroducing the bug.
• test_prophet_rejects_addon_grain_accepted_by_time_grain_sqla configures
  TIME_GRAIN_ADDONS = {"PT7M": "7 minute"} and asserts the addon is still
  advertised by the shared helper while the Prophet schema rejects it. This is
  the reported bug, and it pins the "do not narrow the shared helper"
  constraint.
• test_prophet_accepts_every_mapped_grain covers the five grains that now
  load and resolve, including the two newly advertised aliases.

The three pre-existing time_grain assertions are unchanged and still pass —
P1D remains valid and invalid_grain remains rejected.

ADDITIONAL INFORMATION

• Has associated issue: Custom TIME_GRAIN_ADDONS grains pass chart-data validation but fail in prophet() #43356
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

Note for reviewers: #43206 also edits ChartDataProphetOptionsSchema, about
fifty lines below this change (monthly_seasonality -> daily_seasonality,
plus index). Separate hunks, so the two should merge cleanly, but whichever
lands second may want a rebase.

[bito-code-review]

Code Review Agent Run #a77d73

Actionable Suggestions - 0

Additional Suggestions - 1

• tests/unit_tests/charts/test_schemas.py - 1

  • Misleading test name · Line 584-587
    The name/docstring claim `time_grain_sqla` accepts the addon, but its `OneOf` validator (schemas.py:1154) is evaluated at import time and frozen, so a runtime `TIME_GRAIN_ADDONS` change is not reflected in the field. The test only verifies the helper `get_time_grain_choices()`, not the field. Rename to reflect that the shared helper (not the field) includes the addon.

Review Details

• Files reviewed - 2 · Commit Range: 29939f4..29939f4

  • superset/charts/schemas.py
  • tests/unit_tests/charts/test_schemas.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

• 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	f0bb1a2
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6aa0e996e77ddc0008c43161
😎 Deploy Preview	https://deploy-preview-43710--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 79.90%. Comparing base (c503f92) to head (f0bb1a2).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #43710   +/-   ##
=======================================
  Coverage   79.90%   79.90%           
=======================================
  Files        2916     2916           
  Lines      170738   170742    +4     
  Branches    39556    39557    +1     
=======================================
+ Hits       136435   136439    +4     
  Misses      31785    31785           
  Partials     2518     2518           

Flag	Coverage Δ
hive	37.61% <100.00%> (+<0.01%)	⬆️
mysql	57.39% <100.00%> (+<0.01%)	⬆️
postgres	57.42% <100.00%> (+<0.01%)	⬆️
presto	39.52% <100.00%> (+<0.01%)	⬆️
python	84.34% <100.00%> (+<0.01%)	⬆️
sqlite	57.11% <100.00%> (+<0.01%)	⬆️
unit	75.45% <100.00%> (+0.06%)	⬆️

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.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[rusackas]

Thanks @SEPURI-SAI-KRISHNA, a Prophet-only choices source instead of narrowing the shared helper is the right shape, and picking up PT0.5H/P0.25Y along the way is a nice bonus. It's gone conflicting since, though, just the tail of test_schemas.py where master grew another test. Quick rebase and I think we're good; holler if you want a hand with it.

[SEPURI-SAI-KRISHNA]

Rebased on master, thanks @rusackas. The conflict was where you said, the tail of test_schemas.py: master had grown test_post_processing_operation_schema_rejects_string_helpers in the same slot as the prophet tests. Both sides were pure additions, so both are kept. schemas.py and the import blocks auto-merged.

One extra file beyond the rebase. check-openapi-spec-drift merged in #43841 a few hours after your comment, and it regenerates the spec on every PR, so this change now has to carry the regenerated docs/static/resources/openapi.json. The delta is exactly the two grains this PR adds:

               "PT30M",
+              "PT0.5H",
               "PT1H",
...
               "P3M",
+              "P0.25Y",
               "P1Y",

Nothing else in the spec moves, and the change is scoped to ChartDataProphetOptionsSchema.time_grain. time_grain_sqla still publishes the built-ins plus TIME_GRAIN_ADDONS, unchanged. The published enum is now key-for-key identical to PROPHET_TIME_GRAIN_MAP, in map order, which is the self-maintaining property the fix is aiming for: adding a grain to the map advertises it, with no second list to keep in sync.

Verified locally: the 46 tests in test_schemas.py pass, and pre-commit is clean (ruff, ruff-format, pylint, mypy). I also sanity-checked that the guard actually holds by pointing the field back at get_time_grain_choices(), which fails
test_prophet_schema_advertises_only_resolvable_grains and both new-grain cases, so a future revert to the shared helper won't pass silently.

CI is green on the rebase, check-openapi-spec-drift included, so the committed spec is what the workflow itself regenerates on the pinned apispec version and not an artifact of my local environment.

[bito-code-review]

Code Review Agent Run #df466b

Actionable Suggestions - 0

Additional Suggestions - 1

• superset/charts/schemas.py - 1

  • Inaccurate docstring claim · Line 80-80
    The docstring says the choices are "Deliberately narrower than get_time_grain_choices()", but PROPHET_TIME_GRAIN_MAP also includes HALF_HOUR and QUARTER_YEAR, which are absent from builtin_time_grains. The prophet set is not a strict subset, so the claim is inaccurate. Consider describing the real difference (excludes TIME_GRAIN_ADDONS keys) instead.

Review Details

• Files reviewed - 2 · Commit Range: b9c3dd2..b9c3dd2

  • superset/charts/schemas.py
  • tests/unit_tests/charts/test_schemas.py
• Files skipped - 1

  • docs/static/resources/openapi.json - Reason: Filter setting
• 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

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

AI Code Review powered by

[bito-code-review]

Code Review Agent Run #d8900a

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: 44cc584..44cc584

  • superset/charts/schemas.py
  • tests/unit_tests/charts/test_schemas.py
• Files skipped - 1

  • docs/static/resources/openapi.json - Reason: Filter setting
• 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

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

AI Code Review powered by

[rusackas]

Thanks for the rebase @SEPURI-SAI-KRISHNA, and the openapi diff writeup is appreciated. LGTM, approving!

[rusackas]

Rebased on master (conflict was just where #43585 landed nearby in schemas.py, kept both). CI's check-openapi-spec-drift also caught an unrelated stale enum in time_grain_sqla left over from #43585 switching to a custom validator function apispec can't introspect - regenerated that too so the spec matches what's actually in the code now.

[SEPURI-SAI-KRISHNA]

Thanks for the rebase and for catching the second drift, the ChartDataExtrasSchema
enum needed to go for exactly the reason you gave, and I would not have spotted it from
my side of the conflict.

Worth restating what this PR still does, because #43585 landed the same core narrowing
while this was open and the title now overstates the change.

Already fixed by #43585. The Prophet field validating against
PROPHET_TIME_GRAIN_MAP rather than get_time_grain_choices() is master's behaviour.
My schemas.py change is reduced to routing that same set through a named
get_prophet_time_grain_choices() helper, readability, not behaviour. If you would
rather not carry the extra indirection, dropping that helper and keeping the tests
would be a reasonable trim; I have no attachment to it.

Still outstanding before this PR. #43585 changed the validator but did not
regenerate the spec, so master publishes a Prophet time_grain enum that is narrower
than what the code accepts:

	Prophet time_grain enum
master's openapi.json	19 entries
master's PROPHET_TIME_GRAIN_MAP	21 keys
this branch	21 entries

The two missing from the published spec are PT0.5H and P0.25Y. Both load fine
through the schema on master; they are simply undocumented. That is the +2 in
openapi.json here, and together with your extras resync it puts the spec back in
agreement with the code.

Test coverage. Three tests, none of which have an equivalent in master:

• test_prophet_schema_advertises_only_resolvable_grains, asserts the advertised set
  equals the set prophet() can map, so the spec and the operation cannot drift apart
  again silently. This is the one that would have caught the gap above.
• test_prophet_accepts_every_mapped_grain, parametrised over the map, so a grain
  added to PROPHET_TIME_GRAIN_MAP without a schema update fails here.
• test_prophet_choices_exclude_an_addon_the_shared_helper_includes, pins the
  intended asymmetry: a TIME_GRAIN_ADDONS key stays valid for the shared helper and
  stays out of Prophet's set.

So: a spec resync, a regression guard against the drift that just happened, and an
optional helper. Happy to retitle it along those lines if you would prefer the history
to read accurately, or to trim it to just the spec and tests, your call.

[bito-code-review]

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it