Persist predicates to get_merge_sql and wrap in parentheses

[NiallRees]

Resolves #5680 and dbt-labs/dbt-snowflake#231

This enables the already available incremental_predicates to be persisted to get_merge_sql(). It also wraps all predicates in parentheses to force the correct evaluation order when they're anded.

I can now add this config to an incremental model:

        incremental_predicates=[
            'dbt_internal_source.tenant_id = dbt_internal_dest.tenant_id or dbt_internal_source.tenant_id is null or dbt_internal_dest.tenant_id is null',
            'dbt_internal_source.tenant_id2 = dbt_internal_dest.tenant_id2 or dbt_internal_source.tenant_id2 is null or dbt_internal_dest.tenant_id2 is null'
        ],

Resulting in:

merge into analytics.core.model as DBT_INTERNAL_DEST
        using analytics.core.model__dbt_tmp as DBT_INTERNAL_SOURCE
        on (dbt_internal_source.tenant_id = dbt_internal_dest.tenant_id or dbt_internal_source.tenant_id is null or dbt_internal_dest.tenant_id is null) and (dbt_internal_source.tenant_id2 = dbt_internal_dest.tenant_id2 or dbt_internal_source.tenant_id2 is null or dbt_internal_dest.tenant_id2 is null) and 
                DBT_INTERNAL_SOURCE.pk = DBT_INTERNAL_DEST.pk

improving pruning performance.

I've changed the default value of predicates to be an empty list from none, that may have been done for a reason I'm unaware of though! Also not sure if/where to add tests so appreciate any pointers.

Description

Checklist

• I have read the contributing guide and understand what's expected of me
• I have signed the CLA
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
• I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
• I have run changie new to create a changelog entry

[jtcohen6]

Oh! I see what you mean, since predicates could have their own nested conditional logic (and/or):

(predicate_a) and (predicate_b) and (predicate_c)

Would it make more sense to do that wrapping here, or within the templating logic in get_merge_sql?

[NiallRees]

I concluded here was best, as it avoids the need to replicate this logic into every get_x_sql which might use the predicates. I'll defer to you though!

[jtcohen6]

Thanks @NiallRees! I'd be very excited to (finally) have this functionality. I've got a bit of bookkeeping to do, to clean up the issues tracking this work.

The right place for tests is probably in the "incremental" subsection of the "adapter zone" functional tests: https://github.com/dbt-labs/dbt-core/tree/main/tests/adapter/dbt/tests/adapter/incremental. We'd want the ability to pull these tests in and run them on adapter plugins, too.

One thought: We could take this one step further, and make it available to the other incremental strategies as well, by pulling in predicates to each of those macros, too. @dave-connors-3 had done work in that vein back in #4546. If you want to keep this limited in scope, to just default__get_merge_sql and the adapters which use it, that's fine too — we should open a follow-on ticket to track complete availability across adapters + strategies.

[NiallRees]

Thanks @NiallRees! I'd be very excited to (finally) have this functionality. I've got a bit of bookkeeping to do, to clean up the issues tracking this work.

The right place for tests is probably in the "incremental" subsection of the "adapter zone" functional tests: https://github.com/dbt-labs/dbt-core/tree/main/tests/adapter/dbt/tests/adapter/incremental. We'd want the ability to pull these tests in and run them on adapter plugins, too.

One thought: We could take this one step further, and make it available to the other incremental strategies as well, by pulling in predicates to each of those macros, too. @dave-connors-3 had done work in that vein back in #4546. If you want to keep this limited in scope, to just default__get_merge_sql and the adapters which use it, that's fine too — we should open a follow-on ticket to track complete availability across adapters + strategies.

I think I'll keep this limited in scope if that's cool, just to increase my chances of getting it across the finish line relatively quickly!

[jtcohen6]

Closing this PR, in favor of #5702 (merged for inclusion in v1.4!)