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/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/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/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: 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) == [] 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."""