Repository: dbt-labs/dbt-core
Pull request: Gets columns to update from config for BQ and Snowflake
resolves #1862 


### Description

Incremental models currently default to updating all columns. For databases that support `merge` statements, this PR allows the user to pass in an `update_columns` config parameter to selectively update only a subset of columns by replacing the call to `adapter.get_columns_in_relation` in the materialization for BigQuery and Snowflake.


### Checklist
 - [x] I have signed the [CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
 - [x] I have run this code in development and it appears to resolve the stated issue
 - [x] This PR includes tests, or tests are not required/relevant for this PR
 - [x] I have updated the `CHANGELOG.md` and added information about my change to the "dbt next" section.


Problem statement:
Specify Columns to update on incremental Models
### Describe the feature
Allow model developer the ability to choose the set of columns that require updating on incremental loads

### Who will this benefit?
This will benefit all users that are performing an incremental update of a wide table with few columns that are mutable

Hints from issue discussion:
hey @richmintz! Can you say more about this? I think it's a good idea - I'm curious if you're interested in this feature for performance reasons, or something else.
Hi @drewbanin!
My primary goal would be performance, minimizing writes and index rebuilds. However I also think it will generate more concise sql for large tables when a merge is required. 
Got it! Sure, this makes a lot of sense in `merge` statements. Is this on Snowflake/BigQuery? I think a similar paradigm would apply to Redshift/Postgres, but we'd modify the `update` statement instead of the `merge` statement.

I can imagine a config like `update_columns` which could be supplied like:
```
{{
  config(
    materialized='incremental',
    update_columns=['order_total', 'count_orders']
  )
}}
```

If `update_columns` is not provided, or if it is `null`, dbt will default to updating all of the columns in the model.

You buy all of that?
 

As part of this proposed change, it would also be nice to be able to exclude the `when matched then update set` part of the merge altogether, as in some of my models I'm only interested in adding new rows since the source data is never updated (for event-based data for example or other append-only tables), and it makes the model execution faster (at least in BigQuery).

It could be a separate config `no_update = True` or just a convention that doesn't include the SQL block when `update_columns` is empty.

Please note I tested this and would work for BigQuery, other databases might need a different syntax to support no-op for updates.

Any thoughts?
hey @bfil - if you do not supply a `unique_key` for your incremental model's config, then dbt should not inject a `when matched then update set....` clause to the merge statement. 

Check out the implementation here:
https://github.com/fishtown-analytics/dbt/blob/7a07017b96ac332c872725343833a94b49129c68/core/dbt/include/global_project/macros/materializations/common/merge.sql#L23-L48
@drewbanin I know, but I still want to compare and insert only the new data based on that unique key rather than merging on an always `FALSE` condition. Makes sense?
ah! Sure, that makes a lot of sense. What do you think about pushing this code down from the materialization layer and into the modeling layer. Could you do something like:

```
{{ config(materialized='incremental') }}

select id, col1, col2 from {{ ref('some_table') }}
{% if is_incremental() %}
where id not in (select id from {{ this }})
{% endif %}
```

This is a kind of funny adaptation of the [typical incremental modeling approach](https://docs.getdbt.com/docs/configuring-incremental-models), but it should serve to only select the _new_ records from your source table, inserting them directly into the destination table.
@drewbanin : any update on where we are with this feature...i have a wide table [Snowflake] but i only need to update very few columns in the incremental model. any ETA's on the feature ?
hey @ee07dazn - no movement on this issue on our end to report. I think the spec I mentioned above (pasted below) is still a good idea:

```
{{
  config(
    materialized='incremental',
    update_columns=['order_total', 'count_orders']
  )
}
```

I think that this feature should only be supported on databases that support `merge` statements, as dbt's delete+insert approach doesn't really lend itself well to this approach. If anyone is interested in working on this one, I'd be happy to try to point you in the right direction. 

The key change here will be to replace the call to [`adapter.get_columns_in_relation`](https://github.com/fishtown-analytics/dbt/blob/f3565f3f70062c5818395d03a52c89e87616f956/plugins/snowflake/dbt/include/snowflake/macros/materializations/incremental.sql#L59) with `config.get('update_columns')`. We'll want to implement this for both the Snowflake and BigQuery incremental materializations when the `merge` strategy is used.
Thanks @drewbanin for the information and a pointer. Apart from what you had suggested, i had to make change to the default__get_merge_sql macro to make that approach work. It works now but i am just not happy to make a change to something as low level as default__get_merge_sql. Probably i think i can update the snowflake_get_merge_sql to do this job. Thinking out loud...but thanks for the help.

Commit message: default to get_columns_in_relation if not specified in config

Co-authored-by: Jeremy Cohen <jtcohen6@gmail.com>

Patch to review:
diff --git a/plugins/bigquery/dbt/include/bigquery/macros/materializations/incremental.sql b/plugins/bigquery/dbt/include/bigquery/macros/materializations/incremental.sql
index f4ad80d5a5c..a48178f88a3 100644
--- a/plugins/bigquery/dbt/include/bigquery/macros/materializations/incremental.sql
+++ b/plugins/bigquery/dbt/include/bigquery/macros/materializations/incremental.sql
@@ -112,7 +112,10 @@
       {% endif %}
       {% set build_sql = create_table_as(False, target_relation, sql) %}
   {% else %}
-     {% set dest_columns = adapter.get_columns_in_relation(existing_relation) %}
+     {% set dest_columns = config.get('update_columns', none) %}
+     {% if dest_columns is none %}
+         {% set dest_columns = adapter.get_columns_in_relation(existing_relation) %}
+     {% endif %}
 
      {#-- if partitioned, use BQ scripting to get the range of partition values to be updated --#}
      {% if strategy == 'insert_overwrite' %}
diff --git a/plugins/snowflake/dbt/include/snowflake/macros/materializations/incremental.sql b/plugins/snowflake/dbt/include/snowflake/macros/materializations/incremental.sql
index 188795b1537..6d0d3bf9805 100644
--- a/plugins/snowflake/dbt/include/snowflake/macros/materializations/incremental.sql
+++ b/plugins/snowflake/dbt/include/snowflake/macros/materializations/incremental.sql
@@ -58,7 +58,7 @@
     {% do adapter.expand_target_column_types(
            from_relation=tmp_relation,
            to_relation=target_relation) %}
-    {% set dest_columns = adapter.get_columns_in_relation(target_relation) %}
+    {% set dest_columns = config.get('update_columns') %}
     {% set build_sql = dbt_snowflake_get_incremental_sql(strategy, tmp_relation, target_relation, unique_key, dest_columns) %}
   {% endif %}
 


Code review comments:
diff_hunk: @@ -58,7 +58,7 @@
     {% do adapter.expand_target_column_types(
            from_relation=tmp_relation,
            to_relation=target_relation) %}
-    {% set dest_columns = adapter.get_columns_in_relation(target_relation) %}
+    {% set dest_columns = config.get('update_columns') %}
path: plugins/snowflake/dbt/include/snowflake/macros/materializations/incremental.sql
text: @user1:
As you did for BigQuery, this should be:
```suggestion
    {% set dest_columns = config.get('update_columns', none) %}
    {% if dest_columns is none %}
        {% set dest_columns = adapter.get_columns_in_relation(existing_relation) %}
    {% endif %}
```
I believe this is what's causing the failing integration test:
```
'NoneType' object is not iterable
```

@author:
of course 🤦