Skip to content

Defer to hook setting for split_statements in SQLExecuteQueryOperator - #28635

Merged
dstandish merged 7 commits into
apache:mainfrom
astronomer:defer-to-hook-for-split-statements
Dec 30, 2022
Merged

Defer to hook setting for split_statements in SQLExecuteQueryOperator#28635
dstandish merged 7 commits into
apache:mainfrom
astronomer:defer-to-hook-for-split-statements

Conversation

@dstandish

@dstandish dstandish commented Dec 28, 2022

Copy link
Copy Markdown
Contributor

Some databases, such as snowflake, require you to split statements in order to submit multi-statement sql. For such databases, splitting is the natural default and most intuitive setting, and we should defer to the hook to control that.

cc @potiuk @eladkal @kazanzhy @pgagnon

@dstandish

dstandish commented Dec 28, 2022

Copy link
Copy Markdown
Contributor Author

I don't think this breaks any backcompat because for most providers this is no change and, for snowflake , if you try to submit multistatement r.n. with defaults it will just fail (and this effectively restores the behavior prior to the big sql refactor)

@dstandish
dstandish requested a review from potiuk December 28, 2022 21:20
Comment thread airflow/providers/common/sql/operators/sql.py Outdated
@pgagnon

pgagnon commented Dec 28, 2022

Copy link
Copy Markdown
Contributor

I don't think this breaks any backcompat because for most providers this is no change and, for snowflake , if you try to submit multistatement r.n. with defaults it will just fail

We should be very explicit about this in the release notes though if we merge this change (which I am in favor of), because it potentially alters DAG behavior.

Comment thread airflow/providers/common/sql/operators/sql.py Outdated
@dstandish
dstandish force-pushed the defer-to-hook-for-split-statements branch from 9a9644f to 405ed43 Compare December 29, 2022 19:03
@dstandish
dstandish requested a review from uranusjr December 29, 2022 19:21
Some databases, such as snowflake, require you to split statements in order to submit multi-statement sql.  For such databases, splitting is the natural default, and we should defer to the hook to control that.
@dstandish
dstandish force-pushed the defer-to-hook-for-split-statements branch from faaad8a to 1035a2a Compare December 29, 2022 21:18
Comment thread airflow/providers/common/sql/operators/sql.py Outdated
Comment thread airflow/providers/common/sql/operators/sql.py Outdated
Comment thread airflow/providers/common/sql/operators/sql.pyi
Comment on lines +256 to +259
if self.split_statements is not None:
extra_kwargs = {"split_statements": self.split_statements}
else:
extra_kwargs = {}

@pgagnon pgagnon Dec 30, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if self.split_statements is not None:
extra_kwargs = {"split_statements": self.split_statements}
else:
extra_kwargs = {}
extra_kwargs = {"split_statements": self.split_statements} if self.split_statements else {}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the suggestion. current form is result of TP's suggestion. just gonna leave it as is.

@dstandish
dstandish merged commit 2e7b9f5 into apache:main Dec 30, 2022
@dstandish
dstandish deleted the defer-to-hook-for-split-statements branch December 30, 2022 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants