dbt-labs / dbt-labs/dbt-adapters

[ADAP-629] [BQ] Support new partition configs with all incremental strategies

Open
#604 15 comments 0 reactions 0 assignees View on GitHub
feature:incremental feature:partitioning good-first-issue pkg:dbt-bigquery type:enhancement
Dominant language
Python
Stars
233
Forks
362
Avg merge
3d 22h
Merged PRs (30d)
9

Description

### Describe the feature
Picking up from dbt-labs/dbt#2928, which added support for two new configs in dbt-bigquery: `require_partition_filter` and `partition_expiration_days`.

Let's ensure that `require_partition_filter` works with all the permutations of incremental models on BigQuery. Anyone is welcome to pick this up as a contribution for `v0.20.0`!

### The `merge` strategy

We need the merge condition to be
```sql
on
DBT_INTERNAL_SOURCE.[unique_key] = DBT_INTERNAL_DEST.[unique_key]
and DBT_INTERNAL_DEST.[partition_col] is not null
```

This could be accomplished by passing an additional `predicate` to `get_merge_sql` [here](https://github.com/fishtown-analytics/dbt/blob/1f927a374c8bd52a12a20d892fed9d59cffd04f4/plugins/bigquery/dbt/include/bigquery/macros/materializations/incremental.sql#L147), something like:
```sql
{% is_partition_filter_required = config.get('require_partition_filter', false) %}
{% set predicates = [] %}
{% if is_partition_filter_required %}
{% set partition_filter %} ({{ partition_by.field }} is not null or {{ partition_by.field }} is null) {% endset %}
{% do predicates.append(partition_filter) %}
{% endif %}

{% set build_sql = get_merge_sql(target_relation, source_sql, unique_key, dest_columns, predicates) %}
```

This is a bit of a hack—filtering only in [this sense](https://www.reddit.com/r/funny/comments/1ornz8/these_two_books_contain_the_sum_total_of_all/)—but to be honest there isn't any straightforward way dbt can know in advance the specific partitions it's merging into. For that, you should use...

### The `insert_overwrite` strategy

The `require_partition_filter` config works just fine with "static" `insert_overwrite` strategy—when the user supplies the values in advance via the `partitions` config—which is also the most performant for updating very large datasets. (It would still be a good idea to add a test for this.)

For the "dynamic" `insert_overwrite` strategy, the current error comes in step 2:

https://github.com/fishtown-analytics/dbt/blob/1f927a374c8bd52a12a20d892fed9d59cffd04f4/plugins/bigquery/dbt/include/bigquery/macros/materializations/incremental.sql#L63-L68

We need to either:
- Change this query to include a `where {{ partition_by.field }} is not null`, thereby satisfying the filter requirement
- Change the temp table to _not_ set `require_partition_by`. Should this be a more general rule, that if `temporary = True`, the `create table` statement shouldn't set `require_partition_filter = True`? I think it would make good sense.

### Describe alternatives you've considered
- Not supporting `require_partition_filter` for all types of incremental models. I think we definitely should!

### Related unsolved questions
- Default schema tests will not work on tables with `require_partition_filter` set
- Related addition in dbt-labs/dbt#2928: How should we advise on whether to set `partition_expiration_days` for incremental models? Personally, I'm not so sure

### Who will this benefit?
- BigQuery users with large-volume datasets

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.