You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/11/14 21:24:47 UTC
[GitHub] [airflow] denimalpaca opened a new pull request #18718: Add hook_params in BaseSqlOperator
denimalpaca opened a new pull request #18718:
URL: https://github.com/apache/airflow/pull/18718
Modifies `sql.py` and `connection.py` to take `hook_params` in the form of `**kwargs`. This allows other provider backends, like BigQuery and Snowflake, to use the SQL Operators without subclassing SQL Operators, reducing code duplication and boilerplate for what amounts to just creating a custom `get_db_hook()` function.
This PR can be used in place of (and closes) [18413](https://github.com/apache/airflow/pull/18413).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #18718:
URL: https://github.com/apache/airflow/pull/18718
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] denimalpaca commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
denimalpaca commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r741945657
##########
File path: airflow/providers/snowflake/operators/snowflake.py
##########
@@ -131,281 +126,3 @@ def execute(self, context: Any) -> None:
if self.do_xcom_push:
return execution_info
-
-
-class SnowflakeCheckOperator(SQLCheckOperator):
Review comment:
This PR was opened as an alternative to adding more `SnowflakeCheckOperators`, so it seemed to me that their removal here made sense given the nature of this change making all these `*Check*` operators redundant. Although I see how it could warrant a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#issuecomment-956411525
Can you take a look at the failing tests please @denimalpaca - https://github.com/apache/airflow/runs/4038752898?check_suite_focus=true#step:6:6341
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] denimalpaca commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
denimalpaca commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r741945657
##########
File path: airflow/providers/snowflake/operators/snowflake.py
##########
@@ -131,281 +126,3 @@ def execute(self, context: Any) -> None:
if self.do_xcom_push:
return execution_info
-
-
-class SnowflakeCheckOperator(SQLCheckOperator):
Review comment:
This PR was opened as an alternative to adding more `SnowflakeCheckOperators`, so it seemed to me that their removal here made sense given the nature of this change making all these `*Check*` operators redundant. Although I see how it could warrant a separate PR.
##########
File path: airflow/providers/snowflake/operators/snowflake.py
##########
@@ -131,281 +126,3 @@ def execute(self, context: Any) -> None:
if self.do_xcom_push:
return execution_info
-
-
-class SnowflakeCheckOperator(SQLCheckOperator):
Review comment:
This PR was opened as an alternative to adding more `SnowflakeCheckOperators`, so it seemed to me that their removal here made sense given the nature of this change making all these `*Check*` operators redundant. Although I see how it could warrant a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r723957739
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator or using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >= X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid, and difficult to detect , so maybe we should figure a bit different way of passing parameters here - one that will be easy to detect and flag by pre-commit.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#issuecomment-956411525
Can you take a look at the failing tests please @denimalpaca - https://github.com/apache/airflow/runs/4038752898?check_suite_focus=true#step:6:6341
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r723957739
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change onm it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that either the provider should have the "additional_dependency" added for Airflow >version X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid so maybe we should figure a bit different way here.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r723745553
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
+ """
+ Return hook based on conn_type
+ :param hook_params: dictionary of keyword arguments to be passed to the hook constructor
+ """
Review comment:
This docstring does not seem to match the actual argument?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r723957739
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility (or rather future-compatibility) problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator or using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >= X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid, and difficult to detect , so maybe we should figure a bit different way of passing parameters here - one that will be easy to detect and flag by pre-commit.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r723957739
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >version X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid so maybe we should figure a bit different way here.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r742246987
##########
File path: airflow/providers/snowflake/operators/snowflake.py
##########
@@ -131,281 +126,3 @@ def execute(self, context: Any) -> None:
if self.do_xcom_push:
return execution_info
-
-
-class SnowflakeCheckOperator(SQLCheckOperator):
Review comment:
Yes a separate PR and we don't want the current users of this Operator to have to change their DAGs so let's not remove them please
##########
File path: airflow/providers/snowflake/operators/snowflake.py
##########
@@ -131,281 +126,3 @@ def execute(self, context: Any) -> None:
if self.do_xcom_push:
return execution_info
-
-
-class SnowflakeCheckOperator(SQLCheckOperator):
Review comment:
Yes a separate PR and we don't want the current users of this Operator to have to change their DAGs so let's not remove them please
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] denimalpaca commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
denimalpaca commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r738647444
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
@potiuk the above note by Kaxil has been implemented and tested. Very curious to hear your opinion on that solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] denimalpaca commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
denimalpaca commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r724196989
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
+ """
+ Return hook based on conn_type
+ :param hook_params: dictionary of keyword arguments to be passed to the hook constructor
+ """
Review comment:
Forgot to update it in the previous commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] denimalpaca commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
denimalpaca commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r724196989
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
+ """
+ Return hook based on conn_type
+ :param hook_params: dictionary of keyword arguments to be passed to the hook constructor
+ """
Review comment:
Forgot to update it in the previous commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] denimalpaca commented on pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
denimalpaca commented on pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#issuecomment-966557604
pinging @potiuk and @jedcunningham again for review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#issuecomment-966609442
I just rebased on main branch for tests to pass !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r723957739
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change onm it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that either the provider should have the "additional_dependency" added for Airflow >version X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid so maybe we should figure a bit different way here.
Thoughts?
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that either the provider should have the "additional_dependency" added for Airflow >version X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid so maybe we should figure a bit different way here.
Thoughts?
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >version X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid so maybe we should figure a bit different way here.
Thoughts?
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >= X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid so maybe we should figure a bit different way here.
Thoughts?
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >= X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid, and difficult to detect , so maybe we should figure a bit different way here.
Thoughts?
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >= X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid, and difficult to detect , so maybe we should figure a bit different way of passing parameters here - one that will be easy to detect and flag by pre-commit.
Thoughts?
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator or using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >= X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid, and difficult to detect , so maybe we should figure a bit different way of passing parameters here - one that will be easy to detect and flag by pre-commit.
Thoughts?
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility (or rather future-compatibility) problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator or using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >= X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid, and difficult to detect , so maybe we should figure a bit different way of passing parameters here - one that will be easy to detect and flag by pre-commit.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r723957739
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >= X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid, and difficult to detect , so maybe we should figure a bit different way here.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#issuecomment-968366318
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] denimalpaca commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
denimalpaca commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r741945657
##########
File path: airflow/providers/snowflake/operators/snowflake.py
##########
@@ -131,281 +126,3 @@ def execute(self, context: Any) -> None:
if self.do_xcom_push:
return execution_info
-
-
-class SnowflakeCheckOperator(SQLCheckOperator):
Review comment:
This PR was opened as an alternative to adding more `SnowflakeCheckOperators`, so it seemed to me that their removal here made sense given the nature of this change making all these `*Check*` operators redundant. Although I see how it could warrant a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r742246987
##########
File path: airflow/providers/snowflake/operators/snowflake.py
##########
@@ -131,281 +126,3 @@ def execute(self, context: Any) -> None:
if self.do_xcom_push:
return execution_info
-
-
-class SnowflakeCheckOperator(SQLCheckOperator):
Review comment:
Yes a separate PR and we don't want the current users of this Operator to have to change their DAGs so let's not remove them please
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#issuecomment-962253611
ping @potiuk @jedcunningham for reviews
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r741496164
##########
File path: airflow/providers/snowflake/operators/snowflake.py
##########
@@ -131,281 +126,3 @@ def execute(self, context: Any) -> None:
if self.do_xcom_push:
return execution_info
-
-
-class SnowflakeCheckOperator(SQLCheckOperator):
Review comment:
Why did we remove this operator
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r723957739
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >= X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid, and difficult to detect , so maybe we should figure a bit different way of passing parameters here - one that will be easy to detect and flag by pre-commit.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r723957739
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that the provider should have the "additional_dependency" added for Airflow >= X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid so maybe we should figure a bit different way here.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r730107853
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
Why not just have `hook_kwargs` as a keyword arg as the following:
```suggestion
def get_hook(self, *, hook_kwargs=None):
```
and then L311 can become:
```diff
- return hook_class(**{conn_id_param: self.conn_id}, **kwargs)
+ return hook_class(**{conn_id_param: self.conn_id}, **hook_kwargs)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#issuecomment-967732763
@potiuk @ephraimbuddy Gentle Ping -- to verify this as you'll have requested changes earlier
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r742246987
##########
File path: airflow/providers/snowflake/operators/snowflake.py
##########
@@ -131,281 +126,3 @@ def execute(self, context: Any) -> None:
if self.do_xcom_push:
return execution_info
-
-
-class SnowflakeCheckOperator(SQLCheckOperator):
Review comment:
Yes a separate PR and we don't want the current users of this Operator to have to change their DAGs so let's not remove them please
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r722007122
##########
File path: airflow/operators/sql.py
##########
@@ -34,18 +34,26 @@ class BaseSQLOperator(BaseOperator):
You can custom the behavior by overriding the .get_db_hook() method.
"""
- def __init__(self, *, conn_id: Optional[str] = None, database: Optional[str] = None, **kwargs):
+ def __init__(
+ self,
+ *,
+ conn_id: Optional[str] = None,
+ database: Optional[str] = None,
+ hook_params: Dict = {},
Review comment:
This is wrong, see the important warning on this page: https://docs.python.org/3/tutorial/controlflow.html#default-argument-values
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r723957739
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
Review comment:
There is a backwards-compatibility problem with that approach. While this change on it's own is backwards, compatible and old providers will work, if you implement a change to a provider that adds kwargs to `get_hook`, they will not be usable with already released versions of Airflow.
We have A LOT of operators derived from SQLOperator and using `get_hook()` directly. If we just change a signature of get_hook() it will be very easy for anyone to start using it and breaking compatibility with released Airflow version without anyone noticing.
I am not saying we should not do it, it's a good change but I think this change should be much more thought out and we need to figure out when and how to do it to avoid disruption.
I dop not yet know how to do it best - my current thinking is that we should rather introduce a new method (`get_hook with params()` rather than modify existing ones, and add a pre-commit check that will check if a provider is using that method, and signals that either the provider should have the "additional_dependency" added for Airflow >version X.Y.Z. We would also have to check the usages of SQL Operators the same way and flag problems with dependencies if we see that any of the BaseSQL-derived operators passes `hook_params`. The second part will be much more "brittle" I am afraid so maybe we should figure a bit different way here.
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #18718:
URL: https://github.com/apache/airflow/pull/18718
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18718: Add hook_params in BaseSqlOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18718:
URL: https://github.com/apache/airflow/pull/18718#discussion_r723745553
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, **kwargs):
+ """
+ Return hook based on conn_type
+ :param hook_params: dictionary of keyword arguments to be passed to the hook constructor
+ """
Review comment:
This docstring does not seem to match the actual argument?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org