From 7854bfa2fe4e7b8fd6bf0218d8319b6d5bec004a Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Thu, 2 May 2024 01:45:58 +0200 Subject: [PATCH] PR review. DeferRelation conforms to RelationConfig protocol --- core/dbt/artifacts/resources/v1/components.py | 9 +++++++++ .../artifacts/schemas/manifest/v12/manifest.py | 2 ++ core/dbt/contracts/graph/manifest.py | 17 +++++++++++++++-- core/dbt/contracts/graph/nodes.py | 5 +++++ core/dbt/task/clone.py | 8 +++++++- core/dbt/task/runnable.py | 9 +-------- tests/functional/artifacts/expected_manifest.py | 12 ------------ tests/unit/contracts/graph/test_manifest.py | 4 ++-- 8 files changed, 41 insertions(+), 25 deletions(-) diff --git a/core/dbt/artifacts/resources/v1/components.py b/core/dbt/artifacts/resources/v1/components.py index cb387304260..aca72c5d0f2 100644 --- a/core/dbt/artifacts/resources/v1/components.py +++ b/core/dbt/artifacts/resources/v1/components.py @@ -1,6 +1,7 @@ import time from dataclasses import dataclass, field from dbt.artifacts.resources.base import GraphResource, FileHash, Docs +from dbt.artifacts.resources.types import NodeType from dbt.artifacts.resources.v1.config import NodeConfig from dbt_common.dataclass_schema import dbtClassMixin, ExtensibleDbtClassMixin from dbt_common.contracts.config.properties import AdditionalPropertiesMixin @@ -154,6 +155,14 @@ def quoting_dict(self) -> Dict[str, bool]: class DeferRelation(HasRelationMetadata): alias: str relation_name: Optional[str] + # The rest of these fields match RelationConfig protocol exactly + resource_type: NodeType + name: str + description: str + compiled_code: Optional[str] + meta: Dict[str, Any] + tags: List[str] + config: Optional[NodeConfig] @property def identifier(self): diff --git a/core/dbt/artifacts/schemas/manifest/v12/manifest.py b/core/dbt/artifacts/schemas/manifest/v12/manifest.py index 088d2ff9023..2ac3f3d761c 100644 --- a/core/dbt/artifacts/schemas/manifest/v12/manifest.py +++ b/core/dbt/artifacts/schemas/manifest/v12/manifest.py @@ -186,4 +186,6 @@ def __post_serialize__(self, dct): for unique_id, node in dct["nodes"].items(): if "config_call_dict" in node: del node["config_call_dict"] + if "defer_relation" in node: + del node["defer_relation"] return dct diff --git a/core/dbt/contracts/graph/manifest.py b/core/dbt/contracts/graph/manifest.py index 5d2b1df9e31..75930fba3de 100644 --- a/core/dbt/contracts/graph/manifest.py +++ b/core/dbt/contracts/graph/manifest.py @@ -38,6 +38,7 @@ ResultNode, SavedQuery, SemanticModel, + SeedNode, SourceDefinition, UnpatchedSourceDefinition, UnitTestDefinition, @@ -53,6 +54,7 @@ DeferRelation, BaseResource, ) +from dbt.artifacts.resources.v1.config import NodeConfig from dbt.artifacts.schemas.manifest import WritableManifest, ManifestMetadata, UniqueID from dbt.contracts.files import ( SourceFile, @@ -1466,7 +1468,7 @@ def is_invalid_protected_ref( ) # Called in GraphRunnableTask.before_run, RunTask.before_run, CloneTask.before_run - def merge_from_artifact(self, other: "Manifest", favor_state: bool = False) -> None: + def merge_from_artifact(self, other: "Manifest") -> None: """Update this manifest by adding the 'defer_relation' attribute to all nodes with a counterpart in the stateful manifest used for deferral. @@ -1476,8 +1478,19 @@ def merge_from_artifact(self, other: "Manifest", favor_state: bool = False) -> N for unique_id, node in other.nodes.items(): current = self.nodes.get(unique_id) if current and node.resource_type in refables and not node.is_ephemeral: + assert isinstance(node.config, NodeConfig) # this makes mypy happy defer_relation = DeferRelation( - node.database, node.schema, node.alias, node.relation_name + database=node.database, + schema=node.schema, + alias=node.alias, + relation_name=node.relation_name, + resource_type=node.resource_type, + name=node.name, + description=node.description, + compiled_code=(node.compiled_code if not isinstance(node, SeedNode) else None), + meta=node.meta, + tags=node.tags, + config=node.config, ) self.nodes[unique_id] = replace(current, defer_relation=defer_relation) diff --git a/core/dbt/contracts/graph/nodes.py b/core/dbt/contracts/graph/nodes.py index e1f409ff1de..2a7752500e2 100644 --- a/core/dbt/contracts/graph/nodes.py +++ b/core/dbt/contracts/graph/nodes.py @@ -877,6 +877,11 @@ def language(self): return "sql" +# @property +# def compiled_code(self): +# return None + + # ==================================== # Singular Test node # ==================================== diff --git a/core/dbt/task/clone.py b/core/dbt/task/clone.py index 30187c31fe3..8bf5cf6112f 100644 --- a/core/dbt/task/clone.py +++ b/core/dbt/task/clone.py @@ -1,9 +1,10 @@ import threading -from typing import AbstractSet, Any, List, Iterable, Set +from typing import AbstractSet, Any, List, Iterable, Set, Optional from dbt.adapters.base import BaseRelation from dbt.clients.jinja import MacroGenerator from dbt.context.providers import generate_runtime_model_context +from dbt.contracts.graph.manifest import Manifest from dbt.artifacts.schemas.run import RunStatus, RunResult from dbt_common.dataclass_schema import dbtClassMixin from dbt_common.exceptions import DbtInternalError, CompilationError @@ -93,6 +94,11 @@ class CloneTask(GraphRunnableTask): def raise_on_first_error(self): return False + def _get_deferred_manifest(self) -> Optional[Manifest]: + # Unlike other commands, 'clone' always requires a state manifest + # Load previous state, regardless of whether --defer flag has been set + return self._get_previous_state() + def get_model_schemas(self, adapter, selected_uids: Iterable[str]) -> Set[BaseRelation]: if self.manifest is None: raise DbtInternalError("manifest was None in get_model_schemas") diff --git a/core/dbt/task/runnable.py b/core/dbt/task/runnable.py index b19bc5a811b..82d4561ad93 100644 --- a/core/dbt/task/runnable.py +++ b/core/dbt/task/runnable.py @@ -135,14 +135,7 @@ def defer_to_manifest(self): raise DbtInternalError( "Expected to defer to manifest, but there is no runtime manifest to defer from!" ) - self.manifest.merge_from_artifact( - other=deferred_manifest, - favor_state=bool(self.args.favor_state), - ) - # We're rewriting the manifest because it's been mutated during merge_from_artifact. - # This is to reflect which nodes had the defer_relation attribute added. - if self.args.write_json: - write_manifest(self.manifest, self.config.project_target_path) + self.manifest.merge_from_artifact(other=deferred_manifest) def get_graph_queue(self) -> GraphQueue: selector = self.get_node_selector() diff --git a/tests/functional/artifacts/expected_manifest.py b/tests/functional/artifacts/expected_manifest.py index 6ea0447354a..4d2a58592c6 100644 --- a/tests/functional/artifacts/expected_manifest.py +++ b/tests/functional/artifacts/expected_manifest.py @@ -279,7 +279,6 @@ def expected_seeded_manifest(project, model_database=None, quote_model=False): "group": None, "schema": my_schema_name, "database": model_database, - "defer_relation": None, "alias": "model", "description": "The test model", "columns": { @@ -372,7 +371,6 @@ def expected_seeded_manifest(project, model_database=None, quote_model=False): "group": None, "schema": alternate_schema, "database": project.database, - "defer_relation": None, "alias": "second_model", "description": "The second test model", "columns": { @@ -457,7 +455,6 @@ def expected_seeded_manifest(project, model_database=None, quote_model=False): "schema": my_schema_name, "database": project.database, "alias": "seed", - "defer_relation": None, "description": "The test seed", "columns": { "id": { @@ -579,7 +576,6 @@ def expected_seeded_manifest(project, model_database=None, quote_model=False): "contract": {"checksum": None, "enforced": False, "alias_types": True}, "database": project.database, "group": None, - "defer_relation": None, "depends_on": { "macros": [], "nodes": ["seed.test.seed"], @@ -926,7 +922,6 @@ def expected_references_manifest(project): "nodes": ["source.test.my_source.my_table"], }, "deprecation_date": None, - "defer_relation": None, "description": "", "docs": {"node_color": None, "show": True}, "fqn": ["test", "ephemeral_copy"], @@ -992,7 +987,6 @@ def expected_references_manifest(project): "nodes": ["model.test.ephemeral_copy"], }, "deprecation_date": None, - "defer_relation": None, "description": "A summmary table of the ephemeral copy of the seed data", "docs": {"node_color": None, "show": True}, "fqn": ["test", "ephemeral_summary"], @@ -1061,7 +1055,6 @@ def expected_references_manifest(project): "nodes": ["model.test.ephemeral_summary"], }, "deprecation_date": None, - "defer_relation": None, "description": "A view of the summary of the ephemeral copy of the seed data", "docs": {"node_color": None, "show": True}, "fqn": ["test", "view_summary"], @@ -1145,7 +1138,6 @@ def expected_references_manifest(project): }, }, "config": get_rendered_seed_config(), - "defer_relation": None, "depends_on": {"macros": []}, "description": "The test seed", "docs": {"node_color": None, "show": True}, @@ -1180,7 +1172,6 @@ def expected_references_manifest(project): "config": get_rendered_snapshot_config(target_schema=alternate_schema), "contract": {"checksum": None, "enforced": False, "alias_types": True}, "database": model_database, - "defer_relation": None, "depends_on": {"macros": [], "nodes": ["seed.test.seed"]}, "description": "", "docs": {"node_color": None, "show": True}, @@ -1509,7 +1500,6 @@ def expected_versions_manifest(project): "constraints": [], "sources": [], "depends_on": {"macros": [], "nodes": []}, - "defer_relation": None, "description": "A versioned model", "deprecation_date": ANY, "docs": {"node_color": None, "show": True}, @@ -1580,7 +1570,6 @@ def expected_versions_manifest(project): "contract": {"checksum": None, "enforced": False, "alias_types": True}, "sources": [], "depends_on": {"macros": [], "nodes": []}, - "defer_relation": None, "description": "A versioned model", "deprecation_date": None, "docs": {"node_color": None, "show": True}, @@ -1634,7 +1623,6 @@ def expected_versions_manifest(project): ], }, "deprecation_date": None, - "defer_relation": None, "description": "", "docs": {"node_color": None, "show": True}, "fqn": ["test", "ref_versioned_model"], diff --git a/tests/unit/contracts/graph/test_manifest.py b/tests/unit/contracts/graph/test_manifest.py index c12a5a64e31..a1ff4beec09 100644 --- a/tests/unit/contracts/graph/test_manifest.py +++ b/tests/unit/contracts/graph/test_manifest.py @@ -1045,10 +1045,10 @@ def test_merge_from_artifact(self): # new node added should not be in original manifest assert "model.root.nested2" not in original_manifest.nodes - # old node removed should not have state relation in original manifest + # old node removed should not have defer_relation in original manifest assert original_manifest.nodes["model.root.nested"].defer_relation is None - # for all other nodes, check that state relation is updated + # for all other nodes, check that defer_relation is updated for k, v in original_manifest.nodes.items(): if v.defer_relation: self.assertEqual("other_" + v.database, v.defer_relation.database)