-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add SnowflakeSqlApiOperator operator #30698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SnowflakeSqlApiOperator operator #30698
Conversation
phanikumv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add documentation and example DAG
|
@phanikumv Added documents and example, please review. |
|
@potiuk please review |
I thinkj it's better to avoid calling specific people (unless we know they want to be called). I might be on holidays or focusing on something else. By calling me personally here (when I was not involved before especially) you are limiting the chances for things to be reviewed (because others might think I will do it). It's better to just ask for review Note to self - we need to upgrade our CODEOWNERS rules actually as they might be misleding and they are wrong in multiple cases. |
potiuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Maybe @mik-laj -you would like to have a look ?
|
@potiuk Sure, I'll keep that in mind going forward. Thanks for reviewing :) |
| @@ -0,0 +1,174 @@ | |||
| # Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a hook.
it looks more like util and should be in util path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense. I'll raise a separate PR to move it. thanks for pointing it out :)
| python-modules: | ||
| - airflow.providers.snowflake.hooks.snowflake | ||
| - airflow.providers.snowflake.hooks.snowflake_sql_api | ||
| - airflow.providers.snowflake.hooks.sql_api_generate_jwt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not right. it's not a hook.
| self.query_ids: list[str] = [] | ||
|
|
||
|
|
||
| class SnowflakeSqlApiOperator(SnowflakeOperator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not good.
SnowflakeOperator is deprecated. We should not extend it's functionality.
This class needs to be refactored around SQLExecuteQueryOperator, BaseOperator or possibly have a SnowflakeBaseOperator class ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference... followup PR #31751
Implemented Snowflake SQL API Operator to support multiple SQL statements, the Snowflake SQL API allows submitting multiple SQL statements in a single request, which currently is not possible with
SnowflakeOperatoras it submits multiple statements sequentially.This Operator currently uses key pair authentication, so you need to provide private key raw content or private key file path in the snowflake connection along with other details.
Snowflake SQL API key pair - Authentication https://docs.snowflake.com/en/developer-guide/sql-api/authenticating.html#label-sql-api-authenticating-key-pair
Where can this operator fit in?
- To execute multiple SQL statements in a single request
- To execute the SQL statement and to execute standard queries and most DDL and DML statements