Conversation
Signed-off-by: eakmanrq <6326532+eakmanrq@users.noreply.github.com>
Signed-off-by: eakmanrq <6326532+eakmanrq@users.noreply.github.com>
596e55e to
60086e2
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes planning behavior for PR/dev plans involving seed changes by preventing stale prod interval-end overrides from being reused for new snapshot versions (which could otherwise cause missing intervals to be skipped and downstream physical tables not to be created).
Changes:
- Compute a single
plan_execution_timeand reuse it across default start/end and start-override calculations. - Refresh snapshot intervals and then filter out stale per-model end overrides for snapshots whose new versions have no intervals yet.
- Add a regression test reproducing the stale end-override scenario with
start="2 months ago"and a seed change.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
sqlmesh/core/context.py |
Adds plan_execution_time reuse and filters stale end_override_per_model entries after interval refresh. |
tests/core/test_context.py |
Adds a slow regression test covering stale prod interval-end overrides during a PR plan with a seed change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # This ensures that no models outside the impacted sub-DAG(s) will be backfilled unexpectedly. | ||
| backfill_models = modified_model_names or None | ||
|
|
||
| plan_execution_time = execution_time or now() |
There was a problem hiding this comment.
plan_execution_time = execution_time or now() will ignore valid falsy execution_time values (e.g., epoch millis 0) and silently replace them with now(). Use an explicit None check (e.g., execution_time if execution_time is not None else now()) to preserve caller-provided values.
| plan_execution_time = execution_time or now() | |
| plan_execution_time = execution_time if execution_time is not None else now() |
| @pytest.mark.slow | ||
| def test_seed_model_pr_plan_filters_stale_end_override( | ||
| copy_to_temp_path: t.Callable, mocker: MockerFixture | ||
| ): | ||
| path = copy_to_temp_path("examples/sushi") |
There was a problem hiding this comment.
PR description/test plan references test_seed_model_pr_plan_with_stale_prod_intervals, but the added regression test is named test_seed_model_pr_plan_filters_stale_end_override. Consider updating the PR description/test plan (or renaming the test) so the suggested -k selector matches.
Description
Prevent PR plans with seed changes from reusing stale prod interval ends for new snapshot versions. This falls back to execution time when a carried-over default end would be older than the requested start, and filters stale end overrides after refreshing snapshot intervals so downstream snapshots are still backfilled and their physical tables get created. It also adds a regression test covering a seed change with
start="2 months ago".Test Plan
pytest tests/core/test_context.py -k 'test_plan_seed_model_excluded_from_default_end or test_seed_model_pr_plan_with_stale_prod_intervals'\n\n## Checklist\n\n- [ ] I have runmake styleand fixed any issues\n- [x] I have added tests for my changes (if applicable)\n- [ ] All existing tests pass (make fast-test)\n- [x] My commits are signed off (git commit -s) per the DCO\n