Skip to content
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

SnowflakeSqlApiOperator - Change the base class #31751

Merged
merged 2 commits into from
Jun 11, 2023

Conversation

utkarsharma2
Copy link
Contributor

@utkarsharma2 utkarsharma2 commented Jun 7, 2023

Since SnowflakeOperator will be deprecated, changed the SnowflakeSqlApiOperator base class to SQLExecuteQueryOperator.

cc: @eladkal

@boring-cyborg boring-cyborg bot added area:providers provider:snowflake Issues related to Snowflake provider labels Jun 7, 2023
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

wouldn't it be easier to change the operator interface to have just hook_params rather than warehouse/database/etc..?

@utkarsharma2
Copy link
Contributor Author

@eladkal We can do that, but I think it's better in the current condition for two reasons IMO.

  1. Interface is aligned with the interface of SnowflakeOperator, which I think users already are used to.
  2. Exposing hook_params to users would require them to know what a hook is(just the terminology), which I think is not required if they want to use the operator. Also, a user who is just trying to connect to Snowflake would be more familiar with the warehouse/database so they should be exposed as is.

But if you strongly feel we should directly expose hook_params I'm happy to make that change :)

@eladkal
Copy link
Contributor

eladkal commented Jun 10, 2023

But if you strongly feel we should directly expose hook_params I'm happy to make that change :)

No strong opinion. We can leave it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:snowflake Issues related to Snowflake provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants