potiuk commented on code in PR #28006:
URL: https://github.com/apache/airflow/pull/28006#discussion_r1036456268
##########
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
Cool. So I change my opinion. Let's - as we did now with Snowflake - extend
other "DBApuHoook". run methods with return_dictionary. Sounds 1-1 mappable to
what postgres, snowflake, mysql does - you need to specifically request
dictonaries, they are not returned by default. I am fine with that explanation
- and withdraw my suggestion about using dict_cursor. If many providers did
that - so be it, this will become de-facto standard, let's stick to it and map
it to "return_dictionaries" parameter.
> 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.
Added it - and in the next iteration of changes (common.sql 1.4) it might
even become "standard" feature for all hooks. I have completely no problem with
that as long as we keep consistent behaviour.
> 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.
Sure. let's add it as optional feature. I am with you. But let's not change
default behaviour.
> 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.
:
Yes - in the current implementation I added this.
```
@contextmanager
def _get_cursor(self, conn: Any, return_dictionaries: bool):
cursor = None
try:
if return_dictionaries:
cursor = conn.cursor(DictCursor)
else:
cursor = conn.cursor()
yield cursor
finally:
if cursor is not None:
cursor.close()
```
This can easily be in the future moved to DBApiHoook 1.4 with basic
implementation returning regular cursor and can be overridden eventually by
child hooks. If you feel like this is a good direction - that would be great
next step to do :).
--
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]
