potiuk commented on code in PR #28006:
URL: https://github.com/apache/airflow/pull/28006#discussion_r1036409601
##########
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:
Actuallyy in the first version I even had a parameter. And if you think it
is useful, I can add it - no problem. Will do it shortly.
I think, the main reason why this is the case is that this is quite a bit of
abuse of the DbApi context. The base hook that SnowflakeDBHook is based on is
named `DBApiHook`. And this is for a reason. The "dbapi" has pretty well
defined semantics in which there is a separate description described , and
separate rows. There is a PEP describing it https://peps.python.org/pep-0249/ -
and what current "run" and .description approach in other DBApiHook has, is a
very nice 1-1 mapping of that PEP. And we have a number of operators in
common.sql that simply rely on this behaviour.
I would be even tempted to have a "non-dbapi" hook that simply returns a
dictionary. It's a bit of like "have cake and it it too" - on one hand
Snowflake Hook wants to be "DBApiHook" and derive from it, on the other hand it
does not want to follow the semantics of DBApi. This is precisely what
`python-snowflake-connector` did - they added DictCursor option, to break out
from the standard:
https://docs.snowflake.com/en/user-guide/python-connector-api.html#cursor.
And I do not like it. They should have implemented "dict_cursor" method if
they wanted to do it, that would be way, way more reasonable approach. This is
maybe funny, but for some reaosn you are not able to link to that method
documentation :)
(yes, this is because it's the same method, behaving differently - and that
only stresses that it would have been better if the method was named
differently, the linking would work)
But yeah. I am actuallyt happy to add a parameter (which I already have
"dict_results" or smth - false by default, that will turn the DBApiHook into
something different :).
--
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]
