From 6a746d321ac5e561ee985a2588847fe355c2ef13 Mon Sep 17 00:00:00 2001 From: Cody Fincher Date: Sat, 11 Jul 2026 02:43:19 +0000 Subject: [PATCH 1/3] fix(arrow): retype all-NULL columns to string instead of Arrow null pa.Table.from_pylist infers column types from values only, so a column that is NULL in every row collapsed to Arrow null type and lost the database column's declared type. Downstream consumers misread these columns (DuckDB materializes null as INTEGER, breaking string ops). Fall back all-null value-inferred columns to string in convert_dict_to_arrow. Fixes #631 --- sqlspec/utils/arrow_helpers.py | 19 ++++++++++++++++++- tests/unit/utils/test_arrow_helpers.py | 25 +++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/sqlspec/utils/arrow_helpers.py b/sqlspec/utils/arrow_helpers.py index 2470bcfde..9c17c8d19 100644 --- a/sqlspec/utils/arrow_helpers.py +++ b/sqlspec/utils/arrow_helpers.py @@ -104,7 +104,7 @@ def convert_dict_to_arrow( return batches[0] if batches else pa.RecordBatch.from_pydict({}) return empty_table - arrow_table = pa.Table.from_pylist(data) + arrow_table = _coerce_null_columns_to_string(pa.Table.from_pylist(data)) if return_format == "reader": batches = arrow_table.to_batches(max_chunksize=batch_size) @@ -120,6 +120,23 @@ def convert_dict_to_arrow( return arrow_table +def _coerce_null_columns_to_string(table: "ArrowTable") -> "ArrowTable": + """Retype value-inferred Arrow ``null`` columns to ``string``. + + ``pa.Table.from_pylist`` infers column types from values only, so a column + that is ``NULL`` in every row becomes Arrow ``null`` type and loses the + database column's declared type. Downstream consumers (notably DuckDB) then + misread such columns, so fall back to ``string`` which safely holds the + all-null values without breaking string operations. + """ + import pyarrow as pa + + if not any(pa.types.is_null(field.type) for field in table.schema): + return table + fields = [field.with_type(pa.string()) if pa.types.is_null(field.type) else field for field in table.schema] + return table.cast(pa.schema(fields)) + + def convert_dict_to_arrow_with_schema( data: "list[dict[str, Any]]", return_format: Literal["table", "reader", "batch", "batches"] = "table", diff --git a/tests/unit/utils/test_arrow_helpers.py b/tests/unit/utils/test_arrow_helpers.py index fa804d2e0..4c3c948f0 100644 --- a/tests/unit/utils/test_arrow_helpers.py +++ b/tests/unit/utils/test_arrow_helpers.py @@ -85,6 +85,31 @@ def test_convert_with_null_values() -> None: assert pydict["email"][2] is None +def test_all_null_column_falls_back_to_string_type() -> None: + """All-NULL columns must not collapse to Arrow ``null`` type (issue #631).""" + import pyarrow as pa + + data = [{"id": 1, "label": None}, {"id": 2, "label": None}] + result = convert_dict_to_arrow(data, return_format="table") + + label_type = result.schema.field("label").type + assert not pa.types.is_null(label_type) + assert label_type == pa.string() + assert result.schema.field("id").type == pa.int64() + assert result.to_pydict()["label"] == [None, None] + + +def test_all_null_column_fallback_applies_to_reader_format() -> None: + """The null-type fallback must also flow through non-table return formats.""" + import pyarrow as pa + + data = [{"label": None}, {"label": None}] + reader = convert_dict_to_arrow(data, return_format="reader") + + assert not pa.types.is_null(reader.schema.field("label").type) + assert reader.schema.field("label").type == pa.string() + + def test_convert_with_various_types() -> None: """Test converting data with various Python types.""" From 23fdd46aab19c3bed1c1bd808ab57e6c67e048af Mon Sep 17 00:00:00 2001 From: Cody Fincher Date: Sat, 11 Jul 2026 02:43:23 +0000 Subject: [PATCH 2/3] fix(duckdb): collapse extension install and load into one setup outcome A non-required extension whose explicit install failed and whose subsequent load also failed emitted two warnings for one unusable extension. Treat install plus load as a single setup outcome: install-fail + load-success emits no warning (install detail at DEBUG), and any terminal failure of an explicitly-installed extension emits one pool.extension.setup.failed warning carrying install_error and load_error. Name-only load-only and required=True contracts are unchanged. Fixes #636 --- sqlspec/adapters/duckdb/pool.py | 74 ++++++++++++------- .../test_duckdb/test_extension_lifecycle.py | 44 ++++++++++- 2 files changed, 89 insertions(+), 29 deletions(-) diff --git a/sqlspec/adapters/duckdb/pool.py b/sqlspec/adapters/duckdb/pool.py index 00e168dc6..d3fba4f58 100644 --- a/sqlspec/adapters/duckdb/pool.py +++ b/sqlspec/adapters/duckdb/pool.py @@ -134,24 +134,52 @@ def _create_connection(self) -> DuckDBConnection: force_install = bool(ext_config.get("force_install", False)) explicit_install = bool(ext_config.get("install", False)) or force_install or bool(install_kwargs) + install_error: str | None = None if explicit_install: - self._install_extension_once(connection, ext_name, install_kwargs, force_install, required) + install_error = self._install_extension_once( + connection, ext_name, install_kwargs, force_install, required + ) try: connection.load_extension(ext_name) except Exception as exc: if required: raise - log_with_context( - logger, - logging.WARNING, - "pool.extension.load.failed", - adapter=_ADAPTER_NAME, - pool_id=self._pool_id, - database=self._database_name, - extension=ext_name, - error=str(exc), - ) + if explicit_install: + log_with_context( + logger, + logging.WARNING, + "pool.extension.setup.failed", + adapter=_ADAPTER_NAME, + pool_id=self._pool_id, + database=self._database_name, + extension=ext_name, + install_error=install_error, + load_error=str(exc), + ) + else: + log_with_context( + logger, + logging.WARNING, + "pool.extension.load.failed", + adapter=_ADAPTER_NAME, + pool_id=self._pool_id, + database=self._database_name, + extension=ext_name, + error=str(exc), + ) + else: + if install_error is not None: + log_with_context( + logger, + logging.DEBUG, + "pool.extension.install.failed", + adapter=_ADAPTER_NAME, + pool_id=self._pool_id, + database=self._database_name, + extension=ext_name, + error=install_error, + ) for secret_config in self._secrets: _create_secret(connection, secret_config) @@ -168,8 +196,13 @@ def _install_extension_once( install_kwargs: "dict[str, Any]", force_install: bool, required: bool, - ) -> None: - """Install an extension once per pool per signature, best-effort unless required.""" + ) -> "str | None": + """Install an extension once per pool per signature, best-effort unless required. + + Returns the install error string on a best-effort failure so the caller can + combine it with the load outcome into a single setup result; returns ``None`` + when the install succeeds or is skipped. Raises when ``required`` is set. + """ signature = ( ext_name, install_kwargs.get("version"), @@ -178,7 +211,7 @@ def _install_extension_once( ) with self._lock: if not force_install and signature in self._installed_signatures: - return + return None try: if force_install: connection.install_extension(ext_name, force_install=True, **install_kwargs) @@ -189,18 +222,9 @@ def _install_extension_once( except Exception as exc: if required: raise - log_with_context( - logger, - logging.WARNING, - "pool.extension.install.failed", - adapter=_ADAPTER_NAME, - pool_id=self._pool_id, - database=self._database_name, - extension=ext_name, - error=str(exc), - ) - return + return str(exc) self._installed_signatures.add(signature) + return None def _apply_extension_flags(self, connection: DuckDBConnection) -> None: """Apply connection-level extension flags via SET statements.""" diff --git a/tests/unit/adapters/test_duckdb/test_extension_lifecycle.py b/tests/unit/adapters/test_duckdb/test_extension_lifecycle.py index 2e13e4e7e..f0217a9d6 100644 --- a/tests/unit/adapters/test_duckdb/test_extension_lifecycle.py +++ b/tests/unit/adapters/test_duckdb/test_extension_lifecycle.py @@ -159,17 +159,53 @@ def test_load_failure_raises_when_required(monkeypatch: pytest.MonkeyPatch) -> N pool._create_connection() -def test_install_failure_is_best_effort_by_default( +def test_install_failure_with_successful_load_records_no_warning( monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture ) -> None: - """Test E (install): a failing INSTALL is swallowed with a WARNING by default.""" - _spy_connect(monkeypatch, fail_install=True) + """Test E (setup): install fails but a cached load succeeds => no WARNING (issue #636).""" + _spy_connect(monkeypatch, fail_install=True, fail_load=False) + pool = DuckDBConnectionPool({"database": ":memory:"}, extensions=[{"name": "postgres", "install": True}]) + + with caplog.at_level(logging.DEBUG): + pool._create_connection() + + assert [record for record in caplog.records if record.levelno == logging.WARNING] == [] + + +def test_install_and_load_failure_reports_single_setup_warning( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """Test E (setup): install-fail + load-fail collapses to one WARNING carrying both errors (issue #636).""" + _spy_connect(monkeypatch, fail_install=True, fail_load=True) + pool = DuckDBConnectionPool({"database": ":memory:"}, extensions=[{"name": "postgres", "install": True}]) + + with caplog.at_level(logging.WARNING): + pool._create_connection() + + warnings = [record for record in caplog.records if record.levelno == logging.WARNING] + assert len(warnings) == 1 + assert warnings[0].message == "pool.extension.setup.failed" + fields = warnings[0].__dict__["extra_fields"] + assert fields["install_error"] is not None + assert fields["load_error"] is not None + + +def test_install_success_with_load_failure_reports_single_setup_warning( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """Test E (setup): install-success + load-fail emits one setup WARNING (issue #636).""" + _spy_connect(monkeypatch, fail_install=False, fail_load=True) pool = DuckDBConnectionPool({"database": ":memory:"}, extensions=[{"name": "postgres", "install": True}]) with caplog.at_level(logging.WARNING): pool._create_connection() - assert any("install" in record.message and record.levelno == logging.WARNING for record in caplog.records) + warnings = [record for record in caplog.records if record.levelno == logging.WARNING] + assert len(warnings) == 1 + assert warnings[0].message == "pool.extension.setup.failed" + fields = warnings[0].__dict__["extra_fields"] + assert fields["install_error"] is None + assert fields["load_error"] is not None def test_install_failure_raises_when_required(monkeypatch: pytest.MonkeyPatch) -> None: From e087c6e5a3d348f8dd1615a449f9398717029642 Mon Sep 17 00:00:00 2001 From: Cody Fincher Date: Sat, 11 Jul 2026 02:43:25 +0000 Subject: [PATCH 3/3] fix(mssql-python): materialize Row objects into tuples for tuple row format mssql-python returns mssql_python.Row objects that are iterable and indexable but are not tuple subclasses, while the driver declared row_format="tuple". That mismatch tripped the debug row-format fidelity assertion during dict materialization. Convert fetched rows to real tuples in dispatch_execute so the declared contract holds, matching the tuple row shape used across the SQL Server adapter family. Fixes #630 --- sqlspec/adapters/mssql_python/core.py | 16 +++++++- sqlspec/adapters/mssql_python/driver.py | 9 +++- .../test_mssql_python/test_row_collection.py | 41 +++++++++++++++++++ 3 files changed, 63 insertions(+), 3 deletions(-) create mode 100644 tests/unit/adapters/test_mssql_python/test_row_collection.py diff --git a/sqlspec/adapters/mssql_python/core.py b/sqlspec/adapters/mssql_python/core.py index 227d7d5f9..766253e2b 100644 --- a/sqlspec/adapters/mssql_python/core.py +++ b/sqlspec/adapters/mssql_python/core.py @@ -25,7 +25,7 @@ from sqlspec.utils.type_converters import build_uuid_coercions if TYPE_CHECKING: - from collections.abc import Callable, Mapping + from collections.abc import Callable, Mapping, Sequence from logging import Logger from sqlspec.core import StatementConfig @@ -39,6 +39,7 @@ "create_mapped_exception", "default_statement_config", "driver_profile", + "materialize_tuple_rows", ) _ERROR_NUMBER_PATTERN: Final[re.Pattern[str]] = re.compile(r"\(([-]?\d+)\)") @@ -118,6 +119,19 @@ def create_mapped_exception(error: Exception, *, logger: "Logger | None" = None) return SQLSpecError(f"SQL Server database error. Original error: {error}") +def materialize_tuple_rows(fetched: "Sequence[Any] | None") -> "list[tuple[Any, ...]]": + """Materialize mssql-python ``Row`` objects into plain tuples. + + ``mssql-python`` returns ``mssql_python.Row`` objects that are iterable and + indexable but are not ``tuple`` subclasses. The driver reports + ``row_format="tuple"``, so fetched rows are converted to real tuples to keep + that contract accurate when results are materialized. + """ + if not fetched: + return [] + return [tuple(row) for row in fetched] + + def apply_driver_features( statement_config: "StatementConfig", driver_features: "Mapping[str, Any] | None" ) -> "tuple[StatementConfig, dict[str, Any]]": diff --git a/sqlspec/adapters/mssql_python/driver.py b/sqlspec/adapters/mssql_python/driver.py index 1ec855d46..7f9e73772 100644 --- a/sqlspec/adapters/mssql_python/driver.py +++ b/sqlspec/adapters/mssql_python/driver.py @@ -12,7 +12,12 @@ MssqlPythonRawCursor, MssqlPythonSessionContext, ) -from sqlspec.adapters.mssql_python.core import create_mapped_exception, default_statement_config, driver_profile +from sqlspec.adapters.mssql_python.core import ( + create_mapped_exception, + default_statement_config, + driver_profile, + materialize_tuple_rows, +) from sqlspec.adapters.mssql_python.data_dictionary import MssqlPythonSyncDataDictionary from sqlspec.core import ( build_arrow_result_from_reader, @@ -162,7 +167,7 @@ def dispatch_execute(self, cursor: "MssqlPythonRawCursor", statement: "SQL") -> _execute_cursor(cursor, sql, prepared_parameters) if statement.returns_rows(): - fetched = cursor.fetchall() + fetched = materialize_tuple_rows(cursor.fetchall()) column_names = _resolve_column_names(cursor.description, self._column_name_cache) return self.create_execution_result( cursor, diff --git a/tests/unit/adapters/test_mssql_python/test_row_collection.py b/tests/unit/adapters/test_mssql_python/test_row_collection.py new file mode 100644 index 000000000..4ad6918d0 --- /dev/null +++ b/tests/unit/adapters/test_mssql_python/test_row_collection.py @@ -0,0 +1,41 @@ +"""Row-shape fidelity tests for the mssql-python adapter. + +``mssql-python`` returns ``mssql_python.Row`` objects that are iterable and +indexable but are *not* ``tuple`` subclasses. The driver declares +``row_format="tuple"``, so fetched rows must be materialized into real tuples to +keep that contract accurate (issue #630). +""" + +from typing import Any + +from sqlspec.adapters.mssql_python.core import materialize_tuple_rows + + +class _FakeRow: + """Stand-in for ``mssql_python.Row``: iterable/indexable but not a tuple.""" + + def __init__(self, values: "tuple[Any, ...]") -> None: + self._values = list(values) + + def __iter__(self) -> "Any": + return iter(self._values) + + def __getitem__(self, index: int) -> "Any": + return self._values[index] + + def __len__(self) -> int: + return len(self._values) + + +def test_materialize_tuple_rows_converts_row_objects_to_real_tuples() -> None: + rows = [_FakeRow((1, "alice")), _FakeRow((2, "bob"))] + + result = materialize_tuple_rows(rows) + + assert result == [(1, "alice"), (2, "bob")] + assert all(type(row) is tuple for row in result) + + +def test_materialize_tuple_rows_handles_empty_and_none() -> None: + assert materialize_tuple_rows([]) == [] + assert materialize_tuple_rows(None) == []