From 59302da1e78d99c73fb1d352f99717322b11740e Mon Sep 17 00:00:00 2001 From: Tomasz Zorawik <67728999+xardasos@users.noreply.github.com> Date: Mon, 15 Sep 2025 16:03:48 +0200 Subject: [PATCH 1/9] add test --- tests/core/test_plan.py | 68 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/tests/core/test_plan.py b/tests/core/test_plan.py index c9c19376d9..07c64919c0 100644 --- a/tests/core/test_plan.py +++ b/tests/core/test_plan.py @@ -4131,3 +4131,71 @@ def test_plan_ignore_cron_flag(make_snapshot): ], ) ] + + +def test_indirect_change_to_materialized_view_is_breaking(make_snapshot): + snapshot_a_old = make_snapshot( + SqlModel(name="a", + query=parse_one("select 1 as col_a, col_b"), + kind=ViewKind(materialized=True), + ) + ) + snapshot_a_old.categorize_as(SnapshotChangeCategory.BREAKING, forward_only=False) + + snapshot_b_old = make_snapshot( + SqlModel( + name="b", + query=parse_one("select col_a from a"), + kind=ViewKind(materialized=True), + ), + nodes={'"a"': snapshot_a_old.model}, + ) + snapshot_b_old.categorize_as(SnapshotChangeCategory.BREAKING, forward_only=False) + + snapshot_a_new = make_snapshot( + SqlModel(name="a", + query=parse_one("select 2 as col_a, col_b"), + kind=ViewKind(materialized=True), + ) + ) + + snapshot_a_new.previous_versions = snapshot_a_old.all_versions + + snapshot_b_new = make_snapshot( + snapshot_b_old.model, + nodes={'"a"': snapshot_a_new.model}, + ) + snapshot_b_new.previous_versions = snapshot_b_old.all_versions + + context_diff = ContextDiff( + environment="test_environment", + is_new_environment=True, + is_unfinalized_environment=False, + normalize_environment_name=True, + create_from="prod", + create_from_env_exists=True, + added=set(), + removed_snapshots={}, + modified_snapshots={ + snapshot_a_new.name: (snapshot_a_new, snapshot_a_old), + snapshot_b_new.name: (snapshot_b_new, snapshot_b_old), + }, + snapshots={ + snapshot_a_new.snapshot_id: snapshot_a_new, + snapshot_b_new.snapshot_id: snapshot_b_new, + }, + new_snapshots={ + snapshot_a_new.snapshot_id: snapshot_a_new, + snapshot_b_new.snapshot_id: snapshot_b_new, + }, + previous_plan_id=None, + previously_promoted_snapshot_ids=set(), + previous_finalized_snapshots=None, + previous_gateway_managed_virtual_layer=False, + gateway_managed_virtual_layer=False, + environment_statements=[], + ) + + PlanBuilder(context_diff).build() + + assert snapshot_b_new.change_category == SnapshotChangeCategory.BREAKING From 9d10d01061bcf2c4a4da2368fe7c7a8da538979b Mon Sep 17 00:00:00 2001 From: Tomasz Zorawik <67728999+xardasos@users.noreply.github.com> Date: Mon, 15 Sep 2025 16:05:34 +0200 Subject: [PATCH 2/9] fix import --- tests/core/test_plan.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/core/test_plan.py b/tests/core/test_plan.py index 07c64919c0..46213d98ee 100644 --- a/tests/core/test_plan.py +++ b/tests/core/test_plan.py @@ -26,7 +26,7 @@ SqlModel, ModelKindName, ) -from sqlmesh.core.model.kind import OnDestructiveChange, OnAdditiveChange +from sqlmesh.core.model.kind import OnDestructiveChange, OnAdditiveChange, ViewKind from sqlmesh.core.model.seed import Seed from sqlmesh.core.plan import Plan, PlanBuilder, SnapshotIntervals from sqlmesh.core.snapshot import ( From 55918aeab0f371a6ff6666847bd8a21d1584762d Mon Sep 17 00:00:00 2001 From: Tomasz Zorawik <67728999+xardasos@users.noreply.github.com> Date: Mon, 15 Sep 2025 16:29:52 +0200 Subject: [PATCH 3/9] categorize changes to MVs as breaking --- sqlmesh/core/plan/builder.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sqlmesh/core/plan/builder.py b/sqlmesh/core/plan/builder.py index a84b3b60dc..cf2b3073eb 100644 --- a/sqlmesh/core/plan/builder.py +++ b/sqlmesh/core/plan/builder.py @@ -674,6 +674,12 @@ def _categorize_snapshot( if mode == AutoCategorizationMode.FULL: snapshot.categorize_as(SnapshotChangeCategory.BREAKING, forward_only) elif self._context_diff.indirectly_modified(snapshot.name): + if snapshot.is_materialized_view: + # We categorize changes as breaking to allow for instantaneous switches in a virtual layer. + # Otherwise, there might be a potentially long downtime during MVs recreation. + snapshot.categorize_as(SnapshotChangeCategory.BREAKING, forward_only) + return + all_upstream_forward_only = set() all_upstream_categories = set() direct_parent_categories = set() From 2e1bb9f8825c2ecf7c116e150c206c5e841ceb9f Mon Sep 17 00:00:00 2001 From: Tomasz Zorawik <67728999+xardasos@users.noreply.github.com> Date: Mon, 15 Sep 2025 17:06:36 +0200 Subject: [PATCH 4/9] code format --- tests/core/test_plan.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/tests/core/test_plan.py b/tests/core/test_plan.py index 46213d98ee..59d99c165e 100644 --- a/tests/core/test_plan.py +++ b/tests/core/test_plan.py @@ -4135,10 +4135,11 @@ def test_plan_ignore_cron_flag(make_snapshot): def test_indirect_change_to_materialized_view_is_breaking(make_snapshot): snapshot_a_old = make_snapshot( - SqlModel(name="a", - query=parse_one("select 1 as col_a, col_b"), - kind=ViewKind(materialized=True), - ) + SqlModel( + name="a", + query=parse_one("select 1 as col_a, col_b"), + kind=ViewKind(materialized=True), + ) ) snapshot_a_old.categorize_as(SnapshotChangeCategory.BREAKING, forward_only=False) @@ -4153,10 +4154,11 @@ def test_indirect_change_to_materialized_view_is_breaking(make_snapshot): snapshot_b_old.categorize_as(SnapshotChangeCategory.BREAKING, forward_only=False) snapshot_a_new = make_snapshot( - SqlModel(name="a", - query=parse_one("select 2 as col_a, col_b"), - kind=ViewKind(materialized=True), - ) + SqlModel( + name="a", + query=parse_one("select 2 as col_a, col_b"), + kind=ViewKind(materialized=True), + ) ) snapshot_a_new.previous_versions = snapshot_a_old.all_versions From 08e492d690db3e062a78b5f8264e5537a9f07923 Mon Sep 17 00:00:00 2001 From: Tomasz Zorawik <67728999+xardasos@users.noreply.github.com> Date: Mon, 15 Sep 2025 17:37:00 +0200 Subject: [PATCH 5/9] test - indirect breaking changes --- tests/core/test_plan.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/core/test_plan.py b/tests/core/test_plan.py index 59d99c165e..30e84d7689 100644 --- a/tests/core/test_plan.py +++ b/tests/core/test_plan.py @@ -4200,4 +4200,4 @@ def test_indirect_change_to_materialized_view_is_breaking(make_snapshot): PlanBuilder(context_diff).build() - assert snapshot_b_new.change_category == SnapshotChangeCategory.BREAKING + assert snapshot_b_new.change_category == SnapshotChangeCategory.INDIRECT_BREAKING From 97f001e2a13efce1b1f56f72f5e08a7aff76cdd2 Mon Sep 17 00:00:00 2001 From: Tomasz Zorawik <67728999+xardasos@users.noreply.github.com> Date: Mon, 15 Sep 2025 17:38:25 +0200 Subject: [PATCH 6/9] indirect breaking --- sqlmesh/core/plan/builder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sqlmesh/core/plan/builder.py b/sqlmesh/core/plan/builder.py index cf2b3073eb..7ee7dfa6f0 100644 --- a/sqlmesh/core/plan/builder.py +++ b/sqlmesh/core/plan/builder.py @@ -677,7 +677,7 @@ def _categorize_snapshot( if snapshot.is_materialized_view: # We categorize changes as breaking to allow for instantaneous switches in a virtual layer. # Otherwise, there might be a potentially long downtime during MVs recreation. - snapshot.categorize_as(SnapshotChangeCategory.BREAKING, forward_only) + snapshot.categorize_as(SnapshotChangeCategory.INDIRECT_BREAKING, forward_only) return all_upstream_forward_only = set() From 6609057e6786035a3827346e6b259216f8d182e9 Mon Sep 17 00:00:00 2001 From: Tomasz Zorawik <67728999+xardasos@users.noreply.github.com> Date: Tue, 16 Sep 2025 17:57:18 +0200 Subject: [PATCH 7/9] test forward-only --- tests/core/test_plan.py | 78 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 5 deletions(-) diff --git a/tests/core/test_plan.py b/tests/core/test_plan.py index 30e84d7689..2b73b695d5 100644 --- a/tests/core/test_plan.py +++ b/tests/core/test_plan.py @@ -4137,11 +4137,11 @@ def test_indirect_change_to_materialized_view_is_breaking(make_snapshot): snapshot_a_old = make_snapshot( SqlModel( name="a", - query=parse_one("select 1 as col_a, col_b"), + query=parse_one("select 1 as col_a"), kind=ViewKind(materialized=True), ) ) - snapshot_a_old.categorize_as(SnapshotChangeCategory.BREAKING, forward_only=False) + snapshot_a_old.categorize_as(SnapshotChangeCategory.BREAKING) snapshot_b_old = make_snapshot( SqlModel( @@ -4151,12 +4151,12 @@ def test_indirect_change_to_materialized_view_is_breaking(make_snapshot): ), nodes={'"a"': snapshot_a_old.model}, ) - snapshot_b_old.categorize_as(SnapshotChangeCategory.BREAKING, forward_only=False) + snapshot_b_old.categorize_as(SnapshotChangeCategory.BREAKING) snapshot_a_new = make_snapshot( SqlModel( name="a", - query=parse_one("select 2 as col_a, col_b"), + query=parse_one("select 1 as col_a, 2 as col_b"), kind=ViewKind(materialized=True), ) ) @@ -4198,6 +4198,74 @@ def test_indirect_change_to_materialized_view_is_breaking(make_snapshot): environment_statements=[], ) - PlanBuilder(context_diff).build() + PlanBuilder(context_diff, forward_only=False).build() assert snapshot_b_new.change_category == SnapshotChangeCategory.INDIRECT_BREAKING + + +def test_forward_only_indirect_change_to_materialized_view(make_snapshot): + snapshot_a_old = make_snapshot( + SqlModel( + name="a", + query=parse_one("select 1 as col_a"), + ) + ) + snapshot_a_old.categorize_as(SnapshotChangeCategory.BREAKING) + + snapshot_b_old = make_snapshot( + SqlModel( + name="b", + query=parse_one("select col_a from a"), + kind=ViewKind(materialized=True), + ), + nodes={'"a"': snapshot_a_old.model}, + ) + snapshot_b_old.categorize_as(SnapshotChangeCategory.BREAKING) + + snapshot_a_new = make_snapshot( + SqlModel( + name="a", + query=parse_one("select 1 as col_a, 2 as col_b"), + ) + ) + + snapshot_a_new.previous_versions = snapshot_a_old.all_versions + + snapshot_b_new = make_snapshot( + snapshot_b_old.model, + nodes={'"a"': snapshot_a_new.model}, + ) + snapshot_b_new.previous_versions = snapshot_b_old.all_versions + + context_diff = ContextDiff( + environment="test_environment", + is_new_environment=True, + is_unfinalized_environment=False, + normalize_environment_name=True, + create_from="prod", + create_from_env_exists=True, + added=set(), + removed_snapshots={}, + modified_snapshots={ + snapshot_a_new.name: (snapshot_a_new, snapshot_a_old), + snapshot_b_new.name: (snapshot_b_new, snapshot_b_old), + }, + snapshots={ + snapshot_a_new.snapshot_id: snapshot_a_new, + snapshot_b_new.snapshot_id: snapshot_b_new, + }, + new_snapshots={ + snapshot_a_new.snapshot_id: snapshot_a_new, + snapshot_b_new.snapshot_id: snapshot_b_new, + }, + previous_plan_id=None, + previously_promoted_snapshot_ids=set(), + previous_finalized_snapshots=None, + previous_gateway_managed_virtual_layer=False, + gateway_managed_virtual_layer=False, + environment_statements=[], + ) + + PlanBuilder(context_diff, forward_only=True).build() + + assert snapshot_b_new.change_category == SnapshotChangeCategory.INDIRECT_NON_BREAKING From 3dc06577e8573d09c72c5cb2c3f7b78a417a9085 Mon Sep 17 00:00:00 2001 From: Tomasz Zorawik <67728999+xardasos@users.noreply.github.com> Date: Tue, 16 Sep 2025 17:59:31 +0200 Subject: [PATCH 8/9] indirect forward-only changes: don't always categorize as breaking --- sqlmesh/core/plan/builder.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sqlmesh/core/plan/builder.py b/sqlmesh/core/plan/builder.py index 7ee7dfa6f0..176335926b 100644 --- a/sqlmesh/core/plan/builder.py +++ b/sqlmesh/core/plan/builder.py @@ -674,9 +674,11 @@ def _categorize_snapshot( if mode == AutoCategorizationMode.FULL: snapshot.categorize_as(SnapshotChangeCategory.BREAKING, forward_only) elif self._context_diff.indirectly_modified(snapshot.name): - if snapshot.is_materialized_view: + if snapshot.is_materialized_view and not forward_only: # We categorize changes as breaking to allow for instantaneous switches in a virtual layer. # Otherwise, there might be a potentially long downtime during MVs recreation. + # In the case of forward-only changes this optimization is not applicable because we continue using the + # same (existing) table version. snapshot.categorize_as(SnapshotChangeCategory.INDIRECT_BREAKING, forward_only) return From ea775fbd003fbb35071956b75d3d8937e8337776 Mon Sep 17 00:00:00 2001 From: Tomasz Zorawik <67728999+xardasos@users.noreply.github.com> Date: Wed, 17 Sep 2025 13:26:13 +0000 Subject: [PATCH 9/9] add comments --- sqlmesh/core/plan/builder.py | 4 ++-- tests/core/test_plan.py | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/sqlmesh/core/plan/builder.py b/sqlmesh/core/plan/builder.py index d47567546d..7d753cc330 100644 --- a/sqlmesh/core/plan/builder.py +++ b/sqlmesh/core/plan/builder.py @@ -683,8 +683,8 @@ def _categorize_snapshot( if snapshot.is_materialized_view and not forward_only: # We categorize changes as breaking to allow for instantaneous switches in a virtual layer. # Otherwise, there might be a potentially long downtime during MVs recreation. - # In the case of forward-only changes this optimization is not applicable because we continue using the - # same (existing) table version. + # In the case of forward-only changes this optimization is not applicable because we want to continue + # using the same (existing) table version. snapshot.categorize_as(SnapshotChangeCategory.INDIRECT_BREAKING, forward_only) return diff --git a/tests/core/test_plan.py b/tests/core/test_plan.py index 211c8a691e..30a12f11b5 100644 --- a/tests/core/test_plan.py +++ b/tests/core/test_plan.py @@ -4239,4 +4239,6 @@ def test_forward_only_indirect_change_to_materialized_view(make_snapshot): PlanBuilder(context_diff, forward_only=True).build() + # Forward-only indirect changes to MVs should not always be classified as indirect breaking. + # Instead, we want to preserve the standard categorization. assert snapshot_b_new.change_category == SnapshotChangeCategory.INDIRECT_NON_BREAKING