Fix SnowflakeHook transaction support: multi-statement SQL and AUTOCOMMIT#65040
Open
kaxil wants to merge 3 commits intoapache:mainfrom
Open
Fix SnowflakeHook transaction support: multi-statement SQL and AUTOCOMMIT#65040kaxil wants to merge 3 commits intoapache:mainfrom
SnowflakeHook transaction support: multi-statement SQL and AUTOCOMMIT#65040kaxil wants to merge 3 commits intoapache:mainfrom
Conversation
Member
Author
|
cc @josh-fell |
…MMIT When split_statements=False, pass num_statements=0 to cursor.execute() so Snowflake accepts multi-statement SQL blocks (BEGIN/INSERT/COMMIT). Previously this failed with "Actual statement count N did not match the desired statement count 1". Also respect AUTOCOMMIT in session_parameters instead of unconditionally overriding it with set_autocommit(conn, False). Closes: apache#48233 Closes: apache#30236
646758c to
226f277
Compare
_session_params_has_autocommit already handles non-dict inputs, so the intermediate variable with a dict annotation is unnecessary and causes a mypy assignment error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two longstanding issues with
SnowflakeHook.run()that broke Snowflake transaction support:Multi-statement SQL with
split_statements=False(SnowflakeHook.run() doesn't accept multiple statements without split_statements #48233): When sending a multi-statement block likeBEGIN; INSERT ...; COMMIT;as a singlecursor.execute()call, Snowflake rejects it because the connector defaultsMULTI_STATEMENT_COUNT=1. Now passesnum_statements=0(auto-detect) tocursor.execute()whensplit_statements=False.AUTOCOMMIT session parameter override (Session_parameter AUTOCOMMIT is not set in Snowflake connection #30236): Users who set
session_parameters={"AUTOCOMMIT": True}on their connection had it immediately overridden byset_autocommit(conn, False)inrun(). Now respects the session parameter whenautocommit=False(the default).Design rationale
Why override
_run_command()instead of modifyingrun()inline? The baseDbApiHook._run_command()handles logging, lineage tracking, and row counts. Overriding it inSnowflakeHookkeeps thenum_statementslogic close tocursor.execute()while preserving all side effects. The override drops the psycopg list-to-tuple conversion since Snowflake never uses psycopg.Why case-insensitive AUTOCOMMIT check? Snowflake session parameters are case-insensitive, so users may write
{"autocommit": True}or{"AUTOCOMMIT": True}. The check normalizes to uppercase.Why set
conn.autocommit_mode = Truein the else branch? When we skipset_autocommit()to respect the session parameter,get_autocommit()would returnFalse(sinceautocommit_modewas never set), causing a redundantconn.commit()at the end ofrun(). Setting the mode explicitly avoids this.Why
_get_static_conn_paramsinstead of_get_conn_params()? The static version is a@cached_propertyand doesn't trigger OAuth token refresh, avoiding surprise HTTP requests from what should be a simple config check.Validated against real Snowflake
All three scenarios pass against a real Snowflake instance:
split_statements=Falsesplit_statements=Falsesplit_statements=True(default) still works with separate query IDsCloses #48233
Closes #30236