-
Notifications
You must be signed in to change notification settings - Fork 40
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
Use model alias for the CTE identifier generated during ephemeral materialization #236
Use model alias for the CTE identifier generated during ephemeral materialization #236
Conversation
Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA. In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR. CLA has not been signed by users: @jeancochrane |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
Two places in the product docs that we could update to explain the effect of the |
Thanks for the thorough review @dbeatty10! I think the changeset is now a lot simpler after e5c1b46. Would you like me to make a docs PR to tweak the sections you mentioned above? I'd be happy to do so, but am also down to leave it to your team if you'd prefer to have editorial control over this one. |
🎉 Nice work @jeancochrane !
That would be awesome if you are willing to raise a PR in docs.getdbt.com. If you tag me in it, I'll give a review along with one of my colleagues on the documentation team. |
## What are you changing in this pull request and why? This PR updates the docs to reflect the bugfix made in dbt-labs/dbt-core#10290 and dbt-labs/dbt-adapters#236 to bring the identifiers used for CTEs in line with the identifiers used for tables and views.⚠️ Note that these changes should **not** be deployed until those two PRs have been merged and released, since these docs will not accurately reflect the behavior of the packages until they have been patched. I've included this prerequisite in the checklist below. ## Checklist - [x] Review the [Content style guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md) so my content adheres to these guidelines. - [x] For [docs versioning](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#about-versioning), review how to [version a whole page](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#adding-a-new-version) and [version a block of content](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#versioning-blocks-of-content). - [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." - [ ] Merge and release dbt-labs/dbt-core#10290 and dbt-labs/dbt-adapters#236 --------- Co-authored-by: Matt Shaver <[email protected]>
8b41bc2
to
4f512d7
Compare
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.
Thanks @jeancochrane for the thoughtful PR!
This change looks good to me!
Closing and reopening PR to make sure all needed checks are running |
Partially resolves dbt-labs/dbt-core#5273. Reimplementation of dbt-labs/dbt-core#5488 to match the new split between adapter and core repos.
Problem
Ephemeral model materializations do not currently support using special characters like dots in file names, even if these special characters are sanitized using model aliases.
Solution
Prioritize using the model alias (if specified) via
model.identifier
when creating a node identifier for ephemeral models.This change requires a corresponding change to
dbt-core
to fix the bug in the special case of unit testing, which will be implemented in a separate PR.Checklist