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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
