[ADAP-620] [Bug] Incremental materialization with "append" strategy does not wrap dml with explicit transaction

[danielefrigo]

Is this a new bug in dbt-snowflake?

• I believe this is a new bug in dbt-snowflake
• I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

When running an incremental model with "append" incremental strategy, the INSERT statement is not wrapped with an explicit transaction.

Expected Behavior

The INSERT statement should be wrapped as all other DML statements, leveraging the snowflake_dml_explicit_transaction macro.

Steps To Reproduce

1. create a model with incremental materialization and set incremental_strategy = "append"
2. check run sql under target folder: the statement is not wrapped with begin; commit; end;

Relevant log output

No response

Environment

- OS: Linux
- Python: 3.8
- dbt-core: 1.4.6
- dbt-snowflake: 1.4.3

Additional Context

Should be easily fixed adding this here:

{% macro snowflake__get_incremental_append_sql(get_incremental_append_sql) %} {% set dml = default__get_incremental_append_sql(get_incremental_append_sql) %} {% do return(snowflake_dml_explicit_transaction(dml)) %} {% endmacro %}

[dataders]

@danielefrigo great catch! would you be interested in opening the corresponding pull request? I'd be glad to help you get it over the finish line.

[danielefrigo]

I prepared a PR for this (I'm not an expert so please be patient and give me some suggestion).

But before going on, this digging into the code makes me think about Snowflake transactions management:
dbt-labs/dbt-core#3480

Isn't snowflake_dml_explicit_transaction macro going back from this or am I missing some detail?
This is very interesting for me because the AUTOCOMMIT approach brought a negative side effect: it removed the ability to have hooks inside the main transaction.
So my question is: if we're going back to explicit transactions, does it make sense to restore also this feature?

[dataders]

hey @danielefrigo, just took the time to reproduce your result. Correct me if I'm wrong but the insert logic you hope to have wrapped in a transaction is get_insert_into_sql's, right?

I agree that this macro could be executed as a transaction, but I'm not sure what it would accomplish, because it is just a single SQL statement. I may be missing something, but I thought that transactions are helpful when executing a series of a related SQL statements. Instead the update logic of an append is just the following. right?

insert into TEST_DB.dbt_ajs.daniele ("ID", "FIRST_NAME", "LAST_NAME")
    (
        select "ID", "FIRST_NAME", "LAST_NAME"
        from TEST_DB.dbt_ajs.daniele__dbt_tmp
    )

[danielefrigo]

Let's start saying that I was overriding the snowflake_dml_explicit_transaction, and then I noticed that the statements resulting from this kind of materialization were behaving differently from others.
I agree with your analysis, but the same applies to other statements that are currently wrapped (e.g. default__get_merge_sql), so I guessed this was intentionally done for some other reason like performance.
And this connects to my second question about Snowflake transactions management and hooks (
dbt-labs/dbt-core#3480)