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 2022/07/27 19:21:48 UTC
[GitHub] [airflow] potiuk opened a new pull request, #25350: Move all "old" SQL operators to common.sql providers
potiuk opened a new pull request, #25350:
URL: https://github.com/apache/airflow/pull/25350
Previously, in #24836 we moved Hooks and added some new operators to the
common.sql package. Now we are salso moving the operators
and sensors to common.sql.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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 pull request #25350: Move all "old" SQL operators to common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25350:
URL: https://github.com/apache/airflow/pull/25350#issuecomment-1197424883
Hopefully Green now.
--
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 diff in pull request #25350: Move all "old" SQL operators to common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25350:
URL: https://github.com/apache/airflow/pull/25350#discussion_r931533843
##########
airflow/operators/check_operator.py:
##########
@@ -20,7 +20,7 @@
import warnings
-from airflow.operators.sql import (
+from airflow.providers.common.sql.operators.sql import (
Review Comment:
Very much so @josh-fell :). Fixed.
--
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 diff in pull request #25350: Move all "old" SQL operators to common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25350:
URL: https://github.com/apache/airflow/pull/25350#discussion_r933088257
##########
airflow/providers/common/sql/CHANGELOG.rst:
##########
@@ -24,6 +24,11 @@
Changelog
---------
+1.1.0
Review Comment:
Yes. I need it in order to get the condition fullfilled `>= 1.1.0.*` when building prod images. I will add more description when I prepare the final release.
--
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] eladkal commented on a diff in pull request #25350: Move all "old" SQL operators to common.sql providers
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25350:
URL: https://github.com/apache/airflow/pull/25350#discussion_r932919648
##########
airflow/operators/sql.py:
##########
@@ -15,543 +15,23 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from typing import Any, Dict, Iterable, List, Mapping, Optional, Sequence, SupportsAbs, Union
-
-from airflow.compat.functools import cached_property
-from airflow.exceptions import AirflowException
-from airflow.hooks.base import BaseHook
-from airflow.models import BaseOperator, SkipMixin
-from airflow.providers.common.sql.hooks.sql import DbApiHook
-from airflow.utils.context import Context
-
-
-def parse_boolean(val: str) -> Union[str, bool]:
- """Try to parse a string into boolean.
-
- Raises ValueError if the input is not a valid true- or false-like string value.
- """
- val = val.lower()
- if val in ('y', 'yes', 't', 'true', 'on', '1'):
- return True
- if val in ('n', 'no', 'f', 'false', 'off', '0'):
- return False
- raise ValueError(f"{val!r} is not a boolean-like string value")
-
-
-class BaseSQLOperator(BaseOperator):
- """
- This is a base class for generic SQL Operator to get a DB Hook
-
- The provided method is .get_db_hook(). The default behavior will try to
- retrieve the DB hook based on connection type.
- You can custom the behavior by overriding the .get_db_hook() method.
- """
-
- def __init__(
- self,
- *,
- conn_id: Optional[str] = None,
- database: Optional[str] = None,
- hook_params: Optional[Dict] = None,
- **kwargs,
- ):
- super().__init__(**kwargs)
- self.conn_id = conn_id
- self.database = database
- self.hook_params = {} if hook_params is None else hook_params
-
- @cached_property
- def _hook(self):
- """Get DB Hook based on connection type"""
- self.log.debug("Get connection for %s", self.conn_id)
- conn = BaseHook.get_connection(self.conn_id)
-
- hook = conn.get_hook(hook_params=self.hook_params)
- if not isinstance(hook, DbApiHook):
- raise AirflowException(
- f'The connection type is not supported by {self.__class__.__name__}. '
- f'The associated hook should be a subclass of `DbApiHook`. Got {hook.__class__.__name__}'
- )
-
- if self.database:
- hook.schema = self.database
-
- return hook
-
- def get_db_hook(self) -> DbApiHook:
- """
- Get the database hook for the connection.
-
- :return: the database hook object.
- :rtype: DbApiHook
- """
- return self._hook
-
-
-class SQLCheckOperator(BaseSQLOperator):
Review Comment:
Removing the operators is a breaking change....
We need to deprecate them
##########
airflow/providers/common/sql/CHANGELOG.rst:
##########
@@ -24,6 +24,11 @@
Changelog
---------
+1.1.0
Review Comment:
Did you mean to add it without the log entry of the change?
--
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] josh-fell commented on a diff in pull request #25350: Move all "old" SQL operators to common.sql providers
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25350:
URL: https://github.com/apache/airflow/pull/25350#discussion_r931517038
##########
airflow/operators/check_operator.py:
##########
@@ -20,7 +20,7 @@
import warnings
-from airflow.operators.sql import (
+from airflow.providers.common.sql.operators.sql import (
Review Comment:
Should the deprecation warnings in this file and others also reflect the new import path to use?
--
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] eladkal commented on a diff in pull request #25350: Move all "old" SQL operators to common.sql providers
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25350:
URL: https://github.com/apache/airflow/pull/25350#discussion_r932919648
##########
airflow/operators/sql.py:
##########
@@ -15,543 +15,23 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from typing import Any, Dict, Iterable, List, Mapping, Optional, Sequence, SupportsAbs, Union
-
-from airflow.compat.functools import cached_property
-from airflow.exceptions import AirflowException
-from airflow.hooks.base import BaseHook
-from airflow.models import BaseOperator, SkipMixin
-from airflow.providers.common.sql.hooks.sql import DbApiHook
-from airflow.utils.context import Context
-
-
-def parse_boolean(val: str) -> Union[str, bool]:
- """Try to parse a string into boolean.
-
- Raises ValueError if the input is not a valid true- or false-like string value.
- """
- val = val.lower()
- if val in ('y', 'yes', 't', 'true', 'on', '1'):
- return True
- if val in ('n', 'no', 'f', 'false', 'off', '0'):
- return False
- raise ValueError(f"{val!r} is not a boolean-like string value")
-
-
-class BaseSQLOperator(BaseOperator):
- """
- This is a base class for generic SQL Operator to get a DB Hook
-
- The provided method is .get_db_hook(). The default behavior will try to
- retrieve the DB hook based on connection type.
- You can custom the behavior by overriding the .get_db_hook() method.
- """
-
- def __init__(
- self,
- *,
- conn_id: Optional[str] = None,
- database: Optional[str] = None,
- hook_params: Optional[Dict] = None,
- **kwargs,
- ):
- super().__init__(**kwargs)
- self.conn_id = conn_id
- self.database = database
- self.hook_params = {} if hook_params is None else hook_params
-
- @cached_property
- def _hook(self):
- """Get DB Hook based on connection type"""
- self.log.debug("Get connection for %s", self.conn_id)
- conn = BaseHook.get_connection(self.conn_id)
-
- hook = conn.get_hook(hook_params=self.hook_params)
- if not isinstance(hook, DbApiHook):
- raise AirflowException(
- f'The connection type is not supported by {self.__class__.__name__}. '
- f'The associated hook should be a subclass of `DbApiHook`. Got {hook.__class__.__name__}'
- )
-
- if self.database:
- hook.schema = self.database
-
- return hook
-
- def get_db_hook(self) -> DbApiHook:
- """
- Get the database hook for the connection.
-
- :return: the database hook object.
- :rtype: DbApiHook
- """
- return self._hook
-
-
-class SQLCheckOperator(BaseSQLOperator):
Review Comment:
~Removing the operators is a breaking change.... We need to deprecate them~
Ah you did that :)
I missed it
--
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 merged pull request #25350: Move all "old" SQL operators to common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25350:
URL: https://github.com/apache/airflow/pull/25350
--
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] josh-fell commented on a diff in pull request #25350: Move all "old" SQL operators to common.sql providers
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25350:
URL: https://github.com/apache/airflow/pull/25350#discussion_r931558830
##########
airflow/operators/druid_check_operator.py:
##########
@@ -22,7 +22,7 @@
from airflow.providers.apache.druid.operators.druid_check import DruidCheckOperator # noqa
warnings.warn(
- "This module is deprecated. Please use `airflow.operators.sql.SQLCheckOperator`.",
+ "This module is deprecated. Please use `airflow.providers.apache.druid.operators.druid_check` module.",
Review Comment:
```suggestion
"This module is deprecated. Please use `airflow.providers.common.operators.sql` module.",
```
Looks like the `DruidCheckOperator` and the module is also deprecated and points to the `SQLCheckOperator`. Deprecation inception.
##########
airflow/providers/apache/druid/operators/druid_check.py:
##########
@@ -17,7 +17,7 @@
# under the License.
import warnings
-from airflow.operators.sql import SQLCheckOperator
+from airflow.providers.common.sql.operators.sql import SQLCheckOperator
Review Comment:
One more deprecation message is hiding for `DruidCheckOperator`.
--
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 diff in pull request #25350: Move all "old" SQL operators to common.sql providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25350:
URL: https://github.com/apache/airflow/pull/25350#discussion_r931606453
##########
airflow/operators/druid_check_operator.py:
##########
@@ -22,7 +22,7 @@
from airflow.providers.apache.druid.operators.druid_check import DruidCheckOperator # noqa
warnings.warn(
- "This module is deprecated. Please use `airflow.operators.sql.SQLCheckOperator`.",
+ "This module is deprecated. Please use `airflow.providers.apache.druid.operators.druid_check` module.",
Review Comment:
Ah. That's correct we did not need DruidCheck - plain SQLcheck was enough passing Druid Hook. So all is good after my corrections.
--
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