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/support incremental predicates #5702

Merged
merged 15 commits into from
Dec 14, 2022

Conversation

dave-connors-3
Copy link
Contributor

@dave-connors-3 dave-connors-3 commented Aug 23, 2022

resolves #5680

Description

This PR incorporates the feedback from @NiallRees in #5679 and builds on it for the delete+insert incremental strategy. There's not a ton of functional difference between the two PRs:

  • small difference in handling parentheses
  • I made some argument changes to be a bit more consistent with naming and how incremental_predicates gets passed through the arg_dict
  • added changes for get_delete_insert_merge_sql for predicates in the style described by Niall

In conjunction with this PR, i added support across the other adapters that use merge or delete+insert:

UPDATE - opened new PR for Snowflake adapter tests

Would love some guidance on how to coordinate these changes together (esp Databricks, since it's a forked repo!)

I made a new PR simply becuase I had the branch in motion -- I recognize this is probably to most annoying way to do this given how many versions of this PR there have been, and the fact that it's duplicative to Niall's -- many apologies! Happy to coordinate and consolidate however the team sees fit.

Checklist

@dave-connors-3 dave-connors-3 requested a review from a team August 23, 2022 13:49
@dave-connors-3 dave-connors-3 requested a review from a team as a code owner August 23, 2022 13:49
@cla-bot cla-bot bot added the cla:yes label Aug 23, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@dave-connors-3 dave-connors-3 requested a review from a team as a code owner August 23, 2022 13:57
@jtcohen6 jtcohen6 added the Team:Adapters Issues designated for the adapter area of the code label Sep 6, 2022
@NiallRees
Copy link
Contributor

Pls pls pls can we get this into the next minor release? For some reason I thought it would be released in 1.3.0 until I just checked!

@NiallRees
Copy link
Contributor

For those who want this now, all you have to do to get the incremental_predicates config working is add this macro override:

{% macro default__get_incremental_merge_sql(arg_dict) %}

  {% do return(get_merge_sql(arg_dict["target_relation"], arg_dict["temp_relation"], arg_dict["unique_key"], arg_dict["dest_columns"], arg_dict["predicates"])) %}

{% endmacro %}

@leahwicz leahwicz requested review from nathaniel-may and removed request for a team and stu-k October 13, 2022 12:23
@@ -21,7 +21,7 @@

{% macro default__get_incremental_delete_insert_sql(arg_dict) %}

{% do return(get_delete_insert_merge_sql(arg_dict["target_relation"], arg_dict["temp_relation"], arg_dict["unique_key"], arg_dict["dest_columns"])) %}
{% do return(get_delete_insert_merge_sql(arg_dict["target_relation"], arg_dict["temp_relation"], arg_dict["unique_key"], arg_dict["dest_columns"], arg_dict["incremental_predicates"])) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

One thought is that we could maybe support both, borrowing from @NiallRees :

{% macro default__get_incremental_merge_sql(arg_dict) %}
  {% if "predicates" in arg_dict %}
    {% do return(get_merge_sql(arg_dict["target_relation"], arg_dict["temp_relation"], arg_dict["unique_key"], arg_dict["dest_columns"], arg_dict["predicates"])) %}
  {% else %}
    {% do return(get_delete_insert_merge_sql(arg_dict["target_relation"], arg_dict["temp_relation"], arg_dict["unique_key"], arg_dict["dest_columns"], arg_dict["incremental_predicates"])) %}
{% endmacro %}

Copy link
Contributor

@colin-rogers-dbt colin-rogers-dbt left a comment

Choose a reason for hiding this comment

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

LGTM

@Fleid Fleid added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering Team:Adapters Issues designated for the adapter area of the code test all run tests for all python versions + systems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1054] [Feature] Add predicates to default__get_incremental_merge_sql
6 participants