Create adapter.dispatch #2679
Create adapter.dispatch #2679
Conversation
Added tests Added deprecation warning + tests
add more tests
There was a problem hiding this comment.
This works as expected, and it returns sensible messages when I do things like:
- use
adapter_macroas before (with deprecation warning) - use
adapter_macrowith no adequate implementation (missingdefault__) - use
adapter.dispatchwith no adequate implementation (missingdefault__) - lazily try to swap
adapter_macro('my_package.my_macro')-->adapter.dispatch('my_package.my_macro')instead of realizing I should useadapter.dispatch('my_macro', packages = ['my_package'])- post-realization, I first instinctively wrote
adapter.dispatch('my_macro', packages = 'my_package'), which dbt coerced to[m,y,_,p,a,c,k,a,g,e]. I think that's ok. I'll call out thatpackagesmust be a list in documentation, and the error returned is pretty obvious.
- post-realization, I first instinctively wrote
I went so far as to play around with "overriding" dbt_utils macros from a separate package by passing a var into the packages arg of dispatch, as outlined in #2302. It's a bit finicky only because of how vars work with packages. Most importantly, it works.
core/dbt/context/providers.py
Outdated
| except CompilationException as exc: | ||
| raise CompilationException( | ||
| f'In adapter_macro: {exc.msg}\n' | ||
| f" Original name: '{name}'", |
There was a problem hiding this comment.
Could we make this the full original macro name, including the package namespace if it included a .?
| attempts = [] | ||
|
|
||
| for package_name in search_packages: | ||
| for prefix in self._get_adapter_macro_prefixes(): |
There was a problem hiding this comment.
Am I right in understanding that the order of preference is packages > adapter specificity? So e.g. a dispatch with two packages running on Postgres would look for:
packages[0],postgres_packages[0],default__packages[1],postgres__packages[1],default__
Per the comment above, someday we may make it so that running on Redshift looks for:
packages[0],redshift__packages[0],postgres__packages[0],default__packages[1],redshift__
...
There was a problem hiding this comment.
Yes. We can of course swap this order if we want, but it seemed more correct to me to go by package first - looking at the example in #2302, it seems that you'd want your custom macro to tell dbt-utils "search this other package first and then fall back to dbt-utils if it's not there". This provides a nice escape hatch for issues where "dbt-utils has an implementation of a macro that doesn't work for me because of my obscure workflow". Or even just a bug!
There was a problem hiding this comment.
Agree! Give first priority to the place where end users have the most control, let them override default__ or do anything they want. Just wanted to make sure I had this down before writing docs
I think we should probably make sure the parameter isn't a list. This is an easy mistake to make and python doesn't have the decency to throw a type error. |
resolves #2302 (the rest of it)
Description
This is a follow-up to #2675.
This PR adds the
adapter.dispatchdescribed in #2302, and markedadapter_macroas deprecated. This is a pretty straightforward PR. The dispatch method happens on the "database wrapper" rather than the actual adapter, as adapters themselves don't know about manifests/namespacing. The adapter_macro context method now calls into dispatch.I had to massage some of the error messaging, but I think it's ok.
A large part of this PR is just converting
adapter_macro(foo, *args, **kwargs)calls intoadapter.dispatch(foo)(*args, **kwargs).Checklist
CHANGELOG.mdand added information about my change to the "dbt next" section.