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

Feature: use adapter.dispatch #267

Merged
merged 8 commits into from
Aug 21, 2020
Merged

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Aug 15, 2020

This is a:

  • bug fix PR with no breaking changes (please change the base branch to main)
  • new functionality
  • a breaking change

Description & motivation

  • Replace all instances of adapter_macro with adapter.dispatch
    • Define a variable, dbt_utils_dispatch_list, whereby end users can provide a list of other package namespaces that will dispatch to before dbt_utils, whenever a dbt_utils macro is called
  • Also: slight modification to _is_ephemeral to support plugins with custom ephemeral prefixes (see Feature: adapter cte generation dbt-core#2712)
  • While doing that, get tests passing
    • date_spine on postgres
    • refactor get_relations_by_prefix and get_relations_by_pattern since there was a lot of duplicated code, and the tests for the latter were failing on BQ
  • We won't be able to run automated tests until v0.18.0rc1 is on pypi

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to the changelog

@clrcrl clrcrl self-requested a review August 17, 2020 13:25
@clrcrl
Copy link
Contributor

clrcrl commented Aug 17, 2020

refactor get_relations_by_prefix and get_relations_by_pattern since there was a lot of duplicated code, and the tests for the latter were failing on BQ

Thank you! I also think we should fully deprecate get_tables_by_prefix as we've had a warning up for a few versions now. That can be a separate PR though

@jtcohen6
Copy link
Contributor Author

Thank you! I also think we should fully deprecate get_tables_by_prefix as we've had a warning up for a few versions now. That can be a separate PR though

Agree! I think there's a number of older deprecation warnings that we could actually deprecate soon. We've talked about doing this for dbt deprecations in v0.19 or v0.20.

@jtcohen6 jtcohen6 marked this pull request as ready for review August 20, 2020 17:51
Copy link
Contributor

@clrcrl clrcrl left a comment

Choose a reason for hiding this comment

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

This all LGTM.

My only thoughts are:

  • Do we want to document how to override a macro? I'm still not fully sure who wants to use this behavior, but it must be supported for a reason, right?
  • Do we want to add something to the contributing section of the readme about adapter macros? or nah? We probably need to write better docs on contributing that cover testing / what we do and don't support etc, so maybe that's part of a bigger project.
  • Also, pls update changelog.md :D

macros/cross_db_utils/_is_ephemeral.sql Show resolved Hide resolved
@clrcrl clrcrl mentioned this pull request Aug 20, 2020
13 tasks
@jtcohen6
Copy link
Contributor Author

  • Added a note to the README below contributing. This is tricky stuff that is of huge benefit to the few dozen people who really need it, and totally irrelevant to everyone else. I assume that contributors who create future dispatched macros will just copy-paste from the existing ones.
  • Added to changelog!

@jtcohen6 jtcohen6 merged commit 5f396cc into dev/0.6.0 Aug 21, 2020
@jtcohen6 jtcohen6 deleted the feature/0-18-adapter-dispatch branch August 21, 2020 16:05
Copy link
Contributor

@clrcrl clrcrl left a comment

Choose a reason for hiding this comment

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

I forgot to submit the comment lol

{%- do exceptions.warn(error_message) -%}

{{ return(dbt_utils.get_relations_by_pattern(schema_pattern, table_pattern, exclude='', database=target.database)) }}
Copy link
Contributor

@clrcrl clrcrl Aug 20, 2020

Choose a reason for hiding this comment

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

So this was some bad code that I merged previously. However, I think this is a breaking change, the previous get_tables_by_pattern returned SQL (here)

Suggested change
{{ return(dbt_utils.get_relations_by_pattern(schema_pattern, table_pattern, exclude='', database=target.database)) }}
{{ return(dbt_utils.get_tables_by_pattern_sql(schema_pattern, table_pattern, exclude='', database=target.database)) }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that:

  • the only place that the get_tables_by_prefix macro was used is in get_relations_by_pattern macro
  • AND, I doubt anyone was using get_tables_by_prefix independently of get_relations_by_pattern

I'd be happy to just straight-out remove the get_tables_by_prefix macro

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to remove it, we can do so in #268

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah let's do it

@jtcohen6 jtcohen6 mentioned this pull request Sep 4, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants