Skip to content

Commit 5ea26c2

Browse files
Lazy-load Apache Thrift so SEA/kernel paths never import it
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). Co-authored-by: Isaac
1 parent 84ab9b1 commit 5ea26c2

13 files changed

Lines changed: 325 additions & 42 deletions

File tree

src/databricks/sql/backend/databricks_client.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,13 @@
77
from databricks.sql.client import Cursor
88
from databricks.sql.result_set import ResultSet
99

10-
from databricks.sql.thrift_api.TCLIService import ttypes
10+
# Type-annotation-only import (deferred by ``from __future__ import
11+
# annotations``). ``execute_command`` is typed with ``TSparkParameter`` for
12+
# backwards compatibility, but this abstract base -- and the SEA/kernel
13+
# implementations of it -- never import the Apache Thrift ``thrift`` package
14+
# at load time. See ``test_lazy_thrift_import``.
15+
from databricks.sql.thrift_api.TCLIService import ttypes
16+
1117
from databricks.sql.backend.types import SessionId, CommandId, CommandState
1218

1319

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,12 +47,19 @@
4747
NotSupportedError,
4848
ProgrammingError,
4949
)
50-
from databricks.sql.thrift_api.TCLIService import ttypes
5150

5251
if TYPE_CHECKING:
5352
from databricks.sql.client import Cursor
5453
from databricks.sql.result_set import ResultSet
5554

55+
# Type-annotation-only import (deferred by ``from __future__ import
56+
# annotations``). ``execute_command`` accepts the Thrift-shaped
57+
# ``TSparkParameter`` for interface compatibility and forwards it to
58+
# ``bind_tspark_params``, which only reads its attributes; the kernel
59+
# backend never imports the Apache Thrift ``thrift`` package. See
60+
# ``test_lazy_thrift_import``.
61+
from databricks.sql.thrift_api.TCLIService import ttypes
62+
5663
logger = logging.getLogger(__name__)
5764

5865
# Headers the kernel manages itself and that the connector must NOT

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,19 @@
2121

2222
from __future__ import annotations
2323

24-
from typing import Any, List, Optional, Tuple
24+
from typing import Any, List, Optional, Tuple, TYPE_CHECKING
2525

2626
import pyarrow
2727

2828
from databricks.sql.backend.sea.utils.conversion import SqlType
2929
from databricks.sql.exc import NotSupportedError
30-
from databricks.sql.thrift_api.TCLIService import ttypes
30+
31+
if TYPE_CHECKING:
32+
# Type-annotation-only import (deferred by ``from __future__ import
33+
# annotations``). ``bind_tspark_params`` only reads ``TSparkParameter``
34+
# attributes (duck-typed) at runtime, so the kernel backend never imports
35+
# the Apache Thrift ``thrift`` package. See ``test_lazy_thrift_import``.
36+
from databricks.sql.thrift_api.TCLIService import ttypes
3137

3238
# Type names that the connector emits as compound TSparkParameter
3339
# shapes (payload on ``arguments``, not ``value``). The kernel's

src/databricks/sql/backend/sea/backend.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,17 @@
2020
MetadataCommands,
2121
)
2222
from databricks.sql.backend.sea.utils.normalize import normalize_sea_type_to_thrift
23-
from databricks.sql.thrift_api.TCLIService import ttypes
2423

2524
if TYPE_CHECKING:
2625
from databricks.sql.client import Cursor
2726

27+
# Type-annotation-only import (deferred by ``from __future__ import
28+
# annotations``). ``execute_command`` accepts the Thrift-shaped
29+
# ``TSparkParameter`` for interface compatibility, but only reads its
30+
# attributes (duck-typed) at runtime, so the SEA backend never imports the
31+
# Apache Thrift ``thrift`` package. See ``test_lazy_thrift_import``.
32+
from databricks.sql.thrift_api.TCLIService import ttypes
33+
2834
from databricks.sql.backend.sea.result_set import SeaResultSet
2935

3036
from databricks.sql.backend.databricks_client import DatabricksClient

