Changes for incremental strategy refactor compatibility #223
Conversation
| {{ config.set('include_sql_header', true) }} | ||
| {{ get_insert_overwrite_merge_sql(target_relation, source_sql, dest_columns, [predicate]) }} | ||
| {{ config.set('include_sql_header', false) }} |
There was a problem hiding this comment.
Oh, this is clever! And does not feel any hackier than what we're doing right now.
Could we add a comment here, to the effect of:
/* {#
because we're putting the model SQL _directly_ into the MERGE statement,
we need to prepend the MERGE statement with the user-configured sql_header,
which may be needed to resolve that model SQL (e.g. referencing a variable or UDF in the header)
in the "dynamic" case, we save the model SQL result as a temp table first, wherein the
sql_header is included by the create_table_as macro
#} */| @@ -14,7 +14,7 @@ | |||
|
|
|||
| {% macro dbt_bigquery_validate_get_incremental_strategy(config) %} | |||
| {#-- Find and validate the incremental strategy #} | |||
| {%- set strategy = config.get("incremental_strategy", default="merge") -%} | |||
| {%- set strategy = config.get("incremental_strategy") or 'merge' -%} | |||
There was a problem hiding this comment.
👍 This feels like a fair way to go until we can define as:
{% macro bigquery__get_incremental_default_sql(arg_dict) %}
{{ return(get_incremental_merge_sql(arg_dict)) }}
{% endmacro %}Understood that requires more work to get right, following the example in dbt-labs/dbt-snowflake#196
|
@gshank It does look a test started failing, relevant to the I'm very glad we have a test for it, in some way shape or form |
3d045a7 to
6e249dd
Compare
| @@ -46,6 +46,12 @@ | |||
| ) | |||
| {%- endset -%} | |||
|
|
|||
| {#-- Because we're putting the model SQL _directly_ into the MERGE statement, | |||
There was a problem hiding this comment.
do we need sql comment symbols on each line?
There was a problem hiding this comment.
jinja does support comment blocks if thats what your referring to https://tedboy.github.io/jinja2/templ5.html, https://github.com/HubSpot/jinjava/blob/master/src/test/resources/parse/tokenizer/multiline-comment.jinja.
There was a problem hiding this comment.
The reason to put in the SQL comment lines is not because they help comment but because it makes the file highlighting look better :)
There was a problem hiding this comment.
An open question regarding that sql_header: is there a plan to detect them automatically these kind of headers to avoid the "unfriendly" approach of wrapping the code in a {%- call set_sql_header(config) %} block.
It requires dedicated documentation and mentoring on our end and I feel it could be simplified but maybe it's too much work for the overall usage?
resolves #232
Description
Minimal changes to support compatibility with dbt Core incremental materialization refactor
Checklist
CHANGELOG.mdand added information about my change to the "dbt-bigquery next" section.