Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "other" relation to reffable node classes #7645

Merged
merged 4 commits into from
May 17, 2023
Merged

Add "other" relation to reffable node classes #7645

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
777382a
Add "other" relation to reffable node classes
stu-k May 15, 2023
695b74e
Remove key from context keys
stu-k May 16, 2023
6607b69
Create and use StateRelation class
stu-k May 17, 2023
e988ec6
Add identifier method back
stu-k May 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Under the Hood-20230515-152107.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Under the Hood
body: Add other relation to reffable nodes
time: 2023-05-15T15:21:07.808892-05:00
custom:
Author: stu-k
Issue: "7550"
2 changes: 2 additions & 0 deletions core/dbt/contracts/graph/manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1316,6 +1316,8 @@ 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 "state_relation" in node:
del node["state_relation"]
return dct


Expand Down
13 changes: 10 additions & 3 deletions core/dbt/contracts/graph/nodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,16 +271,20 @@ def add_public_node(self, value: str):


@dataclass
class ParsedNodeMandatory(GraphNode, HasRelationMetadata, Replaceable):
class RelationalNode(HasRelationMetadata):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's confusing to use Node in the name here. It's not a Node. These classes are already complicated enough, I don't think it's necessarily a good idea to reuse the existing classes in nodes.py. You're actually reimplementing a Relation class. Do you actually need the separate database/schema/identifier? ("identifier" is what it's actually called in the Relation classes, and makes more sent to me, at least.). I'm not exactly sure how this is going to be used. Would a relation_name (clone_relation_name?) field work?

alias: str
checksum: FileHash
config: NodeConfig = field(default_factory=NodeConfig)

@property
def identifier(self):
return self.alias


@dataclass
class ParsedNodeMandatory(GraphNode, RelationalNode, Replaceable):
checksum: FileHash
config: NodeConfig = field(default_factory=NodeConfig)


# This needs to be in all ManifestNodes and also in SourceDefinition,
# because of "source freshness"
@dataclass
Expand Down Expand Up @@ -615,6 +619,7 @@ class ModelNode(CompiledNode):
constraints: List[ModelLevelConstraint] = field(default_factory=list)
version: Optional[NodeVersion] = None
latest_version: Optional[NodeVersion] = None
state_relation: Optional[RelationalNode] = None
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the PR description, state_relation is a very general name that doesn't explain much of what this field holds. Please feel free to suggest better names for this attribute.

Copy link
Contributor Author

@stu-k stu-k May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe other_relation, target_relation, cross_environment_relation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke with Gerda about this name and she thought it made sense for the context it was in.


@property
def is_latest_version(self) -> bool:
Expand Down Expand Up @@ -797,6 +802,7 @@ class SeedNode(ParsedNode): # No SQLDefaults!
# and we need the root_path to load the seed later
root_path: Optional[str] = None
depends_on: MacroDependsOn = field(default_factory=MacroDependsOn)
state_relation: Optional[RelationalNode] = None

def same_seeds(self, other: "SeedNode") -> bool:
# for seeds, we check the hashes. If the hashes are different types,
Expand Down Expand Up @@ -995,6 +1001,7 @@ class IntermediateSnapshotNode(CompiledNode):
class SnapshotNode(CompiledNode):
resource_type: NodeType = field(metadata={"restrict": [NodeType.Snapshot]})
config: SnapshotConfig
state_relation: Optional[RelationalNode] = None


# ====================================
Expand Down
1 change: 1 addition & 0 deletions test/unit/test_manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
"version",
"latest_version",
"constraints",
"state_relation",
}
)

Expand Down