Skip to content

Lazy-load Apache Thrift so the SEA/kernel paths never import it - #907

Merged
vikrantpuppala merged 2 commits into
mainfrom
lazy-load-thrift
Aug 26, 2026
Merged

Lazy-load Apache Thrift so the SEA/kernel paths never import it#907
vikrantpuppala merged 2 commits into
mainfrom
lazy-load-thrift

Conversation

@vikrantpuppala

Copy link
Copy Markdown
Contributor

Problem

The connector imports the PyPI thrift package eagerly the moment connect() loads databricks.sql.client — 16 thrift modules including the top-level thrift package — regardless of whether the caller selected use_sea=True or use_kernel=True. The SEA and kernel backends never speak Thrift on the wire, yet they still drag it in at connect time.

This breaks build systems that vendor their own thrift under the thrift top-level namespace (e.g. Meta's Buck): two packages own the same namespace and the build is rejected, even though the connector's SEA/kernel path never uses Apache Thrift.

Reproduction (before this change):

from databricks.sql.client import Connection   # what connect() lazy-imports
import sys; assert "thrift" in sys.modules      # thrift + 15 submodules loaded

What this changes

Makes the thrift import lazy — imported only when the Thrift backend is actually used. No public API changes and no install-semantics changes: thrift stays a base dependency; it is simply not imported on the SEA/kernel path.

Per module on the connect/execute chain:

  • Annotation-only uses (cloud-fetch download manager/downloader, the DatabricksClient ABC's execute_command, and the SEA/kernel TSparkParameter annotations): add from __future__ import annotations and move the thrift import under TYPE_CHECKING, so annotations are never evaluated at runtime.
  • Runtime uses on Thrift-only paths (from_thrift_state, the queue factory's TSparkRowSetType, TProtocolVersion, the SEA _convert_to_thrift_link construction, and the TSparkParameter* constructions in parameters/native): move the import into the function body.
  • Backend selection in Session.open: resolve ThriftDatabricksClient / SeaDatabricksClient via a module-level __getattr__ (PEP 562) and reference them through the module namespace, so thrift is imported only on the branch that needs it. This also preserves the patch("...session.ThriftDatabricksClient") test seam.
  • Preserve historical re-exports parameters.native.TSparkParameter* and client.ThriftDatabricksClient via lazy __getattr__, so existing importers (and tests) keep working without importing thrift at load.

New test

tests/unit/test_lazy_thrift_import.py imports the connector and each non-Thrift backend in a fresh subprocess and asserts the top-level thrift package is absent from sys.modules (and, conversely, that the Thrift backend still imports it). sys.modules is process-global, so the subprocess isolation is what makes the check reliable — a single stray module-level thrift import anywhere on the chain re-poisons the whole path, and this test catches that.

Verification

  • Importing databricks.sql.client and every SEA/kernel backend module loads zero thrift modules; the Thrift backend still loads thrift. ✅
  • Full unit suite passes (892 passed). The only two failures both reproduce unchanged on main: a kernel test-ordering issue in test_session.py and a realkernel-marked test.
  • black clean on all changed files.

Notes for reviewers / follow-ups

  • This is the runtime lazy-load piece. It unblocks Buck if the blocker is the import-time namespace collision. If the customer needs thrift physically absent from the install (disk-presence collision), that's a separate packaging change (thrift → optional extra) with a default-install-backend compatibility impact — intentionally not included here.
  • Public Cursor.execute was already thrift-free (it takes plain Python params); only the internal execute_command ABC referenced TSparkParameter, and that is annotation-only.

This pull request and its description were written by Isaac.

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Medium · 1 Low

Solid, well-scoped lazy-import refactor — the annotation-only moves are all correctly guarded by from __future__ import annotations, every runtime ttypes use on Thrift-only paths has a function-local import, and the patch(...) test seams are preserved via PEP 562 __getattr__. Two items: a Medium test-robustness bug where the new regression test's exit-code scheme can mask an import failure (notably a false-pass for the Thrift-backend counterpart test), and a Low back-compat note on dropped client.py re-exports.

Comment thread tests/unit/test_lazy_thrift_import.py Outdated
Comment thread src/databricks/sql/client.py

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Review bot failed — see workflow logs.

Reason: model did not call finalize_review and the final text was not parseable JSON: No parseable JSON object in response

The connector declared `thrift` as a hard dependency and imported it
eagerly the moment `connect()` loaded `databricks.sql.client` -- 16
thrift modules, including the top-level `thrift` package -- regardless
of whether the caller selected `use_sea=True` or `use_kernel=True`.
Build systems that vendor their own `thrift` (e.g. Meta's Buck) then hit
a namespace collision even on the SEA / kernel code paths, which never
speak Thrift on the wire.

This makes the thrift import lazy without changing any public API or
install semantics: `thrift` stays a base dependency, but it is only
imported when the Thrift backend is actually used.

Mechanism, per module on the connect/execute chain:
- Annotation-only uses (the cloud-fetch download manager/downloader,
  the DatabricksClient ABC's `execute_command`, and the SEA/kernel
  `TSparkParameter` annotations): add `from __future__ import
  annotations` and move the thrift import under `TYPE_CHECKING`, so the
  annotations are never evaluated at runtime.
- Runtime uses on Thrift-only paths (`from_thrift_state`, the queue
  factory's `TSparkRowSetType`, `TProtocolVersion`, the SEA
  `_convert_to_thrift_link` construction, and every `TSparkParameter*`
  construction in parameters/native): move the import into the function
  body.
- Backend selection in `Session.open`: resolve `ThriftDatabricksClient`
  / `SeaDatabricksClient` via a module-level `__getattr__` (PEP 562) and
  reference them through the module namespace, so thrift is imported
  only on the branch that needs it. This also preserves the
  `patch("...session.ThriftDatabricksClient")` test seam.
- Preserve the historical re-exports `parameters.native.TSparkParameter*`
  and `client.ThriftDatabricksClient` via lazy `__getattr__` so existing
  importers (and tests) keep working without importing thrift at load.

Add tests/unit/test_lazy_thrift_import.py, which imports the connector
and each non-Thrift backend in a fresh subprocess and asserts the
top-level `thrift` package is absent from sys.modules (and, conversely,
that the Thrift backend still imports it). This locks the invariant --
a single stray module-level thrift import re-poisons the whole path.

Verified empirically: importing `databricks.sql.client` and the SEA /
kernel backend modules loads zero thrift modules, while the Thrift
backend still loads thrift. Full unit suite passes (the two failures
present also reproduce unchanged on main: a kernel test-ordering issue
and a realkernel-marked test).

Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Co-authored-by: Isaac
Follow-up to the lazy-thrift change addressing two review-bot findings.

Medium (test robustness / CI false-positive): the guard test's child
subprocess exited with code 1 on any failure, which is also Python's
generic uncaught-exception code -- so an import failure was
indistinguishable from "thrift was imported". This caused a false CI
failure: in the "default deps" job (no pyarrow), importing
`kernel.type_mapping` raises `ModuleNotFoundError: pyarrow` (exit 1),
which the test misread as a thrift leak. It was also a false *pass* risk
for the Thrift-backend counterpart test. The child now emits dedicated
sentinel exit codes only after the import completes, captures stderr,
and reports import failure separately; the parametrized test skips
modules that can't import due to a missing optional dependency (rather
than failing), while still detecting a genuine thrift leak.

Low (back-compat): `client.py`'s `__getattr__` only re-exported
`ThriftDatabricksClient`. Extend it to also lazily resolve the other
names `client.py` historically exposed as importable
(`ThriftResultSet`, `TOpenSessionResp`, `TSparkParameter`,
`TOperationState`), so `from databricks.sql.client import <name>` keeps
working without importing the `thrift` package at module load.

Verified: importing `databricks.sql.client` still loads zero thrift
modules; touching any re-export resolves correctly (and only then pulls
thrift). Guard test passes with full deps (14/14) and correctly skips
the kernel modules when pyarrow/kernel are absent.

Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Co-authored-by: Isaac

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

@vikrantpuppala
vikrantpuppala added this pull request to the merge queue Aug 26, 2026
Merged via the queue into main with commit 4ed862d Aug 26, 2026
50 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants