From fe1414f9f6f9cba16514a5b076b00225511fa02b Mon Sep 17 00:00:00 2001 From: Mike Alfare <13974384+mikealfare@users.noreply.github.com> Date: Tue, 7 Nov 2023 19:44:06 -0500 Subject: [PATCH] ADAP-923: Resolve issue where backup parameter was inadvertently considered in configuration change monitoring (#657) * changelog * add a test demonstrating the issue * remove backup parameter from config change monitoring --- .../unreleased/Fixes-20231103-181357.yaml | 6 ++++ dbt/adapters/redshift/relation.py | 7 ----- .../redshift/relation_configs/__init__.py | 1 - .../relation_configs/materialized_view.py | 16 ++-------- .../test_materialized_views.py | 30 +++++++++++++++++-- 5 files changed, 35 insertions(+), 25 deletions(-) create mode 100644 .changes/unreleased/Fixes-20231103-181357.yaml diff --git a/.changes/unreleased/Fixes-20231103-181357.yaml b/.changes/unreleased/Fixes-20231103-181357.yaml new file mode 100644 index 000000000..2995560d2 --- /dev/null +++ b/.changes/unreleased/Fixes-20231103-181357.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: Fix handling of `backup` parameter during config change monitoring +time: 2023-11-03T18:13:57.401994-04:00 +custom: + Author: mikealfare + Issue: "621" diff --git a/dbt/adapters/redshift/relation.py b/dbt/adapters/redshift/relation.py index 182b9b810..ba2ad4a5a 100644 --- a/dbt/adapters/redshift/relation.py +++ b/dbt/adapters/redshift/relation.py @@ -16,7 +16,6 @@ RedshiftMaterializedViewConfig, RedshiftMaterializedViewConfigChangeset, RedshiftAutoRefreshConfigChange, - RedshiftBackupConfigChange, RedshiftDistConfigChange, RedshiftSortConfigChange, RedshiftIncludePolicy, @@ -93,12 +92,6 @@ def materialized_view_config_changeset( context=new_materialized_view.autorefresh, ) - if new_materialized_view.backup != existing_materialized_view.backup: - config_change_collection.backup = RedshiftBackupConfigChange( - action=RelationConfigChangeAction.alter, - context=new_materialized_view.backup, - ) - if new_materialized_view.dist != existing_materialized_view.dist: config_change_collection.dist = RedshiftDistConfigChange( action=RelationConfigChangeAction.alter, diff --git a/dbt/adapters/redshift/relation_configs/__init__.py b/dbt/adapters/redshift/relation_configs/__init__.py index 26e36c86c..0bc69ef4a 100644 --- a/dbt/adapters/redshift/relation_configs/__init__.py +++ b/dbt/adapters/redshift/relation_configs/__init__.py @@ -9,7 +9,6 @@ from dbt.adapters.redshift.relation_configs.materialized_view import ( RedshiftMaterializedViewConfig, RedshiftAutoRefreshConfigChange, - RedshiftBackupConfigChange, RedshiftMaterializedViewConfigChangeset, ) from dbt.adapters.redshift.relation_configs.policies import ( diff --git a/dbt/adapters/redshift/relation_configs/materialized_view.py b/dbt/adapters/redshift/relation_configs/materialized_view.py index 44e18e3ac..e19b45547 100644 --- a/dbt/adapters/redshift/relation_configs/materialized_view.py +++ b/dbt/adapters/redshift/relation_configs/materialized_view.py @@ -1,4 +1,4 @@ -from dataclasses import dataclass +from dataclasses import dataclass, field from typing import Optional, Set import agate @@ -56,7 +56,7 @@ class RedshiftMaterializedViewConfig(RedshiftRelationConfigBase, RelationConfigV schema_name: str database_name: str query: str - backup: bool = True + backup: bool = field(default=True, compare=False, hash=False) dist: RedshiftDistConfig = RedshiftDistConfig(diststyle=RedshiftDistStyle.even) sort: RedshiftSortConfig = RedshiftSortConfig() autorefresh: bool = False @@ -241,18 +241,8 @@ def requires_full_refresh(self) -> bool: return False -@dataclass(frozen=True, eq=True, unsafe_hash=True) -class RedshiftBackupConfigChange(RelationConfigChange): - context: Optional[bool] = None - - @property - def requires_full_refresh(self) -> bool: - return True - - @dataclass class RedshiftMaterializedViewConfigChangeset: - backup: Optional[RedshiftBackupConfigChange] = None dist: Optional[RedshiftDistConfigChange] = None sort: Optional[RedshiftSortConfigChange] = None autorefresh: Optional[RedshiftAutoRefreshConfigChange] = None @@ -262,7 +252,6 @@ def requires_full_refresh(self) -> bool: return any( { self.autorefresh.requires_full_refresh if self.autorefresh else False, - self.backup.requires_full_refresh if self.backup else False, self.dist.requires_full_refresh if self.dist else False, self.sort.requires_full_refresh if self.sort else False, } @@ -272,7 +261,6 @@ def requires_full_refresh(self) -> bool: def has_changes(self) -> bool: return any( { - self.backup if self.backup else False, self.dist if self.dist else False, self.sort if self.sort else False, self.autorefresh if self.autorefresh else False, diff --git a/tests/functional/adapter/materialized_view_tests/test_materialized_views.py b/tests/functional/adapter/materialized_view_tests/test_materialized_views.py index d2bf1cda4..63bcede61 100644 --- a/tests/functional/adapter/materialized_view_tests/test_materialized_views.py +++ b/tests/functional/adapter/materialized_view_tests/test_materialized_views.py @@ -3,6 +3,7 @@ import pytest from dbt.adapters.base.relation import BaseRelation +from dbt.contracts.graph.model_config import OnConfigurationChangeOption from dbt.tests.adapter.materialized_view.basic import MaterializedViewBasic from dbt.tests.adapter.materialized_view.changes import ( @@ -246,7 +247,30 @@ def models(self): def seeds(self): return {"my_seed.csv": MY_SEED} - def test_running_mv_with_backup_false_succeeds(self, project): + @pytest.fixture(scope="class") + def project_config_update(self): + return {"models": {"on_configuration_change": OnConfigurationChangeOption.Fail.value}} + + @pytest.fixture(scope="function") + def dbt_run_results(self, project): run_dbt(["seed"]) - result = run_dbt(["run"]) - assert result[0].node.config_call_dict["backup"] is False + yield run_dbt(["run", "--full-refresh"]) + + def test_running_mv_with_backup_false_succeeds(self, dbt_run_results): + assert dbt_run_results[0].node.config_call_dict["backup"] is False + + def test_running_mv_with_backup_false_is_idempotent(self, project, dbt_run_results): + """ + Addresses: https://github.com/dbt-labs/dbt-redshift/issues/621 + Context: + - The default for `backup` is `True` + - We cannot query `backup` for a materialized view in Redshift at the moment + Premise: + - Set `on_configuration_change` = 'fail' (via `project_config_update`) + - Set `backup` = False (via `NO_BACKUP_MATERIALIZED_VIEW`) + - Create the materialized view (via `dbt_run_results`) + - Run a second time forcing the configuration change monitoring + - There should be no changes monitored, hence the run should be successful + """ + results = run_dbt(["run"]) + assert results[0].node.config_call_dict["backup"] is False