pankajastro commented on code in PR #75334:
URL: https://github.com/apache/airflow/pull/75334#discussion_r4339747364


##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -823,11 +853,38 @@ def _conn_params_to_sqlalchemy_uri(self, conn_params: 
dict) -> str:
             }
         )
 
+    def _get_connector_params(self) -> dict[str, Any]:
+        """
+        Return ``_get_conn_params()`` in the form the installed 
``snowflake-connector-python`` accepts.
+
+        Connector 5 ignores ``region`` and builds the host from ``account`` 
alone, so the hook
+        passes the host earlier connectors built from the region instead. It 
also warns on every
+        connection unless ``enable_server_session_keep_alive_auto_detection`` 
is set, which earlier
+        connectors don't accept.
+        """
+        conn_params: dict[str, Any] = self._get_conn_params()
+
+        if not _is_connector_5_or_later():
+            conn_params.pop(LOGOUT_AUTO_DETECTION_PARAM, None)
+            return conn_params
+
+        region = conn_params.pop("region")

Review Comment:
   Could this be `pop("region", None)`? A subclass that overrides 
`_get_conn_params` without `region` would otherwise get a `KeyError` on 
connector 5.
   
   ---
   Drafted-by: Claude Code (Sonnet 5.5); reviewed by @pankajastro before posting



##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -849,6 +906,12 @@ def get_sqlalchemy_engine(self, engine_kwargs=None):
         if "ocsp_fail_open" in conn_params:
             engine_kwargs.setdefault("connect_args", {})
             engine_kwargs["connect_args"]["ocsp_fail_open"] = 
conn_params["ocsp_fail_open"]
+        if _is_connector_5_or_later():

Review Comment:
   On connector 5, `get_uri()` still builds the URI from the raw 
`_get_conn_params()`, so `region` goes into the SQLAlchemy URL as 
`account.region`. `get_conn` and Snowpark now send an explicit `host` instead. 
Have you checked that the engine path reaches the same host on 5.x? If not, 
should it get the same handling?
   
   ---
   Drafted-by: Claude Code (Sonnet 5.5); reviewed by @pankajastro before posting



-- 
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]

Reply via email to