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.

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

Conversation

jeancochrane
Copy link
Contributor

@jeancochrane jeancochrane commented Jun 10, 2024

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

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development, and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc.) or this PR has already received feedback and approval from Product or DX

@jeancochrane jeancochrane requested a review from a team as a code owner June 10, 2024 19:55
Copy link

cla-bot bot commented Jun 10, 2024

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

@QMalcolm
Copy link
Contributor

@cla-bot check

@cla-bot cla-bot bot added the cla:yes label Jun 18, 2024
Copy link

cla-bot bot commented Jun 18, 2024

The cla-bot has been summoned, and re-checked this pull request!

@dbeatty10
Copy link
Contributor

Two places in the product docs that we could update to explain the effect of the alias config for ephemeral models:

@jeancochrane
Copy link
Contributor Author

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.

@dbeatty10
Copy link
Contributor

Thanks for the thorough review @dbeatty10! I think the changeset is now a lot simpler after e5c1b46.

🎉 Nice work @jeancochrane !

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.

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.

@jeancochrane jeancochrane changed the title Use model alias for the CTE identifier generated during ephemeral materialization Use model identifier for the CTE identifier generated during ephemeral materialization Jul 16, 2024
@jeancochrane jeancochrane changed the title Use model identifier for the CTE identifier generated during ephemeral materialization Use model alias for the CTE identifier generated during ephemeral materialization Jul 16, 2024
matthewshaver added a commit to dbt-labs/docs.getdbt.com that referenced this pull request Aug 6, 2024
## 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]>
@ChenyuLInx ChenyuLInx force-pushed the allow-control-over-ephemeral-cte-identifier branch from 8b41bc2 to 4f512d7 Compare August 8, 2024 22:36
Copy link

@ChenyuLInx ChenyuLInx left a 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!

@ChenyuLInx ChenyuLInx enabled auto-merge (squash) August 8, 2024 22:50
@ChenyuLInx ChenyuLInx closed this Aug 8, 2024
auto-merge was automatically disabled August 8, 2024 22:50

Pull request was closed

@ChenyuLInx ChenyuLInx reopened this Aug 8, 2024
@ChenyuLInx
Copy link

Closing and reopening PR to make sure all needed checks are running

@ChenyuLInx ChenyuLInx enabled auto-merge (squash) August 8, 2024 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-665] [Feature] Allow control over the outer CTE identifier generated in ephemeral materialization
5 participants