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

[CT-2581] [Feature] Allow for more flexible drop statements in drop_relation() #7625

Closed
3 tasks done
mikealfare opened this issue May 15, 2023 · 0 comments · Fixed by #7626
Closed
3 tasks done

[CT-2581] [Feature] Allow for more flexible drop statements in drop_relation() #7625

mikealfare opened this issue May 15, 2023 · 0 comments · Fixed by #7626
Assignees
Labels
enhancement New feature or request

Comments

@mikealfare
Copy link
Contributor

Is this your first time submitting a feature request?

  • I have read the expectations for open source contributors
  • I have searched the existing issues, and I could not find an existing issue for this feature
  • I am requesting a straightforward extension of existing dbt functionality, rather than a Big Idea better suited to a discussion

Describe the feature

The current implementation of drop_relation() looks like this:

{% macro drop_relation(relation) -%}
    {{ return(adapter.dispatch('drop_relation', 'dbt')(relation)) }}
{% endmacro %}

{% macro default__drop_relation(relation) -%}
    {% call statement('drop_relation', auto_begin=False) -%}
        drop {{ relation.type }} if exists {{ relation }} cascade
    {%- endcall %}
{% endmacro %}

That forces a specific string for relation.type. In particular, it forces spaces in relation.type for some scenarios (e.g. materialized views) and it forces the dbt implementation of a model to match that of the underlying database. The latter is an issue if, as an example, dbt decides to implement "dbt materialized views" in dbt-snowflake by using dynamic tables. The issue in the end is that relation.type is playing two roles that can't always be guaranteed to overlap.

The proposal is to turn drop_relation() into a dispatch function and create functions such as drop_table(), drop_materialized_view(), etc. There should still be a default condition in drop_relation() in order to preserve backwards compatibility. The proposal would look something like this:

{% macro drop_relation(relation) -%}
    {{ return(adapter.dispatch('drop_relation', 'dbt')(relation)) }}
{% endmacro %}

{% macro default__drop_relation(relation) -%}
    {% call statement('drop_relation', auto_begin=False) -%}
        {%- if relation.type == 'table' -%}
            {{- drop_table(relation) -}}
        {%- elif relation.type == 'materialized_view' -%}
            {{- drop_materialized_view(relation) -}}
        {%- else -%}
            drop {{ relation.type }} if exists {{ relation }} cascade
        {%- endif -%}
    {%- endcall %}
{% endmacro %}


{% macro drop_table(relation) -%}
  {{ return(adapter.dispatch('drop_table', 'dbt')(relation)) }}
{% endmacro %}

{% macro default__drop_table(relation) -%}
    drop table if exists {{ relation }} cascade
{% endmacro %}


{% macro drop_materialized_view(relation) -%}
  {{ return(adapter.dispatch('drop_materialized_view', 'dbt')(relation)) }}
{% endmacro %}

{% macro default__drop_materialized_view(relation) -%}
    drop materialized view if exists {{ relation }} cascade
{% endmacro %}

Describe alternatives you've considered

An alternative would be to keep the current approach. This would mean creating dbt materializations that match the underlying database's implementation. In this scenario, there would be no materialized view implementation for dbt-snowflake because we implement that concept via dynamic tables. Likewise, there would be no dynamic table implementation for any other adapter besides dbt-snowflake.

Another alternative is to overwrite drop_relation() for each adapter where necessary. That would address materialized view / dynamic table scenario since we could check for that in the dbt-snowflake adapter. But that would encourage overwriting drop_relation() in general, which is a central macro (it gets used in almost every run). The goal of the proposal above is to allow adapter maintainers to configure how an object is dropped without the need to worry about dbt-level functionality (e.g. logging, how to implement call statement, etc.).

Who will this benefit?

This will benefit adapter maintainers and dbt-core maintainers. Adapter maintainers can be more flexible with how they manage the drop logic while also being less concerned with breaking dbt-core logic (or losing logic added in the future). dbt-core maintainers can be more confident that any logic added in the future will be implemented while still allowing adapter maintainers a path for configuration.

Are you interested in contributing this feature?

Yes

Anything else?

No response

@mikealfare mikealfare added enhancement New feature or request triage labels May 15, 2023
@mikealfare mikealfare self-assigned this May 15, 2023
@github-actions github-actions bot changed the title [Feature] Allow for more flexible drop statements in drop_relation() [CT-2581] [Feature] Allow for more flexible drop statements in drop_relation() May 15, 2023
@dbeatty10 dbeatty10 removed the triage label Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants