potiuk commented on code in PR #28006:
URL: https://github.com/apache/airflow/pull/28006#discussion_r1036863362
##########
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:
Hey @dstandish (and others - I would love to know what you think) -
especially @ephraimbuddy @ashb @jedcunningham @eladkal @uranusjr - those
involved in making the upcoming 2.5.0 release happen.
I think what we have now is really responding to all comments and providing
a good path forwards:
* The operator behaves consistently - same way as before
* The user who already used Hook's run can still bring the old feature
easily by adding `return_dictionaries=True` flag
* We open up the way for other hooks to also do a similar - dictionary -
return option and we can turn it in 1.4 into a "feature" of DBApiHook that
child classes can decide to implement or not.
I think the only thing (correct me if I am wrong @dstandish) is breaking.
Breaking the single Snowflake Hook is unfortunate, but I think this is the best
way to approach it.
Why?
We have two options (unfortunately we are in the situation that we have to
do something to fix the current "broken" state. Status quo is not an option,
because things are broken now and we need to fix them fast - before 2.5.0
release publishing because otherwise, by releasing 2.5.0 we know Snowflake's
users will have a buggy version of snowflake provider that just does not work
for at least few cases. #27978, #27976 are but two manifestations of that
reported even if those problems are not yet exposed by constraints/images of
airflow. If we release 2.5.0 with those, the problem will get several magnitude
of orders worse.
1) We either do it now and take this (LAST) pain of standardising the last
remaining DBAPIHook that behaves differently than all other DBApiHooks
(literally) by default - with a clean "breaking" label and easy way to fix
(just add "return_dictionaries" parameter in `run`command to fix it. I strongly
feel this is the **right** way of solving the problem.
2) or we will continue to carry the pain and continue breaking things for
mutiple users becasue the old setting was literally causing everyone who
touched the DBApiHook area breaking things unexpectedly. I am not even sure
how to do it - I spent quite a good deal of thinking on how we could do it
differently, and I could not figure a good way which does not break
**something** - but maybe someone else can take on the task and would like to
attempt it differently. As I said, we need to do something and take action,
because status quo is that things are seriously broken if we release 2.5.0
without fixing it.,
This is not academic problem that I imagined. The #27978 and #27976 are a
clear example of that. Things got broken not because someone was careless or
becasue it could be easily prevented - but because someone wanted to fix and
improve things. Unfortunately the historical inconsistencies in the way how our
internal code is treated caused that it was either very difficult or impossible
to foresee. It's almost like we deliberately set a trap on contributors and
maintainers who want to evolve and improve our SQL capabilities so that they
have to walk into that trap. And we do not want to fix the trap, we want to
keep it by adding even more complexity and handling exceptions.
With this change we effectively end that with single, clean cut. Rather than
imposing a "death by thousands cuts" oin our users (and make our maintainers
feel guilty of making changes that break things - we deliberately set a clear
cut on where we break and from then on everything is consistent - easier,
tested, prevented from making accidental changes and open to further
improvements and changes.
WDYT?
--
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]
