Er/3899 models key mismatch #3959
Conversation
emmyoop
force-pushed
the
er/3899-models-key-mismatch
branch
from
a68b5d7 to
26a30a4
Compare
emmyoop
force-pushed
the
er/3899-models-key-mismatch
branch
from
26a30a4 to
8ee43df
Compare
emmyoop
force-pushed
the
er/3899-models-key-mismatch
branch
from
8ee43df to
7b16b88
Compare
core/dbt/contracts/graph/manifest.py
Outdated
| self._disabled[node.unique_id].append(node) | ||
| else: | ||
| self._disabled[node.unique_id] = [node] | ||
| self.disabled.append(node) |
There was a problem hiding this comment.
What's changed here? Are we no longer patching properties for disabled nodes?
There was a problem hiding this comment.
I don't think we've ever patched disabled nodes. That's the way that it worked when I first saw it anyway.
There was a problem hiding this comment.
I'm not sure that feels right though. I woud have no objection to patching disabled nodes. They do come out in the manifest.json.
There was a problem hiding this comment.
But a lookup won't find them (since they're not included there), so we'd have to separately look them up in disabled.
| @@ -567,7 +567,6 @@ class Manifest(MacroMethods, DataClassMessagePackMixin, dbtClassMixin): | |||
| # Moved from the ParseResult object | |||
| source_patches: MutableMapping[SourceKey, SourcePatch] = field(default_factory=dict) | |||
| # following is from ParseResult | |||
| _disabled: MutableMapping[str, List[CompileResultNode]] = field(default_factory=dict) | |||
There was a problem hiding this comment.
I think we should remove the list form of disabled, and switch to a dictionary (MutableMapping) form. (I'm not sure why this one says [str, List..]. I don't see why it would ever be an actual list, there should only be one node per unique_id). That's more in keeping with the other stored nodes ('nodes', 'macros', etc). That would involve remove the current 'disabled' from the Manifest, and renaming '_disabled' to 'disabled'. (And fixing the definition.) So in the places where a node is added to the disabled list, you'd need to add it to the dictionary.
There was a problem hiding this comment.
@jtcohen6 Disabled does show up in manifest.json, so this would be a manifest schema change. Do you agree with what I propose here? If we're going to do it, for 1.0 is the right time.
There was a problem hiding this comment.
Your proposal makes sense to me! We should feel well within our rights to make these kind of schema changes ahead of 1.0
core/dbt/contracts/graph/manifest.py
Outdated
| self._disabled[node.unique_id].append(node) | ||
| else: | ||
| self._disabled[node.unique_id] = [node] | ||
| self.disabled.append(node) |
There was a problem hiding this comment.
I don't think we've ever patched disabled nodes. That's the way that it worked when I first saw it anyway.
… overlooked arg order
emmyoop
force-pushed
the
er/3899-models-key-mismatch
branch
from
4f5006f to
332d23c
Compare
resolves # 3899
Description
Give a warning when a node is specified under the wrong type in
schema.ymland don't continue processing.Does this still need to be handled - I couldn't produce the behavior: Throw an exception for any model/seed/snapshot that has a
unique_idofNoneand is not disabled.As part of this change
manifest._disabledwas also removed in favor of the existingmanifest.disabled.Checklist
CHANGELOG.mdand added information about my change to the "dbt next" section.