-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
model versions #7287
model versions #7287
Conversation
source_file.append_patch( | ||
versioned_model_patch.yaml_key, versioned_model_node.unique_id | ||
) | ||
self.manifest.rebuild_ref_lookup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: look into whether this can be more granular and moved into the loop for a performance improvement
@@ -405,6 +427,11 @@ def patch(self, patch: "ParsedNodePatch"): | |||
self.created_at = time.time() | |||
self.description = patch.description | |||
self.columns = patch.columns | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider moving this to parse_patch code on PatchParser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good! I just had one question about "build_model_str"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a small partial parsing bug, and you just fixed it.
That's ... really it! I've done my best to break this, and it's been holding up well. A handful of comments, none of them a blocker to merging.
Nice work :)
core/dbt/include/global_project/macros/get_custom_name/get_custom_alias.sql
Show resolved
Hide resolved
core/dbt/contracts/graph/nodes.py
Outdated
# TODO: version, is_latest_version, and access are specific to ModelNodes, consider splitting out to ModelNode | ||
if self.resource_type != NodeType.Model: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would we expect these validation warnings to come up in practice (if ever)? e.g. I've tried adding versions
/ version
attributes to a seed
patch, and I don't see any warnings
b5ba176
to
5ca3bd0
Compare
cffd825
to
1b33416
Compare
Added support for ref resolution for python models in this commit (thank you @jtcohen6!!). Updated adapter tests as well, and verified those work in Snowflake (BigQuery skips these tests, Spark CI is broken - but there is no adapter-specific code so this should work across all adapters if it works for Snowflake) |
Latest change adds a |
resolves #3194 [Page preview](https://deploy-preview-3195--docs-getdbt-com.netlify.app/docs/collaborate/govern/model-versions#how-to-create-a-new-version-of-a-model) ## What are you changing in this pull request and why? Update code examples for `defined_in` and `exclude` to align with the implementation (and code examples) in [dbt-labs/dbt-core#7287](dbt-labs/dbt-core#7287): - e27c2da Example using `defined_in` property - 9da3e00 Fix the example to `exclude` column(s) - d0f5396 Use postgres-specific data types (since it is a neutral database platform and its data types can be easily ported) ## Checklist - [x] Review the [Content style guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md) and [About versioning](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#adding-a-new-version) so my content adheres to these guidelines. - [x] Add a checklist item for anything that needs to happen before this PR is merged, such as "needs technical review" or "change base branch."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some un-submitted review comments relating to changie 😅
time: 2023-04-06T10:10:19.794672-04:00 | ||
custom: | ||
Author: michelleark | ||
Issue: '#7263' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Issue: '#7263'
work?
Or does it need to be like the following?
Issue: '#7263' | |
Issue: "7263" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To close the loop, Issue: '#7263'
didn't work:
- https://github.com/dbt-labs/dbt-core/blob/1.5.latest/CHANGELOG.md
Line 47 in dee5e70
- model versions ([##7263](https://github.com/dbt-labs/dbt-core/issues/#7263))
resolves #7263
Description
ModelPatchParser
andUnparsedModelUpdate
for model-specific parsing. Also introducesVersionedTestBlock
.ParseResult
instead of a list of test blocks from base parserparse
methodList[List[str]]
toList[RefArgs]
TODO
Checklist
changie new
to create a changelog entry🎩
schema.yml:
models/dim_customers_v2.sql
models/arbitrary_file_name.sql
models/test_version.sql