dstandish commented on code in PR #28006:
URL: https://github.com/apache/airflow/pull/28006#discussion_r1036423772
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -369,15 +369,19 @@ def run(
with closing(self.get_conn()) as conn:
self.set_autocommit(conn, autocommit)
- # SnowflakeCursor does not extend ContextManager, so we have to
ignore mypy error here
- with closing(conn.cursor(DictCursor)) as cur: # type:
ignore[type-var]
Review Comment:
from a quick search, seems that it's pretty common.
postgres has it:
https://www.psycopg.org/docs/extras.html#dictionary-like-cursor
and mysql:
https://dev.mysql.com/doc/connector-python/en/connector-python-api-mysqlcursordict.html
i think it's a good, very valuable feature. a very natural way to retrieve
and deal with records. i don't think we should change the behavior of the
snowflake hook. i like the idea of adding some way to control the behavior,
and configure it from connections.
maybe in the hooks we just have an optionally implementable cursor factory
method that returns cursor given conn.
e.g. for snowflake
```
def _cursor_factor(cnx):
return cnx.cursor(DictCursor)
```
and then optionally in future this could be controlled by hook params or
airflow connection, depending on the hook.
aside... just because pep249 says "sequence of sequences" i'm not sure that
should mean we (e.g. postgres, mysql, snowflake) should never add more
capabilities to what a cursor can do. it can still be dbapi compatible and
also go beyond that initial spec with its functionality, no? like parent /
subclass.
--
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]
