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/26 08:42:40 UTC
[GitHub] [airflow] potiuk opened a new pull request, #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
potiuk opened a new pull request, #25299:
URL: https://github.com/apache/airflow/pull/25299
Various providers deriving from DbApi had some variations in some
methods that were derived from the common DbApi Hook. Mostly they
were about extra parameters added and hql parameter used instead of
sql. This prevents from really "common" approach in DbApiHook as
some common sql operators rely on signatures being the same.
This introduced breaking changes in a few providers - but those
breaking changes are easy to fix and most have already been
deprecated.
<!--
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 merged pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25299:
URL: https://github.com/apache/airflow/pull/25299
--
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] kazanzhy commented on a diff in pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r930317248
##########
airflow/providers/apache/pinot/hooks/pinot.py:
##########
@@ -275,7 +275,7 @@ def get_uri(self) -> str:
endpoint = conn.extra_dejson.get('endpoint', 'query/sql')
return f'{conn_type}://{host}/{endpoint}'
- def get_records(self, sql: str, parameters: Optional[Union[Iterable, Mapping]] = None) -> Any:
+ def get_records(self, sql: Union[str, List[str]], parameters: Optional[Any] = None, **kwargs) -> Any:
Review Comment:
I spent a lot of time trying to find out the type of `parameters` parameter.
In OracleHook.callproc() we have the `len(parameters)` situation, therefore it type is `parameters: Optional[Union[List, Dict]]`.
But for the other cases, this parameter is passing to many `.execute()` methods and it's hard to figure out what types these libraries allow.
For example in SQLAlchemy in a few places, there is the following comment.
```
:param parameters: Dictionary, tuple, or list of parameters being
passed to the ``execute()`` or ``executemany()`` method of the
DBAPI ``cursor``
```
Based on this information I decided to generalize it to `parameters: Optional[Union[Iterable, Mapping]]`. Probably, it might be `Union[Sequence, Mapping]`
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25299:
URL: https://github.com/apache/airflow/pull/25299#issuecomment-1196539143
All parameters updated @kazanzhy :)
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25299:
URL: https://github.com/apache/airflow/pull/25299#issuecomment-1195276336
Part of this one was extracted to #25301 - let's see if this one succeds, if so, then we can merge #25301 first to separate the change logically.
--
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] kazanzhy commented on a diff in pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r930319579
##########
airflow/providers/apache/pinot/hooks/pinot.py:
##########
@@ -275,7 +275,7 @@ def get_uri(self) -> str:
endpoint = conn.extra_dejson.get('endpoint', 'query/sql')
return f'{conn_type}://{host}/{endpoint}'
- def get_records(self, sql: str, parameters: Optional[Union[Iterable, Mapping]] = None) -> Any:
+ def get_records(self, sql: Union[str, List[str]], parameters: Optional[Any] = None, **kwargs) -> Any:
Review Comment:
```suggestion
def get_records(self, sql: Union[str, List[str]], parameters: Optional[Union[Iterable, Mapping]] = None, **kwargs) -> Any:
```
--
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] kazanzhy commented on a diff in pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r930321257
##########
airflow/providers/presto/hooks/presto.py:
##########
@@ -142,82 +141,23 @@ def get_isolation_level(self) -> Any:
isolation_level = db.extra_dejson.get('isolation_level', 'AUTOCOMMIT').upper()
return getattr(IsolationLevel, isolation_level, IsolationLevel.AUTOCOMMIT)
- @overload
- def get_records(self, sql: str = "", parameters: Optional[dict] = None):
- """Get a set of records from Presto
-
- :param sql: SQL statement to be executed.
- :param parameters: The parameters to render the SQL query with.
- """
-
- @overload
- def get_records(self, sql: str = "", parameters: Optional[dict] = None, hql: str = ""):
- """:sphinx-autoapi-skip:"""
-
- def get_records(self, sql: str = "", parameters: Optional[dict] = None, hql: str = ""):
- """:sphinx-autoapi-skip:"""
- if hql:
- warnings.warn(
- "The hql parameter has been deprecated. You should pass the sql parameter.",
- DeprecationWarning,
- stacklevel=2,
- )
- sql = hql
-
+ def get_records(self, sql: Union[str, List[str]] = "", parameters: Optional[Any] = None, **kwargs: dict):
Review Comment:
```suggestion
def get_records(self, sql: Union[str, List[str]] = "", parameters: Optional[Union[Iterable, Mapping]] = None, **kwargs: dict):
```
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25299:
URL: https://github.com/apache/airflow/pull/25299#issuecomment-1196644459
Should be 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 pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25299:
URL: https://github.com/apache/airflow/pull/25299#issuecomment-1195188977
cc: @kazanzhy as well. In order to implement #25293 I needed to synchronize all methods in DbAPIHook derived providers (breaking changes in some operators removing deprecated usages). That should make it easier to unify some of the code you plan.
--
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] kazanzhy commented on a diff in pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r930320258
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -197,7 +197,7 @@ def get_records(self, sql, parameters=None):
cur.execute(sql)
return cur.fetchall()
- def get_first(self, sql, parameters=None):
+ def get_first(self, sql: Union[str, List[str]], parameters=None):
Review Comment:
```suggestion
def get_first(self, sql: Union[str, List[str]], parameters: Optional[Union[Iterable, Mapping]] = None):
```
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r929703625
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -981,15 +981,13 @@ def to_csv(
self.log.info("Done. Loaded a total of %s rows.", i)
- def get_records(
- self, hql: str, schema: str = 'default', hive_conf: Optional[Dict[Any, Any]] = None
- ) -> Any:
+ def get_records(self, sql: Union[str, List[str]], parameters: Optional[Any] = None, **kwargs) -> Any:
Review Comment:
Right.
--
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] kazanzhy commented on a diff in pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r930323320
##########
airflow/providers/trino/hooks/trino.py:
##########
@@ -148,96 +147,27 @@ def get_isolation_level(self) -> Any:
isolation_level = db.extra_dejson.get('isolation_level', 'AUTOCOMMIT').upper()
return getattr(IsolationLevel, isolation_level, IsolationLevel.AUTOCOMMIT)
- @overload
- def get_records(self, sql: str = "", parameters: Optional[Union[Iterable, Mapping]] = None):
- """Get a set of records from Trino
-
- :param sql: SQL statement to be executed.
- :param parameters: The parameters to render the SQL query with.
- """
-
- @overload
- def get_records(
- self, sql: str = "", parameters: Optional[Union[Iterable, Mapping]] = None, hql: str = ""
- ):
- """:sphinx-autoapi-skip:"""
-
- def get_records(
- self, sql: str = "", parameters: Optional[Union[Iterable, Mapping]] = None, hql: str = ""
- ):
- """:sphinx-autoapi-skip:"""
- if hql:
- warnings.warn(
- "The hql parameter has been deprecated. You should pass the sql parameter.",
- DeprecationWarning,
- stacklevel=2,
- )
- sql = hql
-
+ def get_records(self, sql: Union[str, List[str]] = "", parameters: Optional[Any] = None, **kwargs: dict):
Review Comment:
```suggestion
def get_records(self, sql: Union[str, List[str]] = "", parameters: Optional[Union[Iterable, Mapping]] = None, **kwargs: dict):
```
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r931135645
##########
airflow/providers/presto/CHANGELOG.rst:
##########
@@ -24,6 +24,12 @@
Changelog
---------
+Breaking changes
+~~~~~~~~~~~~~~~~
+
+Deprecated ``hql`` parameter has been removed in ``get_records``, ``get_first``, ``get_pandas_df`` and ``run``
+methods of the ``TrinoHook``.
Review Comment:
Right: :facepalm:
--
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] kazanzhy commented on a diff in pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r930322863
##########
airflow/providers/presto/hooks/presto.py:
##########
@@ -142,82 +141,23 @@ def get_isolation_level(self) -> Any:
isolation_level = db.extra_dejson.get('isolation_level', 'AUTOCOMMIT').upper()
return getattr(IsolationLevel, isolation_level, IsolationLevel.AUTOCOMMIT)
- @overload
- def get_records(self, sql: str = "", parameters: Optional[dict] = None):
- """Get a set of records from Presto
-
- :param sql: SQL statement to be executed.
- :param parameters: The parameters to render the SQL query with.
- """
-
- @overload
- def get_records(self, sql: str = "", parameters: Optional[dict] = None, hql: str = ""):
- """:sphinx-autoapi-skip:"""
-
- def get_records(self, sql: str = "", parameters: Optional[dict] = None, hql: str = ""):
- """:sphinx-autoapi-skip:"""
- if hql:
- warnings.warn(
- "The hql parameter has been deprecated. You should pass the sql parameter.",
- DeprecationWarning,
- stacklevel=2,
- )
- sql = hql
-
+ def get_records(self, sql: Union[str, List[str]] = "", parameters: Optional[Any] = None, **kwargs: dict):
+ if not isinstance(sql, str):
+ raise ValueError(f"The sql in Presto Hook must be a string and is {sql}!")
try:
return super().get_records(self.strip_sql_string(sql), parameters)
except DatabaseError as e:
raise PrestoException(e)
- @overload
- def get_first(self, sql: str = "", parameters: Optional[dict] = None) -> Any:
- """Returns only the first row, regardless of how many rows the query returns.
-
- :param sql: SQL statement to be executed.
- :param parameters: The parameters to render the SQL query with.
- """
-
- @overload
- def get_first(self, sql: str = "", parameters: Optional[dict] = None, hql: str = "") -> Any:
- """:sphinx-autoapi-skip:"""
-
- def get_first(self, sql: str = "", parameters: Optional[dict] = None, hql: str = "") -> Any:
- """:sphinx-autoapi-skip:"""
- if hql:
- warnings.warn(
- "The hql parameter has been deprecated. You should pass the sql parameter.",
- DeprecationWarning,
- stacklevel=2,
- )
- sql = hql
-
+ def get_first(self, sql: Union[str, List[str]] = "", parameters: Optional[dict] = None) -> Any:
Review Comment:
```suggestion
def get_first(self, sql: Union[str, List[str]] = "", parameters: Optional[Union[Iterable, Mapping]] = None) -> Any:
```
--
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 diff in pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r929702238
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -981,15 +981,13 @@ def to_csv(
self.log.info("Done. Loaded a total of %s rows.", i)
- def get_records(
- self, hql: str, schema: str = 'default', hive_conf: Optional[Dict[Any, Any]] = None
- ) -> Any:
+ def get_records(self, sql: Union[str, List[str]], parameters: Optional[Any] = None, **kwargs) -> Any:
Review Comment:
```suggestion
def get_records(
self,
sql: Union[str, List[str]],
parameters: Optional[Any] = None,
*,
schema: str = "default",
**kwargs,
) -> Any:
```
We can keep `default` here (instead of needing to dig it out from `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] potiuk commented on pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25299:
URL: https://github.com/apache/airflow/pull/25299#issuecomment-1196937390
Woohooo!
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r929709365
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -981,15 +981,13 @@ def to_csv(
self.log.info("Done. Loaded a total of %s rows.", i)
- def get_records(
- self, hql: str, schema: str = 'default', hive_conf: Optional[Dict[Any, Any]] = None
- ) -> Any:
+ def get_records(self, sql: Union[str, List[str]], parameters: Optional[Any] = None, **kwargs) -> Any:
Review Comment:
Actually:
```
"get_records" incompatible with supertype "DbApiHook" [override]
def get_records(
^
airflow/providers/apache/hive/hooks/hive.py:984: note: Superclass:
airflow/providers/apache/hive/hooks/hive.py:984: note: def get_records(self, sql: Union[str, List[str]], parameters: Optional[Any] = ..., **kwargs: Dict[Any, Any]) -> Any
airflow/providers/apache/hive/hooks/hive.py:984: note: Subclass:
airflow/providers/apache/hive/hooks/hive.py:984: note: def get_records(self, sql: Union[str, List[str]], parameters: Optional[Any] = ..., *, schema: str = ..., **kwargs: Any) -> Any
```
I'd like to avoid overloads here so possibly the scheme kwars was a bit better
--
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 diff in pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r929703597
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -81,7 +81,7 @@ class DbApiHook(BaseHook):
Abstract base class for sql hooks.
:param schema: Optional DB schema that overrides the schema specified in the connection. Make sure that
- if you change the schema parameter value in the constructor of the derived Hook, such change
+ if you change the schema parameter value in the constructor of the derived Hook, such changeHOK
Review Comment:
```suggestion
if you change the schema parameter value in the constructor of the derived Hook, such change
```
seems accidental
--
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] kazanzhy commented on a diff in pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r930319937
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -181,7 +181,7 @@ def get_pandas_df_by_chunks(self, sql, parameters=None, *, chunksize, **kwargs):
with closing(self.get_conn()) as conn:
yield from psql.read_sql(sql, con=conn, params=parameters, chunksize=chunksize, **kwargs)
- def get_records(self, sql, parameters=None):
+ def get_records(self, sql: Union[str, List[str]], parameters: Optional[Any] = None, **kwargs: dict):
Review Comment:
```suggestion
def get_records(self, sql: Union[str, List[str]], parameters: Optional[Union[Iterable, Mapping]] = None, **kwargs: dict):
```
--
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] kazanzhy commented on a diff in pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r930320914
##########
airflow/providers/exasol/hooks/exasol.py:
##########
@@ -89,7 +91,7 @@ def get_records(self, sql: str, parameters: Optional[dict] = None) -> List[Union
with closing(conn.execute(sql, parameters)) as cur:
return cur.fetchall()
- def get_first(self, sql: str, parameters: Optional[dict] = None) -> Optional[Any]:
+ def get_first(self, sql: Union[str, List[str]], parameters: Optional[dict] = None) -> Optional[Any]:
Review Comment:
```suggestion
def get_first(self, sql: Union[str, List[str]], parameters: Optional[Union[Iterable, Mapping]] = None) -> Optional[Any]:
```
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25299:
URL: https://github.com/apache/airflow/pull/25299#issuecomment-1196690450
(now it should be green :) )
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r931072172
##########
airflow/providers/presto/CHANGELOG.rst:
##########
@@ -24,6 +24,12 @@
Changelog
---------
+Breaking changes
+~~~~~~~~~~~~~~~~
+
+Deprecated ``hql`` parameter has been removed in ``get_records``, ``get_first``, ``get_pandas_df`` and ``run``
+methods of the ``TrinoHook``.
Review Comment:
```suggestion
Deprecated ``hql`` parameter has been removed in ``get_records``, ``get_first``, ``get_pandas_df`` and ``run``
methods of the ``PrestoHook``.
```
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25299:
URL: https://github.com/apache/airflow/pull/25299#issuecomment-1195196562
This also simplifies and removes some MyPy Hacks with "overrides" :)
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r931069648
##########
scripts/ci/libraries/_testing.sh:
##########
@@ -143,7 +143,7 @@ function testing::setup_docker_compose_backend() {
# so we need to mount an external volume for its db location
# the external db must allow for parallel testing so TEST_TYPE
# is added to the volume name
- export MSSQL_DATA_VOLUME="${HOME}/tmp-mssql-volume-${TEST_TYPE}-${MSSQL_VERSION}"
+ export MSSQL_DATA_VOLUME="${HOME}/tmp-mssql-volume-${TEST_TYPE/\[*\]/}-${MSSQL_VERSION}"
Review Comment:
This is already merged in main via #25338
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25299:
URL: https://github.com/apache/airflow/pull/25299#issuecomment-1196689398
Should be merged after #25338
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r930875548
##########
airflow/providers/apache/pinot/hooks/pinot.py:
##########
@@ -275,7 +275,7 @@ def get_uri(self) -> str:
endpoint = conn.extra_dejson.get('endpoint', 'query/sql')
return f'{conn_type}://{host}/{endpoint}'
- def get_records(self, sql: str, parameters: Optional[Union[Iterable, Mapping]] = None) -> Any:
+ def get_records(self, sql: Union[str, List[str]], parameters: Optional[Any] = None, **kwargs) -> Any:
Review Comment:
Ok. That makes sense. I struggled a bit with that too
--
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 #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25299:
URL: https://github.com/apache/airflow/pull/25299#issuecomment-1196768810
Green :)
--
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] kazanzhy commented on a diff in pull request #25299: Deprecate hql parameters and synchronize DBApiHook method APIs
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on code in PR #25299:
URL: https://github.com/apache/airflow/pull/25299#discussion_r930320639
##########
airflow/providers/exasol/hooks/exasol.py:
##########
@@ -77,7 +77,9 @@ def get_pandas_df(self, sql: str, parameters: Optional[dict] = None, **kwargs) -
df = conn.export_to_pandas(sql, query_params=parameters, **kwargs)
return df
- def get_records(self, sql: str, parameters: Optional[dict] = None) -> List[Union[dict, Tuple[Any, ...]]]:
+ def get_records(
+ self, sql: Union[str, List[str]], parameters: Optional[Any] = None, **kwargs: dict
Review Comment:
```suggestion
self, sql: Union[str, List[str]], parameters: Optional[Union[Iterable, Mapping]] = None, **kwargs: dict
```
--
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