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-1829] [Bug] [v1.4.0rc1] Back compat in get_merge_sql macro signature #6625

Closed
jtcohen6 opened this issue Jan 17, 2023 · 0 comments · Fixed by #6628
Closed

[CT-1829] [Bug] [v1.4.0rc1] Back compat in get_merge_sql macro signature #6625

jtcohen6 opened this issue Jan 17, 2023 · 0 comments · Fixed by #6628
Assignees
Labels
bug Something isn't working Team:Adapters Issues designated for the adapter area of the code
Milestone

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jan 17, 2023

Prompted by Slack thread

Context

In #5702, we changed the signature of the get_merge_sql() macro. This could have implications for users who call get_merge_sql() directly from a custom macro/materialization.

In main / 1.4.latest:

{% macro get_merge_sql(target, source, unique_key, dest_columns, incremental_predicates) -%}
{{ adapter.dispatch('get_merge_sql', 'dbt')(target, source, unique_key, dest_columns, incremental_predicates) }}
{%- endmacro %}
{% macro default__get_merge_sql(target, source, unique_key, dest_columns, incremental_predicates) -%}

In 1.3.latest:

{% macro get_merge_sql(target, source, unique_key, dest_columns, predicates=none) -%}
{{ adapter.dispatch('get_merge_sql', 'dbt')(target, source, unique_key, dest_columns, predicates) }}
{%- endmacro %}
{% macro default__get_merge_sql(target, source, unique_key, dest_columns, predicates) -%}

Repro case

This means that if someone has custom code like the following, they'll start seeing a compilation error in v1.4. (Yes, these are very silly examples! No one should ever call this macro like this.)

-- missing 5th position arg
{{ get_merge_sql(this, this, 'id', ['a', 'b']) }}
Compilation Error in model some_model (models/some_model.sql)
  parameter 'incremental_predicates' was not provided

  > in macro get_merge_sql (macros/materializations/models/incremental/merge.sql)
  > called by model some_model (models/some_model.sql)
-- using old kwarg name
{{ get_merge_sql(this, this, 'id', ['a', 'b'], predicates=['blabla']) }}
Compilation Error in model some_model (models/some_model.sql)
  macro 'dbt_macro__get_merge_sql' takes no keyword argument 'predicates'

Resolution

I think we can provide backwards compatibility quite simply by:

  • switching incremental_predicates back to optional, by providing a default value of None
  • detecting if predicates is passed as a kwarg, and passing it into incremental_predicates if so
 {% macro get_merge_sql(target, source, unique_key, dest_columns, incremental_predicates=None) -%} 
   -- back compat for old kwarg name
   {% set incremental_predicates = kwargs.get('predicates', incremental_predicates) %}
   {{ adapter.dispatch('get_merge_sql', 'dbt')(target, source, unique_key, dest_columns, incremental_predicates) }} 
 {%- endmacro %} 

With that change, both of the silly code examples above start working again.

(If someone passes both predicates and incremental_predicates as keyword arguments, that's on them.)

@jtcohen6 jtcohen6 added bug Something isn't working Team:Adapters Issues designated for the adapter area of the code labels Jan 17, 2023
@jtcohen6 jtcohen6 added this to the v1.4 milestone Jan 17, 2023
@github-actions github-actions bot changed the title [v1.4] Back compat in get_merge_sql macro signature [CT-1829] [v1.4] Back compat in get_merge_sql macro signature Jan 17, 2023
@jtcohen6 jtcohen6 changed the title [CT-1829] [v1.4] Back compat in get_merge_sql macro signature [CT-1829] [Bug] [v1.4.0rc1] Back compat in get_merge_sql macro signature Jan 17, 2023
@dave-connors-3 dave-connors-3 self-assigned this Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants