Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
dbt clone
#7881dbt clone
#7881Changes from 9 commits
3056eda
f63c379
afdada9
23b4113
dafb250
b78e837
31870f9
692e542
69a3dbd
125fb0b
b5dde06
5303346
c936856
2b0eb25
95758d1
bcecced
7327283
0543303
ed66dc0
5aa4209
7eace73
65eaa0a
4bce05f
824a2fc
6a8ecb0
d4f2eca
5be28bc
18ad7ca
6efe6e3
48e01e5
81dad80
d3d3014
88a726b
020cde5
e761aa0
4eb7aa4
cd78b64
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The way that this update works is to overwrite the jinja context so that when we execute
materialization_macro
later on will be running with the updated context to actually create the view.This logic is pretty hidden, can we spend a bit more time thinking about how to avoid changing context in macro space? @gshank Any suggestion on whether it is possible to do this in a better way?
I did a quick search in our codebase and it seems that we are creating a new pattern here. If this is the only way we can do it but we should add comment and call out this is not a pattern we would want to do in the codebase.
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.
Good call-out, I'll discuss this with @gshank in our 1:1 tomorrow.
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.
This is definitely the bit I felt worst about in #7258.
I opened an issue to track real dispatch support for materialization macros: #7799. Until we do that, this will work much of the time, but it won't support adapters that inherit from other adapters. E.g.
dbt-postgres
→dbt-redshift
, ergo Redshift is allowed to usepostgres__
implementations.As far as hard-overwriting the context values for
sql
+compiled_code
: Gross, right? The other potential approach I can think of is, making materialization macros acceptkwargs
, so thatsql
/compiled_code
can be passed in explicitly rather than set + pulled out of theaethercontext.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.
TIL, wow
Yes, but I've seen worse
This makes sense, we can pass in a
dict
for now before migrating to a typed object (similar todbtRunner
)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 break down the questions here a bit:
clone
potentially have a more restrictive assumption than the actualview
materialization, for example: always create a view that points to the other table no matter what's going on? Looks like there's not as much going on in view as other materialization, sometimes it's better to repeat simple stuff again instead of reuseThere 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'll defer to @dbt-labs/core-adapters on 1, I'm fine with either approach.
@McKnight-42: question for you when you get back.
For 2, I don't know the answer, but can sync with @McKnight-42 and reply here.
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.
Good breakdown of the questions @ChenyuLInx!
Thinking through the logic we'd want from the
view
materialization:create or replace view
versus create temp + alter/rename/swapSo - I would argue that's the right way to go. I'm definitely interested in a world where materializations are more composable than they are currently, so that more-complex build strategies ("clone an object from one env into another") can call into simpler ones ("make a view").
Macros are already pretty composable. Materializations are just macros, but they pull all their inputs from this node's g-d context, rather than receiving them as explicit arguments.
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.
Answers to @ChenyuLInx questions from offline chat with @McKnight-42:
I believe so, since different adapters implement views differently.
I previously believed that all adapters share a single view materialization, but that doesn't seem to be the case.
I don't know.
In theory, it should do the same thing as Postgres (create views) since neither support zero-copy cloning.
I'll have to verify what happens in practice.