SnowflakeOperator default autocommit flipped to False

[joeknize-bc]

Apache Airflow Provider(s)

snowflake

Versions of Apache Airflow Providers

This started with apache-airflow-providers-snowflake==4.0.0 and is still an issue with 4.0.4

Apache Airflow version

2.5.1

Operating System

Debian GNU/Linux 11 (bullseye)

Deployment

Astronomer

Deployment details

This is affecting both local and hosted deployments

What happened

We are testing out several updated packages, and one thing that broke was the SnowflakeOperator when it was executing a stored procedure. The specific error points to autocommit being set to False:
Stored procedure execution error: Scoped transaction started in stored procedure is incomplete and it was rolled back.

Whereas this used to work in version 3.2.0:

    copy_data_snowflake = SnowflakeOperator(
        task_id=f'copy_{table_name}_snowflake',
        sql=query,
    )

In order for it to work now, we have to specify autocommit=True:

    copy_data_snowflake = SnowflakeOperator(
        task_id=f'copy_{table_name}_snowflake',
        sql=query,
        autocommit=True,
    )

The code still indicates that the default is True, but I believe this commit broke it.

What you think should happen instead

The default for autocommit should revert to the previous behavior, matching the documentation.

How to reproduce

In Snowflake:

CREATE OR REPLACE TABLE PUBLIC.FOO (BAR VARCHAR);
CREATE OR REPLACE PROCEDURE PUBLIC.FOO()
RETURNS VARCHAR
LANGUAGE SQL
AS $$
    INSERT INTO PUBLIC.FOO VALUES('bar');
$$
;

In Airflow, this fails:

    copy_data_snowflake = SnowflakeOperator(
        task_id='call_foo',
        sql="call public.foo()",
    )

But this succeeds:

    copy_data_snowflake = SnowflakeOperator(
        task_id='call_foo',
        sql="call public.foo()",
        autocommit=True,
    )

Anything else

It looks like this may be an issue with stored procedures specifically. If I instead do this:

    copy_data_snowflake = SnowflakeOperator(
        task_id='call_foo',
        sql="INSERT INTO PUBLIC.FOO VALUES('bar');",
    )

The logs show that although autocommit is confusingly set to False, a COMMIT statement is executed:

[2023-03-09, 18:43:09 CST] {cursor.py:727} INFO - query: [ALTER SESSION SET autocommit=False]
[2023-03-09, 18:43:09 CST] {cursor.py:740} INFO - query execution done
[2023-03-09, 18:43:09 CST] {cursor.py:878} INFO - Number of results in first chunk: 1
[2023-03-09, 18:43:09 CST] {sql.py:375} INFO - Running statement: INSERT INTO PUBLIC.FOO VALUES('bar');, parameters: None
[2023-03-09, 18:43:09 CST] {cursor.py:727} INFO - query: [INSERT INTO PUBLIC.FOO VALUES('bar');]
[2023-03-09, 18:43:09 CST] {cursor.py:740} INFO - query execution done
[2023-03-09, 18:43:09 CST] {sql.py:384} INFO - Rows affected: 1
[2023-03-09, 18:43:09 CST] {snowflake.py:380} INFO - Rows affected: 1
[2023-03-09, 18:43:09 CST] {snowflake.py:381} INFO - Snowflake query id: 01aad76b-0606-feb5-0000-26b511d0ba02
[2023-03-09, 18:43:09 CST] {cursor.py:727} INFO - query: [COMMIT]

Are you willing to submit PR?

• Yes I am willing to submit a PR!

Code of Conduct

• I agree to follow this project's Code of Conduct

[boring-cyborg]

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

[eladkal]

Ill check it but either way the operator is deprecated and will be removed in future release. You should switch to SQLExecuteQueryOperator

[potiuk]

I think that was indeed a behaviour change. But I am not sure if we should change it to be default or rather update the documentation to reflect the current state.

I am a bit torn on that one, but since we already released and have "in-the-wild" versions out there with autocommit = False, I woudl be fore just updating the docs and keeping the breaking change.

Snowflake is the only SQL operator that after the unification would have "autocommit" = True. And the unification of the interface was an important aspect of the change and there were other smaller or bigger. As explained in https://airflow.apache.org/docs/apache-airflow-providers-snowflake/stable/index.html#id4 , 4.0.* line (4.0.1 and 4.0.2 were yanked as they did not really work) we unified all DB operators to conform to the same semantics as other operators.
So having a breaking change in this release like that was quite possible. The operator is indeed deprecated, so you should switch to SQLExecuteQueryOperator instead (and there autocommit is and will be False by default).

It is super-easy to fix it - by adding autocommit=True, and we could make sure to mention it it in the release notes in our documentation.

Probably if we realized it then, leaving the "autocomit = True" woudl be a better option for compatibility, but now - the jinni is out of the bottle. In order to actually fix it now we would literally have to bump the version to 5.0* and make it a breaking change for anyone who already started using 4.0.3+. So we don't have a "good" solution here - either we keep the change that breaks 3.* users or we introduce a new breaking change for those who started to use 4.*. Taking into account that we already deprecated it and suggest to use the generic SQL operator with autocommit = False, I think it would be much more confusing if go back to True.

As bad as it is, I think we should swallow the bitter pill and simply update the docs.

WDYT?

[eladkal]

I think we should leave it as is.
remove the wrong doc and add clarification to the release notes of the relevant snowflake version that there was unintentional behavior change.

[potiuk]

Added PR with doc change #30020

[joeknize-bc]

Ok, I understand the situation and agree with the least-bad option of updating the documentation.
I was surprised to read that it is deprecated - there's nothing on this page that indicates it, and there's no deprecation warning showing up in my logs.