[CT-1507] create config for incremental models to use temp tables or views to fix regression in perf

[McKnight-42]

resolves #323

Description

create a model level config for incremental models to allow choice between using views or temp tables during merge strategy to help with performance issues in some cases.

co-author: @dbeatty10

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

[McKnight-42]

Want to flag this interesting suggestion @dbeatty10 shared with me here #323 (comment) as part of the review discussion. @Fleid any thoughts on this?

[colin-rogers-dbt]

suggested an alternate, maybe cleaner conditional setup but otherwise LGTM

[colin-rogers-dbt]

can we simplify and do the if language == "sql" condition once?

[colin-rogers-dbt]

something like this maybe? not tested or anything...

Suggested change

	{% if language == "sql" and strategy in ("default", "merge") and merge_tmp_relation_type == "table" %}
	{% if language == "sql"%}
	{% if strategy in ("default", "merge")%}
	{% if merge_tmp_relation_type == "table" %}
	{{ return("table") }}
	{% else %}
	{{ return("view") }}
	{% elif strategy == "append" %}
	{{ return("view") }}
	{% elif unique_key is none %}
	{{ return("view") }}
	{% else %}
	{{ return("table") }}

[dbeatty10]

@colin-rogers-dbt What do you think about alternative pseudo code for doing the language condition only once?

It's largely the same as what you proposed. The main difference is that it inverts the sql condition so that it doesn't need any nested conditionals.

if language != "sql":
    return "table"
else if strategy in ('default', 'merge') and merge_tmp_relation_type == "table":
    return "table"
else if strategy in ('default', 'merge') and merge_tmp_relation_type == "view":
    return "view"
else if strategy in ('default', 'merge', 'append'):
    return "view"
else if unique_key is none:
    return "view"
else:
    return "table"

[colin-rogers-dbt]

The != logic is definitely much cleaner

[colin-rogers-dbt]

if merge_tmp_relation_type == "table" would we ever return "view"?

[McKnight-42]

I don't believe so this is actually the crux of the change we are making, trying to offer older method of using table via a config option so as long as they are saying its a merge method and specify their tmp_relation type I think we are saying they are actively choosing then.

[colin-rogers-dbt]

In that case I think we can actually simplify this further:

if language != "sql":
    return "table"
else if strategy in ('append'):
    return "view"
else if merge_tmp_relation_type == "table":
    return "table"
else if merge_tmp_relation_type == "view":
    return "view"
else if unique_key is none:
    return "view"
else:
    return "table"

[McKnight-42]

do you think that would make the fact that merge_tmp_relation_type only technically applies to the merge/default strategy as they are the same in snowflake? I think we wanted to break it out to more readable so users could figure out their configuration needs if needed.

[dbeatty10]

@colin-rogers-dbt Love the way you are trying to make the logic as simple as possible! We just need to make sure to cover each of the moving pieces.

My understanding is that dbt-snowflake supports three different incremental strategies:

1. merge (which is the default)
2. append
3. delete+insert

Something to point out: there's the largely implicit case when strategy == "delete+insert" to consider. In the current version of the PR, it comes into play when all other strategies have already been considered and unique_key is none. If your suggestion is implemented as-is, then the configuration of merge_tmp_relation_type would affect merge and delete+insert, but it wouldn't affect append.

Leaning into your idea, I suspect it would be like:

if language != "sql":
    return "table"
else if strategy in "append":
    return "view"
else if strategy == "delete+insert" and unique_key is none:
    return "view"
else if merge_tmp_relation_type == "table":
    return "table"
else if merge_tmp_relation_type == "view":
    return "view"
else:
    return "table"

Or a more future-proof version presuming some other strategy might be added at a later date:

if language != "sql":
    return "table"
else if strategy in "append":
    return "view"
else if strategy == "delete+insert" and unique_key is none:
    return "view"
else if strategy in ("default", "merge") and merge_tmp_relation_type == "table":
    return "table"
else if strategy in ("default", "merge") and merge_tmp_relation_type == "view":
    return "view"
else:
    return "table"

Side note: here is a proposal to make both the logic and the documentation as simple as possible. It would need buy-in from @Fleid. That proposal is non-blocking for this PR though.

[dbeatty10]

Jinja formatting nit to add missing whitespace:

Suggested change

	{% elif strategy in ('default', 'merge') and merge_tmp_relation_type == "table"%}
	{% elif strategy in ('default', 'merge') and merge_tmp_relation_type == "table" %}

[dbeatty10]

@McKnight-42 we should update the comments in L3-L11 to align with our changes.

Maybe something generally like the following? There's lots of different ways we could write the comments here. A key thing to communicate is why different strategies use either a view or a temporary table. It would be nice if we make it explicit rather than having to do detective work, especially in the case where unique_key is none.

(I didn't bother to limit the line lengths below while we are iterating on the exact wording, but we'll want to add appropriate new lines in the final commit.)

  /* {#
       High-level principles:
       If we are running multiple statements (DELETE + INSERT),
       and we want to guarantee identical inputs to both statements, then
       we _must_ first save the model query results as a temporary table (which presumably comes with a performance cost).
       If we are running a single statement (MERGE or INSERT alone),
       we _may_ save the model query definition as a view instead,
       for (presumably) faster overall incremental processing.

       Low-level specifics:
       Languages other than SQL (like Python) will use a temporary table.
       With the `default` strategy of `merge`, the user may choose between a temporary table and view (defaulting to view).
       The `append` strategy can use a view because it will run a single INSERT statement.
       When the `unique_key` is none, then we can use a view because it will run a single INSERT statement.
       Otherwise, play it safe by using a temporary table.
  #} */

[McKnight-42]

Sounds great! will push up the change to the wording asap

[dbeatty10]

Awesome @McKnight-42 !

Could you choose a maximum line length for the comments and then apply it? It looks like it was using a max line length of 70 previously. Using either 80 or 88 seems reasonable also.

[dbeatty10]

The conversation here makes me think we should make this update in order to future-proof this logic:

Suggested change

	{% elif unique_key is none %}
	{% elif strategy == 'delete+insert' and unique_key is none %}

Otherwise, if some new strategy is added that uses multiple statements in its implementation (like a delete step and an insert step), it might get handled incorrectly accidentally when unique_key is none.