Skip to content

Commit 5d417c0

Browse files
Gabe Pescoclaude
authored andcommitted
fix(databricks): use value check for databricks_connect_use_serverless
Previously, `_set_spark_engine_adapter_if_needed` used a key-presence check (`"databricks_connect_use_serverless" in self._extra_config`) to decide whether to route to serverless. This caused `cluster_id` to be ignored whenever the key existed in config, even if its value was False. Changed to a value check (`self._extra_config.get(...)`) so that `cluster_id` is correctly used when serverless is explicitly disabled. Closes #5842 Signed-off-by: Gabe Pesco <PescoG@medinsight.milliman.com> Co-Authored-By: Claude <noreply@anthropic.com>
1 parent b0bc176 commit 5d417c0

2 files changed

Lines changed: 106 additions & 1 deletion

File tree

sqlmesh/core/engine_adapter/databricks.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ def _set_spark_engine_adapter_if_needed(self) -> None:
154154
host=self._extra_config["databricks_connect_server_hostname"],
155155
token=self._extra_config.get("databricks_connect_access_token"),
156156
)
157-
if "databricks_connect_use_serverless" in self._extra_config:
157+
if self._extra_config.get("databricks_connect_use_serverless"):
158158
connect_kwargs["serverless"] = True
159159
else:
160160
connect_kwargs["cluster_id"] = self._extra_config["databricks_connect_cluster_id"]

tests/core/engine_adapter/test_databricks.py

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -732,3 +732,108 @@ def test_columns(mocker: MockFixture, make_mocked_engine_adapter: t.Callable):
732732
adapter.cursor.execute.assert_called_once_with(
733733
"""SELECT columns.column_name, columns.full_data_type FROM system.information_schema.columns WHERE table_name = 'test_table' AND table_schema = 'test_db' AND table_catalog = 'test_catalog' ORDER BY ordinal_position ASC"""
734734
)
735+
736+
737+
def _make_databricks_connect_adapter(
738+
mocker: MockFixture,
739+
make_mocked_engine_adapter: t.Callable,
740+
extra_config: t.Dict[str, t.Any],
741+
) -> t.Tuple[DatabricksEngineAdapter, t.Any]:
742+
"""Helper that creates a DatabricksEngineAdapter with Databricks Connect mocked out."""
743+
import sys
744+
import types
745+
746+
mock_session = mocker.MagicMock()
747+
mock_builder = mocker.MagicMock()
748+
mock_builder.remote.return_value = mock_builder
749+
mock_builder.userAgent.return_value = mock_builder
750+
mock_builder.getOrCreate.return_value = mock_session
751+
mock_databricks_session_cls = mocker.MagicMock()
752+
mock_databricks_session_cls.builder = mock_builder
753+
754+
# databricks.connect is a local import inside the method, so inject via sys.modules
755+
mock_connect_module = types.ModuleType("databricks.connect")
756+
mock_connect_module.DatabricksSession = mock_databricks_session_cls # type: ignore
757+
mock_databricks_module = types.ModuleType("databricks")
758+
mocker.patch.dict(
759+
sys.modules,
760+
{"databricks": mock_databricks_module, "databricks.connect": mock_connect_module},
761+
)
762+
763+
mocker.patch(
764+
"sqlmesh.core.engine_adapter.databricks.DatabricksEngineAdapter.can_access_spark_session",
765+
return_value=False,
766+
)
767+
mocker.patch(
768+
"sqlmesh.core.engine_adapter.databricks.DatabricksEngineAdapter.can_access_databricks_connect",
769+
return_value=True,
770+
)
771+
772+
adapter = make_mocked_engine_adapter(
773+
DatabricksEngineAdapter,
774+
default_catalog="test_catalog",
775+
**extra_config,
776+
)
777+
return adapter, mock_builder
778+
779+
780+
def test_databricks_connect_routes_to_cluster_id(
781+
mocker: MockFixture, make_mocked_engine_adapter: t.Callable
782+
) -> None:
783+
"""cluster_id is used when databricks_connect_use_serverless is absent."""
784+
extra_config = {
785+
"databricks_connect_server_hostname": "myhost.azuredatabricks.net",
786+
"databricks_connect_access_token": "mytoken",
787+
"databricks_connect_cluster_id": "0123-456789-mycluster",
788+
}
789+
_, mock_builder = _make_databricks_connect_adapter(
790+
mocker, make_mocked_engine_adapter, extra_config
791+
)
792+
793+
mock_builder.remote.assert_called_once_with(
794+
host="myhost.azuredatabricks.net",
795+
token="mytoken",
796+
cluster_id="0123-456789-mycluster",
797+
)
798+
799+
800+
def test_databricks_connect_routes_to_serverless(
801+
mocker: MockFixture, make_mocked_engine_adapter: t.Callable
802+
) -> None:
803+
"""serverless=True is used when databricks_connect_use_serverless is truthy."""
804+
extra_config = {
805+
"databricks_connect_server_hostname": "myhost.azuredatabricks.net",
806+
"databricks_connect_access_token": "mytoken",
807+
"databricks_connect_cluster_id": "0123-456789-mycluster",
808+
"databricks_connect_use_serverless": True,
809+
}
810+
_, mock_builder = _make_databricks_connect_adapter(
811+
mocker, make_mocked_engine_adapter, extra_config
812+
)
813+
814+
mock_builder.remote.assert_called_once_with(
815+
host="myhost.azuredatabricks.net",
816+
token="mytoken",
817+
serverless=True,
818+
)
819+
820+
821+
def test_databricks_connect_cluster_id_not_overridden_by_falsy_serverless(
822+
mocker: MockFixture, make_mocked_engine_adapter: t.Callable
823+
) -> None:
824+
"""cluster_id is used when databricks_connect_use_serverless is present but False."""
825+
extra_config = {
826+
"databricks_connect_server_hostname": "myhost.azuredatabricks.net",
827+
"databricks_connect_access_token": "mytoken",
828+
"databricks_connect_cluster_id": "0123-456789-mycluster",
829+
"databricks_connect_use_serverless": False,
830+
}
831+
_, mock_builder = _make_databricks_connect_adapter(
832+
mocker, make_mocked_engine_adapter, extra_config
833+
)
834+
835+
mock_builder.remote.assert_called_once_with(
836+
host="myhost.azuredatabricks.net",
837+
token="mytoken",
838+
cluster_id="0123-456789-mycluster",
839+
)

0 commit comments

Comments
 (0)