[GitHub] [airflow] potiuk commented on a diff in pull request #28006: Make Snowflake Hook conform to semantics of DBApi

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

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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org