Fix: Regression that caused view snapshos not to be migrated#5389
Fix: Regression that caused view snapshos not to be migrated#5389izeigerman merged 3 commits intomainfrom
Conversation
izeigerman
force-pushed
the
fix-view-migration-regression
branch
from
3e29bdb to
34fab79
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a regression where view snapshots were incorrectly skipped during the migration stage in forward-only deployments. The fix ensures that view snapshots requiring schema updates are properly included in the migration process.
- Updated snapshot migration logic to include view snapshots that need schema migration
- Modified the migration stage to properly handle view creation during schema migration
- Added comprehensive test coverage for forward-only view migration scenarios
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sqlmesh/core/snapshot/definition.py | Updated migration requirement check to include views (not just materialized models) |
| sqlmesh/core/snapshot/evaluator.py | Modified migration logic to handle all snapshots and create views when needed |
| sqlmesh/core/plan/stages.py | Enhanced stage building to include upstream dependencies for schema migrations |
| tests/core/test_snapshot_evaluator.py | Updated test to verify view creation during migration |
| tests/core/test_plan_stages.py | Adjusted test expectations for additional migration stage |
| tests/core/test_integration.py | Added integration test for forward-only view migration |
| tests/core/test_context.py | Updated test expectation for SQL changes |
| tests/fixtures/dbt/sushi_test/models/model_with_raw_code.sql | Added IF NOT EXISTS clause to fixture |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| snapshots_by_name.values(), | ||
| lambda s: self._migrate_snapshot( | ||
| s, | ||
| snapshots_by_name, | ||
| target_data_objects[s.snapshot_id], | ||
| target_data_objects.get(s.snapshot_id), |
There was a problem hiding this comment.
The migration logic now passes None for snapshots without target data objects, but the downstream code may not handle this case properly. Consider adding validation to ensure the migration logic can handle None target_data_object values.
There was a problem hiding this comment.
the downstream method indicates that it supports an optional target_data_object in its signature
| deployability_index=deployability_index, | ||
| create_render_kwargs=render_kwargs, | ||
| rendered_physical_properties=rendered_physical_properties, | ||
| dry_run=True, |
There was a problem hiding this comment.
The dry_run=True parameter suggests this is only a validation step, but for view migration to work properly, the view should actually be created. This may prevent view snapshots from being properly migrated.
| dry_run=True, | |
| dry_run=False, |
There was a problem hiding this comment.
this is just a bad comment all around
| stored_snapshots = self.state_reader.get_snapshots(plan.environment.snapshots) | ||
| snapshots = {**new_snapshots, **stored_snapshots} | ||
| snapshots_by_name = {s.name: s for s in snapshots.values()} | ||
| dag = snapshots_to_dag(snapshots.values()) |
There was a problem hiding this comment.
What is the reasoning for having this logic outside of EvaluatablePlan? For example should it have properties evaluatable_snapshots and dag which would return this info? Reading this over and I was surprised I couldn't consult the plan itself to get this information.
There was a problem hiding this comment.
Because EvaluatablePlan instance only contains data which can be (and will be) serialized. We could consider making it a property method of the EvaluatblePlan but currently there isn't really any other consumer for this. Plus we need access to the StateReader to fetch all snapshots which EvalutablePlan doesn't (and shouldn't) posses.
There was a problem hiding this comment.
Ah yes the state reader. Thanks.
sqlmesh/core/plan/stages.py
Outdated
| if snapshots[s_id].is_paused | ||
| and snapshots[s_id].is_model | ||
| and not snapshots[s_id].is_symbolic |
There was a problem hiding this comment.
Just to help me learn: Why couldn't these if conditions be applied when creating snapshot_ids_with_schema_migration?
There was a problem hiding this comment.
Oh I see it is part of requires_schema_migration_in_prod. This is making sure the returned snapshots in subdag also have these properties.
There was a problem hiding this comment.
correct. So the snapshots in the subdag may not return True for requires_schema_migration_in_prod and it's fine, but we still need to perform basic checks.
Previously, we were incorrectly skipping the migration stage for view snapshots.