-
Notifications
You must be signed in to change notification settings - Fork 510
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
Replace deprecated adapter methods with relations #133
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{% macro _is_relation(arg, macro) %} | ||
{%- if execute and arg.name is none -%} | ||
{%- do exceptions.raise_compiler_error("Macro " ~ macro ~ " expected a Relation but received the value: " ~ arg) -%} | ||
{%- endif -%} | ||
{% endmacro %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
{% macro nullcheck_table(schema, table) %} | ||
{% macro nullcheck_table(relation) %} | ||
|
||
{% set cols = adapter.get_columns_in_table(schema, table) %} | ||
{%- do dbt_utils._is_relation(relation, 'nullcheck_table') -%} | ||
{% set cols = adapter.get_columns_in_relation(relation) %} | ||
|
||
select {{ dbt_utils.nullcheck(cols) }} | ||
from {{schema}}.{{table}} | ||
from {{relation}} | ||
|
||
{% endmacro %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,14 @@ | ||
{% macro star(from, relation_alias=False, except=[]) -%} | ||
|
||
{%- do dbt_utils._is_relation(from, 'star') -%} | ||
|
||
{#-- Prevent querying of db in parsing mode. This works because this macro does not create any new refs. #} | ||
{%- if not execute -%} | ||
{{ return('') }} | ||
{% endif %} | ||
|
||
{%- if from.name -%} | ||
{%- set schema_name, table_name = from.schema, from.name -%} | ||
{%- else -%} | ||
{%- set schema_name, table_name = (from | string).split(".") -%} | ||
{%- endif -%} | ||
|
||
{%- set include_cols = [] %} | ||
{%- set cols = adapter.get_columns_in_table(schema_name, table_name) -%} | ||
{%- set cols = adapter.get_columns_in_relation(from) -%} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there might be merit to adding a check here that returns an error if a non-relation is provided to this macro (and others). This would have the added benefit of raising an error if someone passes an ephemeral model into Maybe:
or something. We could wrap this up in its own little macro as a helper :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea! We might add this error to all of the macros that now expect relation arguments in a break-y sort of way. Two q's:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are really good questions! I don't think we have a great mechanism in place to assert the types of objects from the jinja context. We could conceivably add a context function or a jinja test (like
Let's be sure to test this with dbt-labs/dbt-core#1416 We also don't have a way to get the macro name from the macro context. I think we should just anticipate making a macro to handle raising the exception, then pass in a string to use as the macro name from the caller. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right on. Is an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah - let's add it to dbt-utils! Maybe name it something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly related to the PR you posted: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, yeah, we'll probably want to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think I'd do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ew, maybe you have to even check the type string:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented the most rigorous logic in a macro
|
||
{%- for col in cols -%} | ||
|
||
{%- if col.column not in except -%} | ||
|
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.
I think this will work great, but there's an important thing to know about python (and therefore jinja, I assume?) argument defaults. Check out this article on mutable default arguments.
This code is A-OK because
target.database
is a string, which is immutable. It would only be a problem if the code read:or similar.