src/databricks/sql/backend/sea/queue.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,16 @@
2323
ResultData,
2424
ResultManifest,
2525
)
26+
27+
# Type-annotation-only import (deferred by ``from __future__ import
28+
# annotations``). The SEA backend reuses the Thrift ``TSparkArrowResultLink``
29+
# only as the payload the shared cloud-fetch download manager expects; it is
30+
# constructed via a function-local import in ``_convert_to_thrift_link`` so
31+
# importing the SEA backend never imports the Apache Thrift ``thrift``
32+
# package. See ``test_lazy_thrift_import``.
33+
from databricks.sql.thrift_api.TCLIService.ttypes import TSparkArrowResultLink
2634
from databricks.sql.backend.sea.utils.constants import ResultFormat
2735
from databricks.sql.exc import ProgrammingError, ServerOperationError
28-
from databricks.sql.thrift_api.TCLIService.ttypes import TSparkArrowResultLink
2936
from databricks.sql.types import SSLOptions
3037
from databricks.sql.utils import (
3138
ArrowQueue,
@@ -262,6 +269,10 @@ def get_chunk_link(self, chunk_index: int) -> Optional[ExternalLink]:
262269
@staticmethod
263270
def _convert_to_thrift_link(link: ExternalLink) -> TSparkArrowResultLink:
264271
"""Convert SEA external links to Thrift format for compatibility with existing download manager."""
272+
from databricks.sql.thrift_api.TCLIService.ttypes import (
273+
TSparkArrowResultLink,
274+
)
275+
265276
# Parse the ISO format expiration time
266277
expiry_time = int(dateutil.parser.parse(link.expiration).timestamp())
267278
return TSparkArrowResultLink(

src/databricks/sql/backend/types.py

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,21 @@
1+
from __future__ import annotations
2+
13
from dataclasses import dataclass
24
from enum import Enum
3-
from typing import Dict, List, Optional, Any, Tuple
5+
from typing import Dict, List, Optional, Any, Tuple, TYPE_CHECKING
46
import logging
57

68
from databricks.sql.backend.utils.guid_utils import guid_to_hex_id
79
from databricks.sql.telemetry.models.enums import StatementType
8-
from databricks.sql.thrift_api.TCLIService import ttypes
10+
11+
if TYPE_CHECKING:
12+
# Type-annotation-only import (evaluated lazily thanks to
13+
# ``from __future__ import annotations``). The runtime uses of ``ttypes``
14+
# in this module are function-local imports inside the Thrift-only code
15+
# paths (``from_thrift_state``, ``to_thrift_handle``,
16+
# ``to_operation_handle``), so importing this module never pulls in the
17+
# Apache Thrift ``thrift`` package. See ``test_lazy_thrift_import``.
18+
from databricks.sql.thrift_api.TCLIService import ttypes
919

1020
logger = logging.getLogger(__name__)
1121

@@ -60,6 +70,11 @@ def from_thrift_state(
6070
- CANCELED_STATE -> CANCELLED
6171
"""
6272

73+
# Function-local import: this classmethod is only ever called from the
74+
# Thrift backend, so deferring the import keeps ``thrift`` out of the
75+
# SEA/kernel load path.
76+
from databricks.sql.thrift_api.TCLIService import ttypes
77+
6378
if state in (
6479
ttypes.TOperationState.INITIALIZED_STATE,
6580
ttypes.TOperationState.PENDING_STATE,

src/databricks/sql/client.py

Lines changed: 44 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,17 @@
1+
from __future__ import annotations
2+
13
import time
2-
from typing import Dict, Tuple, List, Optional, Any, Union, Sequence, BinaryIO
4+
from typing import (
5+
Dict,
6+
Tuple,
7+
List,
8+
Optional,
9+
Any,
10+
Union,
11+
Sequence,
12+
BinaryIO,
13+
TYPE_CHECKING,
14+
)
315
import pandas
416

517
try:
@@ -25,8 +37,6 @@
2537
DatabaseError,
2638
)
2739

28-
from databricks.sql.thrift_api.TCLIService import ttypes
29-
from databricks.sql.backend.thrift_backend import ThriftDatabricksClient
3040
from databricks.sql.backend.databricks_client import DatabricksClient
3141
from databricks.sql.utils import (
3242
ParamEscaper,
@@ -49,7 +59,7 @@
4959
ParameterApproach,
5060
)
5161

52-
from databricks.sql.result_set import ResultSet, ThriftResultSet
62+
from databricks.sql.result_set import ResultSet
5363
from databricks.sql.types import Row, SSLOptions
5464
from databricks.sql.auth.auth import get_python_sql_connector_auth_provider
5565
from databricks.sql.experimental.oauth_persistence import OAuthPersistence
@@ -60,11 +70,18 @@
6070
from databricks.sql.common.unified_http_client import UnifiedHttpClient
6171
from databricks.sql.common.http import HttpMethod
6272

63-
from databricks.sql.thrift_api.TCLIService.ttypes import (
64-
TOpenSessionResp,
65-
TSparkParameter,
66-
TOperationState,
67-
)
73+
if TYPE_CHECKING:
74+
# Type-annotation-only imports (deferred by ``from __future__ import
75+
# annotations``). ``get_protocol_version`` and ``_prepare_native_parameters``
76+
# are typed with these Thrift-generated types, but the Thrift backend and
77+
# its result set are imported lazily (only on the Thrift connect path), so
78+
# importing this module -- and connecting with the SEA or kernel backend --
79+
# never imports the Apache Thrift ``thrift`` package. See
80+
# ``test_lazy_thrift_import``.
81+
from databricks.sql.thrift_api.TCLIService.ttypes import (
82+
TOpenSessionResp,
83+
TSparkParameter,
84+
)
6885
from databricks.sql.telemetry.telemetry_client import (
6986
TelemetryHelper,
7087
TelemetryClientFactory,
@@ -95,6 +112,24 @@
95112
TRANSACTION_ISOLATION_LEVEL_REPEATABLE_READ = "REPEATABLE_READ"
96113

97114

115+
def __getattr__(name: str) -> Any:
116+
"""Lazily resolve the Thrift backend class as a module attribute.
117+
118+
``client.py`` itself never instantiates ``ThriftDatabricksClient`` (the
119+
backend is chosen in ``Session.open``), but the name is exposed here as a
120+
module attribute so it can be resolved without importing the Apache Thrift
121+
``thrift`` package at module load -- which is what keeps the SEA/kernel
122+
connect path Thrift-free (see ``test_lazy_thrift_import``). It also
123+
preserves the long-standing test seam
124+
``patch("databricks.sql.client.ThriftDatabricksClient")``.
125+
"""
126+
if name == "ThriftDatabricksClient":
127+
from databricks.sql.backend.thrift_backend import ThriftDatabricksClient
128+
129+
return ThriftDatabricksClient
130+
raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
131+
132+
98133
class Connection:
99134
def __init__(
100135
self,

src/databricks/sql/cloudfetch/download_manager.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
1+
from __future__ import annotations
2+
13
import logging
24

35
from concurrent.futures import ThreadPoolExecutor, Future
4-
from typing import List, Union, Tuple, Optional
6+
from typing import List, Union, Tuple, Optional, TYPE_CHECKING
57

68
from databricks.sql.cloudfetch.downloader import (
79
ResultSetDownloadHandler,
@@ -10,7 +12,12 @@
1012
)
1113
from databricks.sql.types import SSLOptions
1214
from databricks.sql.telemetry.models.event import StatementType
13-
from databricks.sql.thrift_api.TCLIService.ttypes import TSparkArrowResultLink
15+
16+
if TYPE_CHECKING:
17+
# Type-annotation-only import; see the note in downloader.py. Keeping the
18+
# ``thrift`` package out of this module lets the SEA/kernel backends use the
19+
# cloud-fetch download manager without importing Apache Thrift.
20+
from databricks.sql.thrift_api.TCLIService.ttypes import TSparkArrowResultLink
1421

1522
logger = logging.getLogger(__name__)
1623

src/databricks/sql/cloudfetch/downloader.py

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,26 @@
1+
from __future__ import annotations
2+
13
import logging
24
from dataclasses import dataclass
3-
from typing import Optional
5+
from typing import Optional, TYPE_CHECKING
46

57
import lz4.frame
68
import time
79
from databricks.sql.common.http import HttpMethod
8-
from databricks.sql.thrift_api.TCLIService.ttypes import TSparkArrowResultLink
910
from databricks.sql.exc import Error
1011
from databricks.sql.types import SSLOptions
1112
from databricks.sql.telemetry.latency_logger import log_latency
1213
from databricks.sql.telemetry.models.event import StatementType
1314
from databricks.sql.common.unified_http_client import UnifiedHttpClient
1415

16+
if TYPE_CHECKING:
17+
# Imported for type annotations only. ``from __future__ import annotations``
18+
# makes every annotation a string, so this import is never evaluated at
19+
# runtime -- which keeps the (Apache Thrift) ``thrift`` package out of the
20+
# cloud-fetch code path used by the SEA and kernel backends. See the
21+
# ``test_lazy_thrift_import`` regression test.
22+
from databricks.sql.thrift_api.TCLIService.ttypes import TSparkArrowResultLink
23+
1524
logger = logging.getLogger(__name__)
1625

1726

0 commit comments

Comments
 (0)