Skip to content

Commit a406b9e

Browse files
ai: apply changes for #925 (1 review thread)
Addresses: - #3877431848 at src/databricks/sql/backend/kernel/client.py:180 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent 46ef531 commit a406b9e

3 files changed

Lines changed: 131 additions & 6 deletions

File tree

src/databricks/sql/backend/kernel/client.py

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222

2323
from __future__ import annotations
2424

25+
import inspect
2526
import logging
2627
import threading
2728
import uuid
@@ -173,10 +174,39 @@ def _is_staging_statement(operation: str) -> bool:
173174
return verb in _STAGING_VERBS
174175

175176

177+
def _kernel_session_accepts_kwarg(name: str) -> bool:
178+
"""True iff the installed ``databricks_sql_kernel.Session`` constructor
179+
declares keyword ``name``.
180+
181+
The kernel ``Session`` is a PyO3 class with a **fixed** signature (no
182+
``**kwargs`` catch-all), so forwarding a kwarg it doesn't declare raises
183+
``TypeError`` at construction. The phase-7 identity/telemetry kwargs
184+
(``driver_name`` etc.) only exist on wheels newer than the pinned
185+
``^0.2.0`` (whose ``Session`` accepts none of them), so we must gate them
186+
on what the actually-installed wheel supports rather than pass them
187+
unconditionally. Falls open (returns ``True``) only when the signature
188+
can't be introspected, so a future non-introspectable binding still gets
189+
the kwargs.
190+
"""
191+
try:
192+
params = inspect.signature(_kernel.Session).parameters
193+
except (TypeError, ValueError):
194+
return True
195+
if any(p.kind is inspect.Parameter.VAR_KEYWORD for p in params.values()):
196+
return True
197+
return name in params
198+
199+
176200
def _kernel_telemetry_kwargs(options: Dict[str, Any]) -> Dict[str, Any]:
177-
"""Build phase-7 telemetry/system kwargs for ``databricks_sql_kernel.Session``."""
201+
"""Build phase-7 telemetry/system kwargs for ``databricks_sql_kernel.Session``.
202+
203+
Only kwargs the installed ``Session`` constructor actually accepts are
204+
returned; on the pinned ``^0.2.0`` wheel (which predates phase 7) this is
205+
empty, so ``open_session`` doesn't break with ``TypeError`` on a wheel
206+
that doesn't yet know these kwargs.
207+
"""
178208
system = TelemetryHelper.get_driver_system_configuration()
179-
out: Dict[str, Any] = {
209+
candidates: Dict[str, Any] = {
180210
"driver_name": system.driver_name,
181211
"driver_version": system.driver_version,
182212
"runtime_name": system.runtime_name,
@@ -193,14 +223,18 @@ def _kernel_telemetry_kwargs(options: Dict[str, Any]) -> Dict[str, Any]:
193223
"process_name": None,
194224
}
195225
if options.get("enable_telemetry") is not None:
196-
out["telemetry_enabled"] = bool(options["enable_telemetry"])
226+
candidates["telemetry_enabled"] = bool(options["enable_telemetry"])
197227
if options.get("telemetry_batch_size") is not None:
198-
out["telemetry_batch_size"] = options["telemetry_batch_size"]
228+
candidates["telemetry_batch_size"] = options["telemetry_batch_size"]
199229
if options.get("telemetry_circuit_breaker_enabled") is not None:
200-
out["telemetry_circuit_breaker_enabled"] = options[
230+
candidates["telemetry_circuit_breaker_enabled"] = options[
201231
"telemetry_circuit_breaker_enabled"
202232
]
203-
return out
233+
return {
234+
name: value
235+
for name, value in candidates.items()
236+
if _kernel_session_accepts_kwarg(name)
237+
}
204238

205239

206240
# ─── Client ─────────────────────────────────────────────────────────────────

tests/unit/_scratch_kernel_kwargs_probe.py

Whitespace-only changes.

tests/unit/test_kernel_client.py

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,6 +428,97 @@ def fake_session(**kw):
428428
assert captured["telemetry_circuit_breaker_enabled"] is False
429429

430430

431+
def test_open_session_omits_phase_7_kwargs_kernel_does_not_accept(monkeypatch):
432+
"""Phase-7 identity/telemetry kwargs must NOT be forwarded to a kernel
433+
``Session`` whose (fixed, no-``**kwargs``) constructor doesn't declare
434+
them.
435+
436+
The real ``databricks_sql_kernel.Session`` is a PyO3 class with a fixed
437+
signature; the pinned ``^0.2.0`` wheel predates phase 7 and accepts none
438+
of these kwargs, so forwarding them unconditionally raises ``TypeError``
439+
and breaks every ``use_kernel=True`` connection. The other tests here use
440+
a ``**kwargs`` MagicMock that silently swallows the kwargs and hides the
441+
break; this one uses a fixed-signature fake mirroring the real 0.2.0
442+
surface to prove the client gates on what the installed Session supports.
443+
"""
444+
captured = {}
445+
446+
# Fixed signature mirroring the pinned 0.2.0 kernel Session: it accepts
447+
# the base connection/tls/retry kwargs but NONE of the phase-7 identity
448+
# or telemetry kwargs, and has no **kwargs catch-all.
449+
def fake_session_v0_2_0(
450+
host,
451+
http_path,
452+
*,
453+
auth_type=None,
454+
access_token=None,
455+
client_id=None,
456+
client_secret=None,
457+
oauth_scopes=None,
458+
token_url=None,
459+
redirect_port=None,
460+
oauth_callback_timeout_secs=None,
461+
tls_ca_cert=None,
462+
tls_skip_verify=False,
463+
tls_skip_hostname_verify=False,
464+
tls_client_cert=None,
465+
tls_client_key=None,
466+
retry_min_wait_secs=None,
467+
retry_max_wait_secs=None,
468+
retry_max_attempts=None,
469+
retry_overall_timeout_secs=None,
470+
http_headers=None,
471+
catalog=None,
472+
schema=None,
473+
session_conf=None,
474+
complex_types_as_json=False,
475+
intervals_as_string=False,
476+
request_timeout_secs=None,
477+
):
478+
captured["host"] = host
479+
sess = MagicMock()
480+
sess.session_id = "sess-id"
481+
return sess
482+
483+
monkeypatch.setattr(kernel_client._kernel, "Session", fake_session_v0_2_0)
484+
monkeypatch.setattr(
485+
kernel_client.TelemetryHelper,
486+
"get_driver_system_configuration",
487+
lambda: types.SimpleNamespace(
488+
driver_name="Databricks SQL Python Connector",
489+
driver_version="1.2.3",
490+
runtime_name="Python 3.12.0",
491+
runtime_version="3.12.0",
492+
runtime_vendor="CPython",
493+
os_name="Linux",
494+
os_version="6.1",
495+
os_arch="x86_64",
496+
client_app_name=None,
497+
locale_name="en_US",
498+
char_set_encoding="utf-8",
499+
),
500+
)
501+
502+
# The kwargs the client builds must be filtered to what fake_session
503+
# accepts, so open_session succeeds instead of raising TypeError.
504+
kwargs = kernel_client._kernel_telemetry_kwargs(
505+
{"enable_telemetry": True, "telemetry_batch_size": 17}
506+
)
507+
assert kwargs == {}, f"expected no phase-7 kwargs on 0.2.0 Session, got {kwargs}"
508+
509+
c = kernel_client.KernelDatabricksClient(
510+
server_hostname="example.cloud.databricks.com",
511+
http_path="/sql/1.0/warehouses/abc",
512+
auth_provider=AccessTokenAuthProvider("dapi-test"),
513+
ssl_options=None,
514+
telemetry_options={"enable_telemetry": True, "telemetry_batch_size": 17},
515+
)
516+
# Would raise TypeError: unexpected keyword argument if the client
517+
# forwarded phase-7 kwargs the fixed-signature Session doesn't declare.
518+
c.open_session(session_configuration=None, catalog=None, schema=None)
519+
assert captured["host"] == "example.cloud.databricks.com"
520+
521+
431522
def test_execute_command_forwards_parameters_to_bind_param():
432523
"""``execute_command(parameters=[...])`` routes each parameter
433524
through ``bind_tspark_params`` onto the kernel statement before

0 commit comments

Comments
 (0)