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

Port the adapter macro to python #2675

Merged
merged 5 commits into from
Aug 3, 2020

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Jul 31, 2020

resolves part of #2302

Description

This ports adapter_macro to python, which involved some cleanup of the node selection logic.

I split the macros and manifest+query header contexts out from the configured context as that file was getting too busy.

I made no change to actual behavior, so the real test here is that all the existing integration tests pass.

I added a get_adapter_type_names to the adapter factory to handle an inheritance-style lookup, but that looked like it broke things. I rolled that back but would like to keep the adapter factory changes as I think they're probably for the best.

In the future, we should consider changing it so redshift automatically tries the postgres version of a macro when there's no redshift version before falling back to the default, but let's keep the breaking changes to a minimum for now.

A future PR will implement the dispatch name and additional arguments.

Checklist

  • I have signed the CLA
  • 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
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Jul 31, 2020
@beckjake beckjake marked this pull request as ready for review July 31, 2020 18:55
@beckjake beckjake requested review from kwigley and jtcohen6 and removed request for kwigley July 31, 2020 18:55
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I ran the dbt-utils integration tests with this branch, and all went according to plan. My only comment is around the error message returned when adapter_macro fails to find its match.

core/dbt/context/providers.py Outdated Show resolved Hide resolved
core/dbt/context/providers.py Outdated Show resolved Hide resolved
@beckjake beckjake requested a review from jtcohen6 August 3, 2020 15:30
@beckjake beckjake merged commit ba828e5 into dev/marian-anderson Aug 3, 2020
@beckjake beckjake deleted the feature/python-adapter-macro branch August 3, 2020 15:50
@beckjake beckjake mentioned this pull request Aug 3, 2020
4 tasks
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.

3 participants