[GitHub] [airflow] potiuk commented on a diff in pull request #25430: Fix common sql DbApiHook fetch_all_handler

[GitBox <gi...@apache.org>]

potiuk commented on code in PR #25430:
URL: https://github.com/apache/airflow/pull/25430#discussion_r937751711


##########
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 returend.



-- 
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