Re: [PR] Reuse ClickHouse client instead of reconnecting on every hook call [airflow]

["potiuk (via GitHub)" <gi...@apache.org>]

potiuk commented on code in PR #72768:
URL: https://github.com/apache/airflow/pull/72768#discussion_r4186174746


##########
providers/clickhousedb/src/airflow/providers/clickhousedb/hooks/clickhouse.py:
##########
@@ -236,6 +243,9 @@ def _get_client_kwargs(self) -> dict[str, Any]:
             k: v for k, v in merged_client_kwargs.items() if k not in _HOOK_MANAGED_KWARGS
         }
 
+        # Shared/reused client -> no auto-generated session ID unless the caller opts in.
+        kwargs.setdefault("autogenerate_session_id", False)

Review Comment:
   I see why sessions are off for the shared client, since one auto-generated session can't serve concurrent queries. But this changes behaviour that the "No public API change" in the description doesn't cover. `clickhouse-connect`'s sync client defaults to `autogenerate_session_id=True` (`common.py:88` in 1.9.0), and before this PR every `get_conn()` built a new client with its own session. So all statements of one `run()` shared a session. With this default, each statement runs without one:
   - `SQLExecuteQueryOperator` with `CREATE TEMPORARY TABLE t ...; INSERT INTO t ...; SELECT * FROM t` now fails with `UNKNOWN_TABLE`;
   - `SET ...` followed by queries silently stops applying.
   
   Could you keep the old behaviour for the common single-threaded case, for example a session per `run()` / `get_conn()` caller rather than none? Or, if turning sessions off stays the default, call it out as a behaviour change in `providers/clickhousedb/docs/changelog.rst`, with the `client_kwargs={"autogenerate_session_id": True}` way back, and add a test for a multi-statement `run()`.



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