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 all adapter names for adapter.dispatch #2923

Closed
jtcohen6 opened this issue Dec 1, 2020 · 2 comments · Fixed by #3296
Closed

Use all adapter names for adapter.dispatch #2923

jtcohen6 opened this issue Dec 1, 2020 · 2 comments · Fixed by #3296
Labels
adapter_plugins Issues relating to third-party adapter plugins enhancement New feature or request
Milestone

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Dec 1, 2020

Describe the feature

The dbt-redshift adapter plugin inherits from the dbt-postgres adapter plugin. This reflects the fact that they share a lot of code: connection mechanism (psycopg2), basic syntax, types, materializations, etc. (This can be obfuscated by the fact that their shared implementations of macros are also often the default__ dbt implementations.)

Today, when dbt dispatches package_name.macro_name macro on Redshift, it looks for:

  1. my_project.redshift__macro_name
  2. my_project.redshift__macro_name
  3. package_name.postgres__concat
  4. package_name.postgres__concat

I think it should instead look for:

  1. my_project.redshift__macro_name
  2. my_project.postgres__macro_name
  3. my_project.default__macro_name
  4. package_name.redshift__macro_name
  5. package_name.postgres__macro_name
  6. package_name.default__macro_name

This is a possibility we weighed in #2679, to the point that we make it possible as a one-line change:
https://github.com/fishtown-analytics/dbt/blob/5ba5271da99bc1ef7fbad2f7d0b45634087300fa/core/dbt/context/providers.py#L104-L109

What's the risk?

There are cases today where Redshift uses the default__ implementation and Postgres uses a custom postgres__ implementation, e.g. dbt_utils.dateadd.

I don't think this is a breaking change per se, because Redshift largely supports PostgreSQL syntax, but we may want to add implementations that explicitly preserve existing behavior:

{% macro redshift__dateadd(datepart, interval, from_date_or_timestamp) %}
    {% do return(default__dateadd(datepart, interval, from_date_or_timestamp)) %}
{% endmacro %}

Who will this benefit?

This will enable maintainers of similar-yet-slightly-different plugins, e.g. dbt-sqlserver and dbt-synapse, who can then "share" macro implementations between parent and child adapters. See microsoft/dbt-synapse#18.

It's possible to imagine many more adapter inheritances in the future:

  • a dedicated dbt-databricks or dbt-delta that inherits from dbt-spark
  • a dbt-hive that inherits from dbt-spark or dbt-presto
  • a dbt-materialize that inherits from dbt-postgres
@dataders
Copy link
Contributor

dataders commented Apr 5, 2021

friendly, curious bump. is this still something in consideration for Margaret Mead. Relatedly, I just peeped MaterializeInc/materialize-dbt-utils. Pretty cool to see folks catching dispatch fever!

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Apr 5, 2021

Yes, this is still something I'd like to do for Margaret Mead! The change itself is very straightforward. We'll want to add some tests for it, and it will likely require a few changes in packages, especially dbt-utils.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapter_plugins Issues relating to third-party adapter plugins enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants