potiuk commented on code in PR #25430:
URL: https://github.com/apache/airflow/pull/25430#discussion_r937765188
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -30,13 +30,10 @@
from airflow.utils.module_loading import import_string
from airflow.version import version
-if TYPE_CHECKING:
- from sqlalchemy.engine import CursorResult
-
-def fetch_all_handler(cursor: 'CursorResult') -> Optional[List[Tuple]]:
+def fetch_all_handler(cursor) -> Optional[List[Tuple]]:
"""Handler for DbApiHook.run() to return results"""
- if cursor.returns_rows:
+ if cursor.description is not None:
return cursor.fetchall()
Review Comment:
What I particularly do not like about rowcount is this not about `-1`:
```
The attribute is -1 in case no
[.execute*()](https://peps.python.org/pep-0249/#id14)
has been performed on the cursor or the rowcount of the last operation
is cannot be determined by the interface.
[[7]](https://peps.python.org/pep-0249/#id46)
Note
Future versions of the DB API specification could redefine the latter case
to have the object return None instead of -1.
```
I think just having the note indicate that we should avoid it, and there is
absolutely no more guarantees rowcount gives us than description:
```
This attribute will be None for operations that do not return rows
or if the cursor has not had an operation
invoked via the [.execute*()](https://peps.python.org/pep-0249/#id14) method
yet."
```
This is the same, only less ambiguous IMHO.
##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -30,13 +30,10 @@
from airflow.utils.module_loading import import_string
from airflow.version import version
-if TYPE_CHECKING:
- from sqlalchemy.engine import CursorResult
-
-def fetch_all_handler(cursor: 'CursorResult') -> Optional[List[Tuple]]:
+def fetch_all_handler(cursor) -> Optional[List[Tuple]]:
"""Handler for DbApiHook.run() to return results"""
- if cursor.returns_rows:
+ if cursor.description is not None:
return cursor.fetchall()
Review Comment:
```
about cursor.description https://peps.python.org/pep-0249/#description
"This attribute will be None for operations that do not return rows
or if the cursor has not had an operation
invoked via the [.execute*()](https://peps.python.org/pep-0249/#id14) method
yet."
```
This is indeed part of the standardm, so I do no see why we should not base
the decision on that @uranusjr ? It's quite explicitly stated in the PEP that
description is only present when there are some rows potentially to be returned
(and it can be 0 rows as well).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
