From ba671efd129fcac11b4d766ae6c4d03d79e02dd1 Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Tue, 4 Jun 2024 11:10:23 -0400 Subject: [PATCH 1/6] Test not failing --- .../saved_queries/test_saved_query_parsing.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/functional/saved_queries/test_saved_query_parsing.py b/tests/functional/saved_queries/test_saved_query_parsing.py index ce3763acfbc..8aff5c09c6c 100644 --- a/tests/functional/saved_queries/test_saved_query_parsing.py +++ b/tests/functional/saved_queries/test_saved_query_parsing.py @@ -1,4 +1,6 @@ from typing import List +import os +import shutil import pytest from dbt_semantic_interfaces.type_enums.export_destination_type import ( @@ -6,7 +8,7 @@ ) from dbt.contracts.graph.manifest import Manifest -from dbt.tests.util import write_file +from dbt.tests.util import write_file, run_dbt from dbt_common.events.base_types import BaseEvent from tests.functional.assertions.test_runner import dbtTestRunner from tests.functional.saved_queries.fixtures import ( @@ -32,6 +34,11 @@ def models(self): "docs.md": saved_query_description, } + def copy_state(self): + if not os.path.exists("state"): + os.makedirs("state") + shutil.copyfile("target/manifest.json", "state/manifest.json") + def test_semantic_model_parsing(self, project): runner = dbtTestRunner() result = runner.invoke(["parse", "--no-partial-parse"]) @@ -52,6 +59,10 @@ def test_semantic_model_parsing(self, project): assert saved_query.exports[0].config.export_as == ExportDestinationType.TABLE assert saved_query.exports[0].config.schema_name == "my_export_schema_name" + self.copy_state() + results = run_dbt(["ls", "--select", "state:modified", "--state", "./state"]) + assert len(results) == 0 + def test_saved_query_error(self, project): error_schema_yml = saved_queries_yml.replace("simple_metric", "metric_not_found") write_file(error_schema_yml, project.project_root, "models", "saved_queries.yml") From 8e666380df54f446dd6dde6a2be82591dc21a2af Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Tue, 11 Jun 2024 14:16:34 -0400 Subject: [PATCH 2/6] Fix deferred manifest and selector methods for saved_queries --- core/dbt/contracts/graph/manifest.py | 1 + core/dbt/graph/selector_methods.py | 8 ++++++-- tests/functional/saved_queries/fixtures.py | 12 ------------ .../saved_queries/test_saved_query_parsing.py | 18 ++++++++++++++++-- 4 files changed, 23 insertions(+), 16 deletions(-) diff --git a/core/dbt/contracts/graph/manifest.py b/core/dbt/contracts/graph/manifest.py index 9ca11166388..95b018bd69b 100644 --- a/core/dbt/contracts/graph/manifest.py +++ b/core/dbt/contracts/graph/manifest.py @@ -1094,6 +1094,7 @@ def from_writable_manifest(cls, writable_manifest: WritableManifest) -> "Manifes metrics=cls._map_resources_to_map_nodes(writable_manifest.metrics), groups=cls._map_resources_to_map_nodes(writable_manifest.groups), semantic_models=cls._map_resources_to_map_nodes(writable_manifest.semantic_models), + saved_queries=cls._map_resources_to_map_nodes(writable_manifest.saved_queries), selectors={ selector_id: selector for selector_id, selector in writable_manifest.selectors.items() diff --git a/core/dbt/graph/selector_methods.py b/core/dbt/graph/selector_methods.py index 31d6d6c1fc6..dbeaf7ed4c3 100644 --- a/core/dbt/graph/selector_methods.py +++ b/core/dbt/graph/selector_methods.py @@ -109,7 +109,7 @@ def is_selected_node(fqn: List[str], node_selector: str, is_versioned: bool) -> SelectorTarget = Union[ - SourceDefinition, ManifestNode, Exposure, Metric, SemanticModel, UnitTestDefinition + SourceDefinition, ManifestNode, Exposure, Metric, SemanticModel, UnitTestDefinition, SavedQuery ] @@ -202,6 +202,7 @@ def all_nodes( self.metric_nodes(included_nodes), self.unit_tests(included_nodes), self.semantic_model_nodes(included_nodes), + self.saved_query_nodes(included_nodes), ) def configurable_nodes( @@ -680,7 +681,8 @@ def check_modified_content( self, old: Optional[SelectorTarget], new: SelectorTarget, adapter_type: str ) -> bool: if isinstance( - new, (SourceDefinition, Exposure, Metric, SemanticModel, UnitTestDefinition) + new, + (SourceDefinition, Exposure, Metric, SemanticModel, UnitTestDefinition, SavedQuery), ): # these all overwrite `same_contents` different_contents = not new.same_contents(old) # type: ignore @@ -775,6 +777,8 @@ def search(self, included_nodes: Set[UniqueId], selector: str) -> Iterator[Uniqu previous_node = SemanticModel.from_resource(manifest.semantic_models[unique_id]) elif unique_id in manifest.unit_tests: previous_node = UnitTestDefinition.from_resource(manifest.unit_tests[unique_id]) + elif unique_id in manifest.saved_queries: + previous_node = SavedQuery.from_resource(manifest.saved_queries[unique_id]) keyword_args = {} if checker.__name__ in [ diff --git a/tests/functional/saved_queries/fixtures.py b/tests/functional/saved_queries/fixtures.py index e938760a12e..77735c9d1a9 100644 --- a/tests/functional/saved_queries/fixtures.py +++ b/tests/functional/saved_queries/fixtures.py @@ -3,8 +3,6 @@ """ saved_queries_yml = """ -version: 2 - saved_queries: - name: test_saved_query description: "{{ doc('saved_query_description') }}" @@ -27,8 +25,6 @@ """ saved_queries_with_defaults_yml = """ -version: 2 - saved_queries: - name: test_saved_query description: "{{ doc('saved_query_description') }}" @@ -50,8 +46,6 @@ """ saved_queries_with_diff_filters_yml = """ -version: 2 - saved_queries: - name: test_saved_query_where_list description: "{{ doc('saved_query_description') }}" @@ -83,8 +77,6 @@ """ saved_query_with_extra_config_attributes_yml = """ -version: 2 - saved_queries: - name: test_saved_query description: "{{ doc('saved_query_description') }}" @@ -105,8 +97,6 @@ """ saved_query_with_export_configs_defined_at_saved_query_level_yml = """ -version: 2 - saved_queries: - name: test_saved_query description: "{{ doc('saved_query_description') }}" @@ -131,8 +121,6 @@ """ saved_query_without_export_configs_defined_yml = """ -version: 2 - saved_queries: - name: test_saved_query description: "{{ doc('saved_query_description') }}" diff --git a/tests/functional/saved_queries/test_saved_query_parsing.py b/tests/functional/saved_queries/test_saved_query_parsing.py index 8aff5c09c6c..5d4c9536501 100644 --- a/tests/functional/saved_queries/test_saved_query_parsing.py +++ b/tests/functional/saved_queries/test_saved_query_parsing.py @@ -1,6 +1,6 @@ -from typing import List import os import shutil +from typing import List import pytest from dbt_semantic_interfaces.type_enums.export_destination_type import ( @@ -8,13 +8,14 @@ ) from dbt.contracts.graph.manifest import Manifest -from dbt.tests.util import write_file, run_dbt +from dbt.tests.util import run_dbt, write_file from dbt_common.events.base_types import BaseEvent from tests.functional.assertions.test_runner import dbtTestRunner from tests.functional.saved_queries.fixtures import ( saved_queries_with_diff_filters_yml, saved_queries_yml, saved_query_description, + saved_query_with_cache_configs_defined_yml, ) from tests.functional.semantic_models.fixtures import ( fct_revenue_sql, @@ -59,10 +60,23 @@ def test_semantic_model_parsing(self, project): assert saved_query.exports[0].config.export_as == ExportDestinationType.TABLE assert saved_query.exports[0].config.schema_name == "my_export_schema_name" + # Save state self.copy_state() + # Nothing has changed, so no state:modified results results = run_dbt(["ls", "--select", "state:modified", "--state", "./state"]) assert len(results) == 0 + # Change saved_query + write_file( + saved_query_with_cache_configs_defined_yml, + project.project_root, + "models", + "saved_queries.yml", + ) + # State modified finds changed saved_query + results = run_dbt(["ls", "--select", "state:modified", "--state", "./state"]) + assert len(results) == 1 + def test_saved_query_error(self, project): error_schema_yml = saved_queries_yml.replace("simple_metric", "metric_not_found") write_file(error_schema_yml, project.project_root, "models", "saved_queries.yml") From 3957c1ea791c25a9de108a4864f9310c085fa4e6 Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Wed, 12 Jun 2024 15:06:07 -0400 Subject: [PATCH 3/6] use same_exports in saved query state modified --- core/dbt/contracts/graph/nodes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/dbt/contracts/graph/nodes.py b/core/dbt/contracts/graph/nodes.py index af5b317f2f8..7edec1fefe3 100644 --- a/core/dbt/contracts/graph/nodes.py +++ b/core/dbt/contracts/graph/nodes.py @@ -1562,7 +1562,6 @@ def same_group(self, old: "SavedQuery") -> bool: return self.group == old.group def same_exports(self, old: "SavedQuery") -> bool: - # TODO: This isn't currently used in `same_contents` (nor called anywhere else) if len(self.exports) != len(old.exports): return False @@ -1592,6 +1591,7 @@ def same_contents(self, old: Optional["SavedQuery"]) -> bool: and self.same_label(old) and self.same_config(old) and self.same_group(old) + and self.same_exports(old) and True ) From 3cce059090f493bd0c7d0045d812ee78d2975a39 Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Wed, 12 Jun 2024 15:16:21 -0400 Subject: [PATCH 4/6] Add test for changed exports config --- .../functional/saved_queries/test_saved_query_parsing.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/functional/saved_queries/test_saved_query_parsing.py b/tests/functional/saved_queries/test_saved_query_parsing.py index 5d4c9536501..1c0a56b95ca 100644 --- a/tests/functional/saved_queries/test_saved_query_parsing.py +++ b/tests/functional/saved_queries/test_saved_query_parsing.py @@ -12,6 +12,7 @@ from dbt_common.events.base_types import BaseEvent from tests.functional.assertions.test_runner import dbtTestRunner from tests.functional.saved_queries.fixtures import ( + saved_queries_with_defaults_yml, saved_queries_with_diff_filters_yml, saved_queries_yml, saved_query_description, @@ -77,6 +78,14 @@ def test_semantic_model_parsing(self, project): results = run_dbt(["ls", "--select", "state:modified", "--state", "./state"]) assert len(results) == 1 + # change exports + write_file( + saved_queries_with_defaults_yml, project.project_root, "models", "saved_queries.yml" + ) + # State modified finds changed saved_query + results = run_dbt(["ls", "--select", "state:modified", "--state", "./state"]) + assert len(results) == 1 + def test_saved_query_error(self, project): error_schema_yml = saved_queries_yml.replace("simple_metric", "metric_not_found") write_file(error_schema_yml, project.project_root, "models", "saved_queries.yml") From 2bd551ed9e23505082cd881ac3e878a16ba155cf Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Wed, 12 Jun 2024 15:21:52 -0400 Subject: [PATCH 5/6] Changie --- .changes/unreleased/Fixes-20240612-152139.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/unreleased/Fixes-20240612-152139.yaml diff --git a/.changes/unreleased/Fixes-20240612-152139.yaml b/.changes/unreleased/Fixes-20240612-152139.yaml new file mode 100644 index 00000000000..8881e470780 --- /dev/null +++ b/.changes/unreleased/Fixes-20240612-152139.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: Implement state:modified for saved queries +time: 2024-06-12T15:21:39.851426-04:00 +custom: + Author: gshank + Issue: "10294" From c9152835fcbcf3a86345fcd1279d41816591588e Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Mon, 17 Jun 2024 22:40:46 -0400 Subject: [PATCH 6/6] Create unit test for modified saved query --- tests/unit/graph/test_selector_methods.py | 29 +++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/tests/unit/graph/test_selector_methods.py b/tests/unit/graph/test_selector_methods.py index 28ed9202f86..455d1585b95 100644 --- a/tests/unit/graph/test_selector_methods.py +++ b/tests/unit/graph/test_selector_methods.py @@ -587,6 +587,27 @@ def test_select_saved_query_by_tag(manifest: Manifest) -> None: search_manifest_using_method(manifest, method, "any_tag") +def test_modified_saved_query(manifest: Manifest) -> None: + metric = make_metric("test", "my_metric") + saved_query = make_saved_query( + "pkg", + "test_saved_query", + "my_metric", + ) + manifest.metrics[metric.unique_id] = metric + manifest.saved_queries[saved_query.unique_id] = saved_query + # Create PreviousState with a saved query, this deepcopies manifest + previous_state = create_previous_state(manifest) + method = statemethod(manifest, previous_state) + + # create another metric and add to saved query + alt_metric = make_metric("test", "alt_metric") + manifest.metrics[alt_metric.unique_id] = alt_metric + saved_query.query_params.metrics.append("alt_metric") + + assert search_manifest_using_method(manifest, method, "modified") == {"test_saved_query"} + + def test_select_unit_test(manifest: Manifest) -> None: test_model = make_model("test", "my_model", "select 1 as id") unit_test = make_unit_test("test", "my_unit_test", test_model) @@ -605,8 +626,7 @@ def test_select_unit_test(manifest: Manifest) -> None: } -@pytest.fixture -def previous_state(manifest): +def create_previous_state(manifest): writable = copy.deepcopy(manifest).writable_manifest() state = PreviousState( state_path=Path("/path/does/not/exist"), @@ -617,6 +637,11 @@ def previous_state(manifest): return state +@pytest.fixture +def previous_state(manifest): + return create_previous_state(manifest) + + def add_node(manifest, node): manifest.nodes[node.unique_id] = node