[el-marvin]

Fixes #732; #736 is its duplicate.

Since 2.0, a program that imports duckdb can end with a block of "nanobind: leaked" lines on stderr after its last statement, although the program leaks nothing at run time. The reporter's script

from typing import overload

import duckdb
import numpy

@overload
def connect(database: str) -> duckdb.DuckDBPyConnection: ...
@overload
def connect(database: None) -> None: ...
def connect(database):
    return None if database is None else duckdb.connect(database)

prints at exit on 2.0.0 (the current v2.0-cyanoptera prints the same with 85 functions):

nanobind: leaked 27 instances!
nanobind: leaked 6 types!
 - leaked type "_duckdb._sqltypes.DuckDBPyType"
 - leaked type "_duckdb.DuckDBPyConnection"
 ...
nanobind: leaked 84 functions!
 ...
nanobind: this is likely caused by a reference counting issue in the binding code.

The same block follows pl.DataFrame(duckdb.connect().sql("select 1 as a")) with polars, the same typing.overload with torch imported instead of numpy, and the one line ctypes.pythonapi.Py_IncRef(ctypes.py_object(duckdb.DuckDBPyConnection)) from #736. A connection object in an ordinary module global prints nothing; what triggers the block is a reference to a duckdb object that is never dropped, as the extra reference of that line is. 1.5 printed nothing, because pybind11 has no such report.

nanobind registers a Py_AtExit handler that runs after Py_FinalizeEx and prints every instance of a bound type that is still alive, and then, once an instance leaked, the types and functions that are still alive too. The 27 instances were duckdb's own copies of its type constants. duckdb/value/constant/__init__.py imported the 27 DuckDBPyType constants by name from duckdb.sqltypes, and the __init__ functions of its Value classes (super().__init__(object, INTEGER) and 26 like it) keep that module's dict alive through their globals. duckdb._expression imports Value, duckdb._relation imports from duckdb._expression, and the 16 Python functions duckdb/__init__.py attaches to DuckDBPyConnection carry duckdb._relation's globals, so anything that keeps the connection type or one of those functions alive past finalization reaches that dict. CPython clears the dict of a module object that is still alive at finalization, not a dict whose module object is gone while functions hold it, so the 27 copies survived. What kept the chain alive came from outside: with numpy or torch imported, typing's globals survive, and with them typing._overload_registry, whose signatures name DuckDBPyConnection; after the polars conversion a functools.cache in polars holds DuckDBPyRelation. Clearing the registry at exit, or from __future__ import annotations, makes the reporter's script exit silently, which is how the chain was found. But the copies at its end were duckdb's choice, and duckdb can release them.

The three modules that bound type constants by name, duckdb.value.constant and the spark column and functions modules, now hold the sqltypes module and read sqltypes.INTEGER when they need it, so no copy of a constant lives in a dict that a held function keeps. duckdb.value.constant exports the same names as before, and the 27 constants it used to import still resolve from it, and still show in dir(), through a module __getattr__ that reads sqltypes at access time; type checkers see them as imports and keep rejecting any other name. The spark DataType singletons held a DuckDBPyType each in their class level cache, 16 instances that any held spark function reached the same way; they now hold the name of their type, and the parsed DuckDBPyType lives in a dict owned by a module of its own, _type_cache, which defines no function, so CPython clears it at finalization like sqltypes. Only what needs a parse goes into that dict, and only when a spark type is constructed, never when one is read: an atomic name is parsed on duckdb's default connection, as a singleton's construction did before, and every singleton is now built when the types module is imported (sixteen of them always were), so that building one later parses nothing. One shape is parsed on a read: a struct without field names, which DuckDB builds for a tuple and every DataFrame over a relation with a tuple column builds for its schema, parses its TUPLE(...) text on the first read of that text, because the text needs the engine's parser and the binding has no constructor for such a struct; a session query over row(1, 2) works in both default connection states because building its schema reads nothing. Decimals, arrays and maps are composed with duckdb.decimal_type, duckdb.list_type and duckdb.map_type, which touch no connection, and hold their type on the instance as before. The cache holds one entry per parsed name or tuple text for the life of the process: the 29 singleton names, and the text of every distinct struct without field names a program reads, also through an array or map built over one (a query over [row(...)] with a new shape, a tuple cast, createDataFrame with a tuple schema); 100,000 distinct containers of named structs add nothing, 100,000 distinct tuple texts add 100,000 entries and about 50 MB. A spark session has a connection of its own, so for the types the module builds and composes, singletons, decimals, arrays and maps, a type built while the default connection was idle and first read while another thread runs a query on it or after a statement failed in its transaction, a type built in that state, a cast to it, createDataFrame with a schema of them and a session query over row(...) keep working, where the merge base failed for a singleton first built in that state. The struct without field names is the exception: the first read of a tuple text that no earlier read parsed, whether the struct was built inside that state or before it, parses on the default connection and refuses, as every read of such a struct did on the merge base (building the struct itself parses nothing); so do, on the merge base and here alike, a query over a list or map of row(...) or a frame built from a tuple column's own schema, and the paths that parse type text themselves rather than a spark type, a cast to a struct with field names, createDataFrame from pandas with a schema, and show() of a tuple holding a TIMESTAMPTZ. createDataFrame of 20,000 rows is level with the merge base: 441 and 447 ms against 452 and 452 ms for an array of intervals in alternating runs, 452 against 437 ms for an array of integers, 413 against 408 ms flat (1,014 and 1,008 ms when reads parsed). Two reads of one atomic type, decimal, array, map or unnamed struct give the same object; a struct with field names composes its type on each read, as before. A read of duckdb_type is a property now, 53 ns for a composed type and 72 ns for a singleton against 28 ns for the plain attribute before, which shows only where types are built per row: createDataFrame without a schema over 20,000 list rows costs 1.6 percent more instructions than on the merge base, 2.3 percent over map rows. Two things change for the experimental spark API: duckdb_type is a property without a setter, and a spark type pickles, where it raised cannot pickle DuckDBPyType before: a decimal, array or map leaves its engine type out of its pickled state and composes it again on load, so a schema handed to a multiprocessing worker works there, and a subclass with its own constructor or an attribute set on an instance survives copy, deepcopy and pickle as it is. With that, the reporter's script, the torch and polars variants and the kept type of #736 exit silently with nanobind's report on, and a connection object kept past exit is reported as what it is: nanobind: leaked 1 instances! with its keep_alive record, and in a full API build an instance line naming _duckdb.DuckDBPyConnection; an abi3 wheel prints the count alone, because nanobind lists the instances only outside the limited API. Three tests keep a later module from bringing the fan out back: a child imports every module of the package, keeps every module dict but the three type stores' alive while the module objects leave sys.modules and die, as happens to a module whose function something holds, and must print nothing with the report on; the same child with a type constant planted in the value module reports exactly that one instance; and a static walk asserts that no module but the type stores binds a DuckDBPyType in its globals. A fourth child casts to spark types, builds a nested frame and keeps a spark function alive, which a cache on the type instances would fail; a fifth does that and then keeps every module dict, which a cache dict kept by the types module instead of its own module would fail.

What the report can still name after this is an object the program itself keeps alive past finalization, and that is not always harmless. A connection to a file database in a plain module global prints nothing and leaves no .wal: CPython clears the module's dict at finalization and the connection closes. When something keeps that dict alive past finalization, as typing's overload registry does for the reporter's script once numpy is imported, the same connection is never closed, is reported as that one instance, and its .wal file stays beside the database until the next open replays it. nanobind's author says of such reports in wjakob/nanobind#644: "In general, these are actual genuine leaks, but they are Python's fault rather than being an issue with nanobind itself. They are also benign in the sense that Python fails to free all instances at shutdown time, but it's not a leak that grows over time at runtime. You can disable the leak warnings if they bother you." In wjakob/nanobind#1013 he says when they appear: "These messages are printed after Python has already shut down (and that's the only time when leaks can be reliably determined)."

The module calls nb::set_leak_warnings(false) at the start of NB_MODULE in builds without assertions, which is every wheel and every install from the sdist, so a user who keeps such an object does not see the block; builds with assertions keep the report, a Debug build, which the editable install is by default, and any build with FORCE_ASSERT, which includes CI's full-API assertion run. That run is a CMake Release build, so the decision does not follow the build type alone. It reads the two compile definitions cmake/duckdb_loader.cmake already derives from the configuration, DUCKDB_DEBUG_MODE from $<CONFIG:Debug> and DUCKDB_FORCE_ASSERT from the FORCE_ASSERT option, which reach every binding object through _duckdb_dependencies; the engine's own DEBUG define is set only for the engine's sources and never reaches the bindings. The module exposes the same answer as the private attribute _duckdb._assertions, and the private function _duckdb._set_leak_report(enabled) turns the report on or off in any build, so that a test child, or a developer who wants to see the report in a wheel, does not need a build from source. Both are declared in the stub as private and not re-exported from duckdb.

nanobind keeps the flag in state it shares with every extension that has the same key, which names, among others, the nanobind internals version, the compiler family and standard library, the limited API, free threading and the domain. duckdb was built without a domain of its own, so it shared that state with every other nanobind extension in the process with the same key, and the sharing went beyond the flag: duckdb's catch all exception translator ran for those extensions too, and another extension's std::bad_alloc surfaced as _duckdb.OutOfMemoryException instead of MemoryError once duckdb was imported. A release duckdb would also have turned off such an extension's exit report from the moment it was imported, and such an extension that turns its flag off would have silenced a Debug duckdb's report. The module is therefore now built with NB_DOMAIN=duckdb, an interface compile definition of _duckdb_dependencies, which gives it nanobind state of its own: the flag, the type registry and the exception translators. The define sits there and not on nanobind_add_module because nanobind puts that one on the module target only, while NB_MODULE expands in duckdb_python.cpp, which is compiled into an object library. duckdb binds no type that another nanobind extension consumes, so the own registry changes nothing for them. A small nanobind extension that leaks one instance on purpose prints its report alone and in both import orders with a release duckdb, duckdb prints nothing next to it, in an assertion build duckdb's report prints after that extension turned its own flag off, and a second extension that throws std::bad_alloc raises MemoryError with duckdb imported.

The one real leak behind the same message, a connection whose Python callables form a cycle the garbage collector cannot see, is fixed at its root in the open #743 and stays visible in builds with assertions.

The existing tests that check that a child process prints no "nanobind: leaked" line, a scan after the module left sys.modules, interpreter exit with a registered Arrow source, the interpreter shutdown cases in the slow suite, and the map function child whose stderr must stay empty, keep that check live in every build: each child whose exit is checked calls _duckdb._set_leak_report(True) right after its own import of duckdb, and in the shutdown suite the check asserts that the child's script carries the call, so a child checked without it fails instead of passing for nothing in a build without assertions. The one child that leaves through the C library's exit is no longer checked for the report: that exit skips Py_FinalizeEx and with it nanobind's report, so the check could never fail.

CI pins the configuration of every artifact it tests. The abi3 wheel test job and the sdist install step assert not _duckdb._assertions before anything else runs, the sdist one in the step that every run executes rather than the test step that pull request runs skip, the cibuildwheel test command asserts the same before its suite, and the assertion job's build check asserts _duckdb._assertions, so a wheel or sdist that starts printing the report again, or an assertion job that stops, turns CI red. The macOS universal2 wheels, whose test command cibuildwheel skips in favour of the thin wheels of the same configuration, are the one artifact without the check.

The new tests run each case in a child interpreter with the report turned on and read its stderr: the reporter's overload with numpy and with torch, the polars conversion, the type reference from #736, the child that keeps every module dict and the child that uses spark types print nothing in every build, and a connection object kept past exit prints exactly one instance; on the tree before the fix the same children print 27, 27, 27, 27 and 28 instances, and the module dict child 16 with the spark cache alone. The 27 names that duckdb.value.constant used to import are tested to resolve from it by import and by attribute to the sqltypes objects, to show in dir(), and to leave the star import and any other name as they were. Two spark tests build types while the default connection is idle and read them for the first time, build decimals, arrays of intervals and a singleton, cast and build frames while the default connection runs a query on another thread and after a failed statement in its transaction; a child interpreter builds singletons, an array of intervals and a decimal for the first time while its default connection is held, another loads pickled types (an array and a map with non default nullability among them) and casts with them, a session query over row(1, 2) runs in each state, a struct without field names is built through add in each state, a subclass with its own constructor and an attribute survives copy, deepcopy and pickle, and building a thousand distinct containers of named structs leaves the cache where it was. Without assertions and without the switch none of the five prints anything, and with assertions the kept connection prints its one instance without the switch, except on a free threaded interpreter, where nanobind compiles its leak check out. One test turns the report off in a child that keeps a connection and reads silence. One test reads the interpreter's state dict and finds nanobind's record under a key that ends in the duckdb domain. A last test checks _duckdb._assertions against the CMake cache of the build tree the module was loaded from, reading the build type and FORCE_ASSERT as CMake's generator expressions do; an installed wheel and a multi configuration tree skip it.

[el-marvin]

Changes requested.

I reviewed 13f90a7, the one commit on top of the merge base 4583a30 with v2.0-cyanoptera. I built the head and the merge base the same way (editable Release builds, CPython 3.13, Linux aarch64), and a third copy of the head with FORCE_ASSERT, the way the CI assertion job builds it; I also ran the reporters' scripts against the released 2.0.0 wheel and against 1.5.6. The premise holds. The four scripts in the description print the block on 2.0.0 and on the merge base and print nothing at the head; 1.5.6 prints nothing for any of them. What keeps the objects alive starts outside duckdb: on the merge base, typing.clear_overloads() at the end of the reporter's script, or polars._dependencies._might_be.cache_clear() after the polars conversion (that functools.cache holds duckdb._relation.DuckDBPyRelation), makes the exit silent. Turning the report off where users see it and keeping it where developers look is the right layer for this, and the flag only gates the printing: nanobind's internals_cleanup_one decides whether to free its state from its own leak count, never from the flag, so nothing is freed earlier or twice, and the exit under a daemon thread that holds an open result or runs a query is unchanged (exit status 0, no fatal error, under -X dev too). Two things must change before this lands. The change takes the exit leak checks of the slow shutdown suite out of every automatic CI job, and the flag it writes belongs to every nanobind extension in the process, not to duckdb alone.

Findings

1. No automatic CI job checks the exit leak report of the interpreter shutdown tests any more (tests/slow/test_interpreter_shutdown.py:70)

Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Keep these checks live in automatic CI: at least run tests/slow/test_interpreter_shutdown.py in the pull request's assertion job, or give the test children a private way to turn the report back on so that every wheel job keeps them.

The evidence is in the inline comment.

2. Importing a release duckdb turns off the exit leak report of every other nanobind extension that shares its nanobind state, and such an extension can turn off duckdb's (src/duckdb_python.cpp:723)

Severity: should fix. Category: design. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Give _duckdb its own nanobind domain, defined on _duckdb_dependencies so that the define reaches duckdb_python.cpp.

The evidence is in the inline comment.

3. The sdist check sits in a step that pull request runs skip, and the 3.11 wheels have none (.github/workflows/packaging_sdist.yml:98)

Severity: minor. Category: missing coverage. Evidence: traced. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Move the sdist check into the "Install sdist" step, which always runs, and add the same check to the cibuildwheel test command.

The evidence is in the inline comment.

4. Four sentences of the description do not hold as written

Severity: minor. Category: docs. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Correct these sentences before the description becomes the merge commit's text.

• "fixed at its root in #743": that pull request is open. At this head the callable cycle still keeps a connection and its database alive to the end of the process (a function defined in __main__ registered with create_function prints 28 instances and a keep_alive record on the merge base), and a release build now says nothing about it. Say it is fixed in the open #743.
• "that type keeps its methods and their default argument values alive" and "what is held are types and constants": the 27 DuckDBPyType constants are reached through the module globals of the 16 Python functions that duckdb/init.py:208 attaches to DuckDBPyConnection. On the merge base, deleting those 16 attributes from the kept type in an atexit handler makes the Py_IncRef script exit silently, while the nanobind methods and their defaults stay on the type. And the held set is not only types and constants: with the overloads defined in __main__, the script's own globals survive too. A connection to a file database in a global of the reporter's script (numpy imported, overloads defined) is never closed, the report counts 28 instances and a keep_alive record, and the .wal file is left beside the database on both builds (the rows come back on reopen). The report means that some duckdb Python object outlived finalization; say so.
• "a connection object still referenced at exit": a connection held in an ordinary module global prints nothing on 2.0.0; the trigger is a reference that is never dropped, as the test's Py_IncRef does.
• The ABI key parenthesis, see finding 2.

Smaller things

• The skip and the "nanobind: leaked" literal are written in three places (the two arrow tests inline, the shutdown helper), while the new file names the string REPORT. One shared helper would keep them in step, and finding 1 touches all three.
• The new file's _run has no NO_CORE_DUMP prefix, which its sibling in tests/slow/test_interpreter_shutdown.py:37 puts on the same kind of exit time child, so a crash at exit there writes a core file.
• The new check in packaging_wheels.yml:256 could share the interpreter launch of the line above it.

Outside findings not taken

• Leave the original assert ... not in stderr in place instead of skipping, so the test reports as passed: not taken. The skip shows in the run summary that the check did not run; a pass that cannot fail would hide it. The author's choice is the honest one.
• The two by construction cases are one case: not taken. One keeps the type, the other an instance with its keep_alive record (27 against 28 instances), the two shapes a held connection takes.
• Drop the CMake cache test or the workflow line that asserts the flag: not taken. The test is the only check of the Debug side ($<CONFIG:Debug>), which no CI job builds; the workflow lines are the only checks that do not take their expectation from the artifact itself.
• The silence test runs four scripts against one switch, so the four are one check: not taken. They are the reporters' reproducers and pin what users see.
• _needs is a third copy of a helper: not taken; the repository keeps such helpers per file.
• The engine's FORCE_DEBUG is not forwarded to the bindings: not taken. No build or workflow of this repository sets it, and the comment at src/duckdb_python.cpp:707 holds for the two options the loader forwards.
• The flag is set before duckdb_register_static_extensions(), so a failed import leaves it off: not taken; finding 2 confines it to duckdb either way.
• The convention check found no violations; its two borderline comments both state a reason and are not taken.

What I verified

• Head 13f90a7, merge base 4583a30, one commit, nine files, 182 lines added and 6 removed; no merge commit in the range.
• Builds: the head tree and the merge base clone, both CMAKE_BUILD_TYPE=Release, FORCE_ASSERT=OFF, CPython 3.13.18, gcc, editable; a clone of the head with FORCE_ASSERT=1 added to SKBUILD_CMAKE_DEFINE, as assert_test.yml does. The compile command of duckdb_python.cpp carries -DDUCKDB_STATIC_BUILD -DNDEBUG at the head and -DDUCKDB_FORCE_ASSERT -DDUCKDB_STATIC_BUILD -DNDEBUG in the assertion build; _duckdb._assertions is False and True. The released duckdb 2.0.0 (abi3, CPython 3.12) and 1.5.6 (CPython 3.13) from PyPI.
• Premise, every probe connection with 8 threads and an 8 GB memory limit: the overload with numpy, the polars conversion, the kept type and the kept connection print 27, 27, 27 and 28 instances on 2.0.0 and on the merge base (84 functions on 2.0.0, 85 on the merge base, as the description says) and nothing at the head or on 1.5.6; a connection in a module global prints nothing anywhere; the overload without numpy prints nothing.
• Tests, at most 8 workers with a 600 second timeout. Head Release, the new file, the two touched arrow tests and the shutdown file: 17 passed, 31 skipped. Merge base Release, the touched arrow tests and its own shutdown file: 41 passed. The new file against the merge base build (the attribute set to False by a conftest, since the base has none): 4 failed, 1 passed, 2 skipped, each failure with the block printed. FORCE_ASSERT head, same selection as the head: 44 passed, 4 skipped.
• Variants, all in the FORCE_ASSERT clone and each taken after the one before was undone. Report always off: 2 failed, 6 passed, 4 skipped, the two failures being test_objects_held_past_finalization_are_reported_with_assertions. Flag hard coded False: 5 failed, 2 skipped, the CMake cache test among them. So the tests tell the change from both weaker forms apart, in the assertion job. Own domain with the report always off, then own domain with the head's guard: the cross extension probes of finding 2, and the whole fast suite on the second: 17604 passed, 117 skipped, 164 xfailed in 709.20s. Of the FORCE_ASSERT results quoted for the head, 44 passed, 4 skipped was taken on the unedited clone before the first variant edit, and the probe where another extension silences duckdb after the first two variants were undone and the clone rebuilt; after the last variant the clone was restored and rebuilt, and the head results were taken again there: git diff HEAD empty, the compile command back to -DDUCKDB_FORCE_ASSERT -DDUCKDB_STATIC_BUILD -DNDEBUG, the kept connection script prints 28 instances and a keep_alive record, the other extension again silences duckdb's report, and the new file gives 3 passed, 4 skipped.
• Shutdown probes, merge base and head, under -X dev: a daemon thread parked on an open result, a daemon thread inside a long query, a UDF from __main__: exit status 0, no fatal error, the expected output, the report on the merge base only.
• Outside reviews: the code-review skill at high effort over the range (its ten finder reports came back through the coordinator and are saved beside this review), and Codex at xhigh effort, read only. Both raised findings 1, 2 and 3; the mechanism in the second item of finding 4 came from one of the code-review skill's finders, and I confirmed it with my own probe. The rest is listed above.
• duckdb_python.cpp compiles against the limited API (-DPy_LIMITED_API=0x030C0000, syntax only) at the head and at the merge base.
• CI at 13:19 UTC: no red check. Code quality, submodule sanity, both cp311 Linux sanity legs (which run the whole suite) and five of the six abi3 seed builds are green; the sdist build, the Linux x86_64 seed and the assertion job are still pending, so no abi3 test leg has started.

What held

• The define test: both macros reach duckdb_python.cpp through _duckdb_dependencies, and the engine's own DEBUG stays in the engine's directory scope (external/duckdb/CMakeLists.txt:1056).
• The flag changes printing and nothing else; the exit status and the output of every child above are the same on the merge base and the head.
• The two shutdown tests that were split still check both child processes, after every other check.
• _cmake_bool follows the $<BOOL:...> rule, $<CONFIG:Debug> compares case insensitively as the test does, and the cache lookup finds build/debug/CMakeCache.txt from the editable layout and nothing from an installed wheel.
• The free threaded skip matches nanobind, which compiles its leak report out under NB_FREE_THREADED.
• The quotes from wjakob/nanobind#644 and #1013 are verbatim.
• No hot path is touched: the change runs once at import.

[el-marvin]

No automatic CI job checks the exit leak report of the interpreter shutdown tests any more (tests/slow/test_interpreter_shutdown.py:70)

Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Keep these checks live in automatic CI: at least run tests/slow/test_interpreter_shutdown.py in the pull request's assertion job, or give the test children a private way to turn the report back on so that every wheel job keeps them.

Before this change the pull request gate ran them live: on_pr.yml calls packaging.yml with testsuite: all, so every abi3 test leg (Linux, macOS, Windows) and the cp311 sanity legs ran ./tests on the release wheel, and the assertion "nanobind: leaked" not in proc.stderr held. On the merge base Release build, the shutdown file and the two fast exit tests give 41 passed with the leak assertions live. At the head the same selection gives 25 shutdown tests and 4 fast cases skipped at the new skips (17 passed, 31 skipped together with the new file). The one build that keeps the report is assert_test.yml, and both automatic callers pass testsuite: fast (on_pr.yml:40, on_push.yml:40), which runs ./tests/fast. So the shutdown cases that keep relations, open results and Arrow scans alive at exit, threads finishing during shutdown and the checkpoint at exit are leak checked by no automatic job, and the fast arrow exit checks now run on one configuration (Linux x86_64, CPython 3.12, full API) instead of every platform, every Python version and the abi3 build. These are the checks that would catch the next real exit leak of the kind of the callable cycle the description mentions. On the FORCE_ASSERT head the file passes live: the touched selection gives 44 passed, 4 skipped in 20.07s at 8 workers.

The smallest fix is to add the file to the assertion job's fast selection. A private switch keeps more: a module function (or an environment variable read in NB_MODULE) that turns the report back on, which the test children call, would keep every existing leak check live in every wheel job on every platform, the skips at this line and in the two arrow tests would go away, and the four new silence cases would still pin the default.

[el-marvin]

Importing a release duckdb turns off the exit leak report of every other nanobind extension that shares its nanobind state, and such an extension can turn off duckdb's (src/duckdb_python.cpp:723)

Severity: should fix. Category: design. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Give _duckdb its own nanobind domain, defined on _duckdb_dependencies so that the define reaches duckdb_python.cpp.

nb::set_leak_warnings writes p->print_leak_warnings on the nb_internals record (nanobind 3.1.0, src/common.cpp:1150), and nb_module_init_impl shares one record per key __nb_internals_<ABI tag>_<domain>__ (src/nb_internals.cpp:1125); CMakeLists.txt:197 passes no NB_DOMAIN. I built a small nanobind 3.1.0 extension with the same compiler that leaks one instance on purpose. Alone, or with the merge base duckdb imported: nanobind: leaked 1 instances!. With the head Release duckdb imported, before or after it: the extension's own nb::leak_warnings() reads False and nothing is printed. In the other direction, on the FORCE_ASSERT head: the extension calls nb::set_leak_warnings(false), the call nanobind's author suggests in wjakob/nanobind#644, then duckdb is imported and its connection type is kept, and duckdb's report is gone. So test_objects_held_past_finalization_are_reported_with_assertions, and every live leak check of an assertion build, would pass or fail on what else the process imported. The description's answer asks other projects to call nb::set_leak_warnings(true) after importing duckdb.

In a variant with target_compile_definitions(_duckdb_dependencies INTERFACE DUCKDB_STATIC_BUILD NB_DOMAIN=duckdb), the extension's report prints in both import orders while duckdb's own stays off, and on the FORCE_ASSERT build duckdb's report prints even after the extension turned its own off. Passing NB_DOMAIN to nanobind_add_module is not enough: nanobind adds the define to the module target only (nanobind-config.cmake:549), and duckdb_python.cpp, where NB_MODULE expands, is compiled into the python_src object library. The whole fast suite passes on that variant built with FORCE_ASSERT: 17604 passed, 117 skipped, 164 xfailed in 709.20s. duckdb binds no type that another nanobind extension consumes, so the own domain costs nothing I could find. While at it, the description's parenthesis on who shares the key should add that the key carries the limited API: the abi3 wheel shares with other abi3 nanobind 3 extensions, the 3.11 wheel and editable builds with full API ones.

[el-marvin]

The sdist check sits in a step that pull request runs skip, and the 3.11 wheels have none (.github/workflows/packaging_sdist.yml:98)

Severity: minor. Category: missing coverage. Evidence: traced. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Move the sdist check into the "Install sdist" step, which always runs, and add the same check to the cibuildwheel test command.

The check is inside "Test sdist", which has if: ${{ inputs.testsuite != 'none' }}, and packaging.yml:79 passes testsuite: ${{ inputs.minimal && 'none' || inputs.testsuite }}, so pull request and merge queue runs never execute it; release runs do, before publishing. The 3.11 wheels are tested by CIBW_TEST_COMMAND (.github/actions/build-wheel/action.yml:138), which has no check, and there the tests take their expectation from the artifact's own flag: a 3.11 wheel built with assertions would skip the silence cases, pass the "reported" cases and skip the CMake cache test. Low risk while every wheel comes from one pyproject configuration, hence minor.

[el-marvin]

Changes requested.

I reviewed 7853c26, which adds one commit to 13f90a7, the head of the first round; the range from the merge base 4583a30 with v2.0-cyanoptera is now two commits, eleven files, 307 lines added and 3 removed. I rebuilt the head (editable Release build, CPython 3.13, Linux aarch64), moved my copy of the head that is built with FORCE_ASSERT (the way the CI assertion job builds) to the new commit and rebuilt it, and kept the released 2.0.0 wheel and the merge base build of the first round. The new commit answers all four findings of the first round, and I verified each by re-running its reproducer: a private _duckdb._set_leak_report(enabled) keeps every existing exit leak check live in every wheel job, and NB_DOMAIN=duckdb keeps the flag away from other nanobind extensions in both directions. But the premise check of this round overturns what I concluded in the first: the 27 instances that every reported case prints are not objects of other packages but duckdb's own module level copies of the type constants in duckdb/value/constant/init.py, which outlive finalization because that module's dict is held by the __init__ functions of its Value classes. Making that module hold the sqltypes module instead of the 27 names makes the reporter's script, the polars conversion and the kept type of #736 exit silently with the report on, and leaves a deliberately kept connection reported as the one instance it is. The description's "duckdb cannot release these objects itself" does not hold, and turning the report off in every release build is containment at the wrong layer for the reported cases. That root fix, with tests that run the reported cases with the report on, has to be in this pull request; whether release builds should still hide the report for objects a program itself keeps past finalization is then a separate decision for the maintainer, and the switch, _duckdb._assertions and the CI configuration checks stay or go with it. The own domain stays either way: it also stops duckdb's exception translator from rewriting other nanobind extensions' C++ exceptions, which I confirmed with a second probe extension, and that is worth keeping on its own. Beyond that, one minor item. At 15:15 UTC CI had no red check: the sdist build and the assertion job were green, the cp311 sanity legs and the abi3 test legs not yet run.

Since the last review

• Finding 1, no automatic CI job checked the exit leak report of the shutdown tests: addressed. The touched selection of the first round on the head Release build went from 17 passed, 31 skipped to 51 passed, 2 skipped; the four touched files whole give 680 passed, 2 skipped on Release and 678 passed, 4 skipped on the FORCE_ASSERT build, the skips being the two "reported with assertions" cases and the four silence cases, nothing else. The pull request gate calls packaging.yml with testsuite: all (on_pr.yml:31), so the wheel legs run ./tests, slow suite included, with the checks live. All 21 children whose exit is checked as leak free call the switch after their own import of duckdb, and _assert_clean_exit fails a child checked without it (tests/slow/test_interpreter_shutdown.py:60). With the switch made a no-op in the FORCE_ASSERT copy, test_the_switch_turns_the_report_off_in_every_build fails twice; with it removed, 34 tests of the selection fail; on Release a no-op switch leaves the report off, which test_the_switch_turns_the_report_on_in_every_build fails on (traced: those children print nothing on the head Release build without the switch, and 27 and 28 instances with it). The switch is the larger of the two fixes I offered and better than adding the slow file to the assertion job.
• Finding 2, importing a release duckdb turned off the report of every other nanobind extension sharing its state: addressed. The compile command of src/duckdb_python.cpp carries -DNB_DOMAIN=duckdb on both builds. With my probe extension (nanobind 3.1.0, same compiler, leaks one instance on purpose), on the head Release build the probe reports its instance alone and in both import orders and its flag stays True; duckdb with its type kept prints nothing, also after the probe turned its own flag on or off first; after the probe turned its flag off, _duckdb._set_leak_report(True) prints duckdb's 27 instances and leaves the probe's flag False; after _duckdb._set_leak_report(False) the probe still reports. On the FORCE_ASSERT build duckdb's report prints after the probe turned its own flag off. The interpreter state dict holds two nanobind records, one ending _v22___ and one ending _v22_duckdb__. With the define removed the two merge and the probe silences duckdb again; test_the_module_has_its_own_nanobind_domain fails then, and also when the domain is given to nanobind_add_module instead, which does not reach the object library. The domain also isolates the exception translators: a second probe extension whose function throws std::bad_alloc raises MemoryError alone, _duckdb.OutOfMemoryException once the merge base duckdb is imported (its catch all translator in src/common/exceptions.cpp:408 turns any std::exception that ErrorData recognizes into a duckdb error), and MemoryError again with the head imported. That fixes a defect of the released module the description does not mention; better than what I asked for.
• Finding 3, the sdist check sat in a step pull request runs skip and the 3.11 wheels had none: addressed, by reading. The check runs in "Install sdist" (.github/workflows/packaging_sdist.yml:89), which has no condition, and the folded CIBW_TEST_COMMAND (.github/actions/build-wheel/action.yml:139) parses to uv run python -c "import _duckdb; assert not _duckdb._assertions, ..." && uv run -v pytest ..., so a failed assertion stops the suite and fails the command on sh and cmd alike. The sdist build of this head is green, so the moved check ran and held in a pull request run; the cp311 sanity legs, which run the cibuildwheel command, were still pending.
• Finding 4, four sentences of the description: addressed as asked. #743 is described as open, and it is; the connection in an ordinary global prints nothing on 2.0.0, as the sentence now says; the key parenthesis names the limited API and free threading; the mechanism sentence names the 16 functions of CONNECTION_METHODS (duckdb/_relation.py:4436), bound at duckdb/init.py:208. That last sentence now says the 16 functions' "module globals hold the 27 DuckDBPyType constants", which is not quite what I measured: duckdb._relation's globals hold none of them, they reach them through duckdb._expression and its import of Value; see finding 1.
• Corrected: in the first round I wrote that what keeps the objects alive starts outside duckdb and that turning the report off where users see it is the right layer. The outer holders (typing's registry kept by numpy, the polars cache) are outside duckdb, but what they reach is duckdb's own: a module dict that binds the 27 constants by name. I traced the path to duckdb._relation and stopped there; I should have asked which dict holds the instances. Finding 1 replaces that judgment.

Findings

1. The 27 instances that every reported case prints are duckdb's own copies of the type constants in duckdb/value/constant/init.py, so duckdb can release them itself, and the change hides the report instead (src/duckdb_python.cpp:720)

Severity: blocking. Category: design. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: widened.

Make duckdb/value/constant/init.py hold the sqltypes module instead of the 27 constants, pin it with tests that run the reported cases with the report on in every build, correct the comment at this line and the description, and argue any silencing of release builds that stays as a policy of its own.

The evidence is in the inline comment on src/duckdb_python.cpp:720.

2. If the silencing stays, the empty stderr check of the sys.exit map child no longer sees the report in builds without assertions (tests/fast/test_map.py:776)

Severity: minor. Category: missing coverage. Evidence: traced. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Have _SYS_EXIT_CHILD call the switch after its import, like the three children the change already updated.

test_map_function_calling_sys_exit_raises_instead_of_ending_the_process asserts child.stderr == "" (line 776), which on the merge base also failed when the SystemExit path left a duckdb object alive at exit, in every build. _SYS_EXIT_CHILD (line 694 on) imports duckdb and never calls _duckdb._set_leak_report, so in the wheel jobs that check now passes for a leak too; it stays live only in the assertion job. It is the only child process test in the suite that asserts an empty stderr. Moot if finding 1 ends with the report on in release builds.

Smaller things

• The description gives the leak flag as the reason for the own domain; the exception translator effect above is the stronger reason and could be named there.
• The key parenthesis in the description lists the internals version, the compiler family and standard library, the limited API, free threading and the domain; nanobind's key (nb_internals.h:93, nb_platform.h:96) also carries its backend ABI major (_a1) and a tag for MSVC debug and _GLIBCXX_DEBUG builds. "among others" would make it exact.

Questions

• The torch variant of the overload case in the description is the same mechanism (typing's globals kept alive), but torch is not installed here, so I did not run it with the root fix. If the test group has torch, the new tests could include it.

Outside findings not taken

• Codex, xhigh effort, read only: no findings, recommended approval. It read the saved logs of this round, so the test figures in its report are mine.
• One of the code-review skill's finders found finding 1, with the probe I repeated; another found finding 2 (I did not rerun its child, hence traced). Its point that the own domain is containment for the switch is part of finding 1.
• The comment above the define in CMakeLists.txt is five lines: not taken; it says why the define exists and where it must sit, as its neighbours do.
• One switch test per build would do, saving under half a second per job: not taken; both directions are stated in every build.
• Read the build's compile commands instead of the CMake cache in the configuration test: not taken; a drift fails loudly, and the end to end check from configuration to attribute is deliberate.
• The three YAML copies of the configuration check: not taken; each runs against a different artifact, and the sdist one must run where no test suite runs, which was the first round's finding 3.
• Append the switch after the body in _run instead of 21 copies and a text check: not taken; a suffix silently does not run in a child that leaves after BODY_DONE or raises, where the call right after the import plus the check that it is there cannot be skipped.
• Shared helpers in conftest.py, and the four tests' repeated three line body: not taken; the repository keeps such helpers per file, and the rest is style.
• The macOS universal2 wheels never get the configuration check: not taken; the description says so, and their slices are the checked arm64 and x86_64 wheels.
• Two finders found nothing. The skill's verifiers confirmed finding 1, finding 2 and the universal2 gap, refuted the single switch test and the folding of the YAML checks, and called the suffix form plausible; their remark that the text check also passes for a call under if False is about code nobody writes. One verifier pointed out the exception translator effect of the domain, which I probed (above).

What I verified

• Head 7853c26, merge base 4583a30, two commits, no merge commit; the engine pin is unchanged (910c73da).
• Builds: the head tree, editable, CMAKE_BUILD_TYPE=Release, FORCE_ASSERT=OFF, _duckdb._assertions False; the copy of the head with FORCE_ASSERT=1 added to SKBUILD_CMAKE_DEFINE, _duckdb._assertions True. Compile command of src/duckdb_python.cpp: -DDUCKDB_STATIC_BUILD -DNB_DOMAIN=duckdb -DNDEBUG and -DDUCKDB_FORCE_ASSERT -DDUCKDB_STATIC_BUILD -DNB_DOMAIN=duckdb -DNDEBUG. The released 2.0.0 wheel (abi3, CPython 3.12) and the merge base build of the first round.
• Premise, every probe connection with 8 threads and an 8 GB memory limit: the overload with numpy, the polars conversion, the kept type and the kept connection print 27, 27, 27 and 28 instances on 2.0.0 and on the FORCE_ASSERT head, and nothing on the head Release build; a connection in a module global prints nothing anywhere; with from __future__ import annotations the reporter's script exits silently on 2.0.0 and on the merge base, as the description says.
• A child that keeps a connection and leaves through ctypes.CDLL(None).exit(0) prints no report on either build with the switch on, where the same child exiting normally prints 28 instances and a keep_alive record: the C exit child's check could never fail, as the description says.
• Tests, at most 8 workers with a 600 second timeout, from the tree root. The selection of the first round (the new file, the two arrow exit tests, the slow shutdown file): 51 passed, 2 skipped in 20.29s on Release. The four touched files whole: 680 passed, 2 skipped in 40.27s on Release, 678 passed, 4 skipped in 34.33s on FORCE_ASSERT. Because the own domain touches the whole module, the whole fast suite on the head Release build: 17611 passed, 115 skipped, 164 xfailed in 843.97s, the figure the author gives.
• Mutations, all in the FORCE_ASSERT copy after the measurements above, each undone before the next: the domain define removed, 1 failed, 7 passed, 4 skipped on the new file (the domain test); the domain given to nanobind_add_module only, the same; the switch a no-op, 2 failed, 47 passed, 4 skipped on the selection; the switch removed, 34 failed, 15 passed, 4 skipped; the value module holding sqltypes instead of the 27 names, the results in finding 1. Restored afterwards: git diff HEAD empty, the compile command as above, the new file 8 passed, 4 skipped.
• Exception translators: a second probe extension (round 2, nanobind 3.1.0, same compiler) with functions that throw std::bad_alloc and std::runtime_error, run alone and with duckdb imported, on the merge base and the head builds; results in What held.
• src/duckdb_python.cpp compiles against the limited API (-DPy_LIMITED_API=0x030C0000, syntax only) at the head.
• The private names: duckdb exposes neither _set_leak_report nor _assertions; the stub declares _set_leak_report(enabled: bool) -> None, matching nanobind's signature.
• The workflow edits, parsed as YAML: the heredoc in packaging_wheels.yml loses its block indentation, so EOF ends it at column 0, under set -eo pipefail.
• Outside reviews: Codex at xhigh effort, read only, over the range; the code-review skill at high effort over the range, whose ten finder reports I read from its folder and saved beside this review (its own summary had not returned when I wrote this).
• CI at 15:15 UTC: code quality, submodule sanity, the sdist build, the assertion job and five of the six abi3 seeds green; the macOS x86_64 seed and the two cp311 sanity legs pending; the abi3 test legs not started, so the shutdown leak checks have not yet run live on Windows or macOS. The job logs cannot be read until the run completes. The first round's head had its abi3 test legs cancelled by this push.

What held

• The define reaches every binding object through _duckdb_dependencies; nanobind reads it only where NB_MODULE expands (nb_defs.h:224) and in register_module (nb_misc.h:191), which duckdb does not call, and its static library takes the domain as an argument.
• The switch changes printing only: nanobind decides whether to free its state at exit from its own leak count, never from the flag (internals_cleanup_one), so turning the report on in the children changes nothing they check besides the report.
• The child of the arrow test that imports duckdb inside mock.patch.dict(sys.modules) calls the switch inside the block and never imports duckdb again, so module initialization does not run a second time and reset the flag.
• The own domain keeps duckdb's translator to duckdb's functions: another nanobind extension's std::bad_alloc stays MemoryError with the head imported, where the merge base turned it into _duckdb.OutOfMemoryException; a plain std::runtime_error passes through as RuntimeError on both.
• The ctypes read of the interpreter state dict uses pointer sized return types, takes the borrowed dict through .value, and restores the changed restype and argtypes in a finally.

[el-marvin]

The 27 instances that every reported case prints are duckdb's own copies of the type constants in duckdb/value/constant/init.py, so duckdb can release them itself, and the change hides the report instead (src/duckdb_python.cpp:720)

Severity: blocking. Category: design. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: widened.

Make duckdb/value/constant/init.py hold the sqltypes module instead of the 27 constants, pin it with tests that run the reported cases with the report on in every build, correct the comment at this line and the description, and argue any silencing of release builds that stays as a policy of its own.

duckdb/value/constant/init.py:4 binds from duckdb.sqltypes import (BIGINT, BIT, ..., VARCHAR, DuckDBPyType), exactly 27 DuckDBPyType constants, into its globals, and the __init__ functions of the Value classes that pass them (super().__init__(object, INTEGER) at line 96 and 26 like it) keep that dict alive through __globals__. duckdb/_expression.py:34 imports Value, duckdb/_relation.py:28 imports from duckdb._expression, and the 16 functions duckdb/init.py:208 attaches to DuckDBPyConnection carry duckdb._relation's globals, so anything that keeps that type or one of those functions alive reaches the dict. Counting DuckDBPyType values in the globals of every duckdb module at the head finds three: _duckdb._sqltypes 29, duckdb.sqltypes 29, duckdb.value.constant 27. CPython clears the dict of a module object that is still alive at finalization, not a dict whose module object is gone while functions hold it, so these 27 are the ones that survive.

In the FORCE_ASSERT copy, where the report is on by default, I rewrote that import to from duckdb import sqltypes and the 27 calls to sqltypes.INTEGER and so on (a labelled mutation, after every pristine measurement, undone afterwards). The overload with numpy, the polars conversion and the kept type all exit silently. The kept connection prints nanobind: leaked 1 instances! of type _duckdb.DuckDBPyConnection and one keep_alive record instead of 28 instances. nanobind lists types and functions only when an instance or a keep_alive record leaked ("Only report function/type leaks if actual nanobind instances were leaked", if (!leak) print_leak_warnings = false; in internals_cleanup_one), so a kept type alone stays silent. IntegerValue, StringValue, NullValue and TimestampValue still bind, and the new file with the ten test files that use duckdb.value gives 2 failed, 1412 passed, 4 skipped, 132 xfailed, the two failures being the connection_type_kept cases that expect a report from a kept type alone.

So the comment at this line ("Objects that another package keeps alive past finalization fill nanobind's exit report") and the description's "The holders belong to other packages, so duckdb cannot release these objects itself" do not hold: the outer holders keep a dict alive, and what that dict holds is duckdb's choice. With the root fix #732 and #736 are fixed in every build with the report on, and the report keeps naming real leaks precisely. What the silencing would still hide is an object a program itself keeps past finalization: with the fix, the reporter's script with a file database connection in a global prints that one connection and its keep_alive record (then the 6 types and 85 functions, as nanobind lists them once an instance leaked) and still leaves the .wal beside the database. Whether release builds should hide that is the separate decision; if the answer is no, the switch, _duckdb._assertions and the CI configuration checks have no job left (the own domain keeps one, see What held); if yes, the description argues it on that ground. The tests as written cannot tell the root fix from the switch: the reporters' cases run only with the report off (tests/fast/test_exit_leak_report.py:111), and CONNECTION_KEPT, a real leak, is asserted to print nothing in release builds. Tests that run the overload, polars and kept type cases with the report on and assert no report, and the kept connection with exactly one _duckdb.DuckDBPyConnection instance, would; a test that no duckdb module binds a DuckDBPyType by name outside _duckdb._sqltypes and duckdb.sqltypes would keep a later module from bringing the fan out back.

[el-marvin]

Changes requested.

I reviewed 0aa614f, which adds one commit to 7853c26, the head of the second round; the range from the merge base 4583a30 with v2.0-cyanoptera is now three commits, sixteen files, 580 lines added and 138 removed. I rebuilt the head (editable Release build, CPython 3.13, Linux aarch64), moved my copy of the head that is built with FORCE_ASSERT (the way the CI assertion job builds it) to the new commit and rebuilt it, and kept the released 2.0.0 wheel and the merge base build. The new commit takes the root fix the second round asked for, and for the Value classes it holds: with nanobind's report on, the reporter's overload with numpy and with torch, the polars conversion and the kept connection type exit silently, a kept connection is reported as the one _duckdb.DuckDBPyConnection instance it is, and no module but the two sqltypes modules binds a type constant. That is the right layer. What blocks is the spark half of the same commit: DataType.duckdb_type now builds DuckDBPyType(name) on every read, which parses the name on duckdb's default connection inside a transaction, so the spark API fails while another thread runs a query on the default connection or after a statement failed inside its transaction, both of which work on the merge base, and createDataFrame with a nested column takes more than twice as long. A cache owned by a module object, read through a module reference as the Value classes now read sqltypes, removes both and keeps the exit silent; I prototyped it. The new tests would not catch the other obvious cache, one kept on the instance, because no child uses a spark type before it keeps what it keeps, and the forwarding that keeps from duckdb.value.constant import INTEGER working has no test. Whether release builds should still hide the report for objects a program itself keeps past exit is with the maintainer and is not part of this verdict. CI at 19:44 UTC had no red check: 25 checks green, the four macOS abi3 test legs pending.

Since the last review

• Finding 1, the 27 instances were duckdb's own copies of the type constants in duckdb/value/constant/init.py: addressed for the Value classes, verified by rerunning the second round's reproducers on the new head. With the report on (the private switch on the Release build; no switch needed on the FORCE_ASSERT build) the overload with numpy, the overload with torch, the polars conversion and the kept connection type print nothing, where 2.0.0 and the merge base print 27 instances each; the kept connection prints nanobind: leaked 1 instances! of type _duckdb.DuckDBPyConnection and one keep_alive record, where they print 28. A file database connection in a plain module global prints nothing and leaves no .wal; the same connection behind the overload is reported as that one instance and leaves its .wal, as the description now says. Counting DuckDBPyType values in the module globals gives _duckdb._sqltypes 29 and duckdb.sqltypes 29 and nothing else, where the merge base also had 27 in duckdb.value.constant. The reported cases now run with the report on in every build, and the comment at src/duckdb_python.cpp:720 and the description give the chain as it is. The two spark modules that bound constants by name, which the code-review skill named after the last review, are fixed the same way, and both new guards catch them; how the spark types were fixed is finding 1 below.
• Finding 2, the empty stderr check of the sys.exit map child: addressed. _SYS_EXIT_CHILD turns the report on (tests/fast/test_map.py:700 to 703); the child as in the test exits with an empty stderr on both head builds, and with a connection kept it prints the one instance on the Release build too.
• The two smaller things are in the description (the exception translator as the stronger reason for the own domain, "among others" in the key sentence). The question on torch is answered: the test group has torch, and overload_signature_with_torch ran in both selections below instead of skipping.

Findings

1. Reading a spark type now parses its name on duckdb's default connection, so the spark API fails while that connection is busy or after its transaction failed, and nested createDataFrame is more than twice as slow (duckdb/experimental/spark/sql/types.py:79)

Severity: blocking. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Build each DuckDBPyType once per type name and keep it in a dict owned by a module object that the spark types hold as a module, as the Value classes now hold sqltypes, instead of building it on every read; add tests that read an existing spark type and cast to it while the default connection is busy on another thread and after a statement failed in its transaction; and give before and after figures for createDataFrame with a nested column.

The evidence is in the inline comment on duckdb/experimental/spark/sql/types.py:79.

2. No child uses a spark type before it keeps what it keeps, so a cache filled on the first read passes every new test while it brings the report back, and the module dict child does not see a holder in duckdb.value.constant (tests/fast/test_exit_leak_report.py:111)

Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Add a child that casts to a few singleton spark types and runs a createDataFrame with a nested schema, then keeps a spark function alive past exit with the report on and must print nothing; and narrow the description's "which catches a holder of any shape" and the comment at line 197 to what the module dict child shows.

The evidence is in the inline comment on tests/fast/test_exit_leak_report.py:111.

3. Nothing tests the forwarding that keeps the 27 constants importable from duckdb.value.constant (duckdb/value/constant/init.py:275)

Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Add a test that from duckdb.value.constant import INTEGER and attribute access give the sqltypes objects for the 27 names, that dir() lists them, that TIME_NS and an unknown name raise AttributeError, and that from duckdb.value.constant import * gives the names of __all__ as before.

The evidence is in the inline comment on duckdb/value/constant/init.py:275.

Smaller things

• DataType.duckdb_type is now a property without a setter, so assigning it raises AttributeError: property 'duckdb_type' of 'IntegerType' object has no setter, where the merge base allowed it. Nothing in the tree assigns it, and a subclass that still passes a DuckDBPyType to DataType.__init__ keeps working. The description could name it as a change of the experimental API.
• IntegerType().duckdb_type is IntegerType().duckdb_type is now False; nothing in the tree depends on that identity, and a cache as in finding 1 makes it True again.
• Pickling a spark type now works (IntegerType(), DecimalType(10, 2), ArrayType(IntegerType()) and a StructType round trip), where the merge base raised TypeError: cannot pickle '_duckdb._sqltypes.DuckDBPyType' object. A cache keyed by name keeps that.

Questions

• A class attribute holding a type constant, appended to each of the 53 module files in turn, makes no child of the new file print the report for 29 of them. For the two of those I checked against a real outside holder, the spark functions and types modules with a kept spark function or an overload naming Column, the holder does not leak in the real program either, because those module objects stay alive to finalization and CPython clears their dicts; so I do not count the 29 as a finding. A child per module that keeps only that module's dict would model a dict whose module object died, and would settle whether any of the rest can leak.

Outside findings not taken

• Codex, at xhigh effort and read only, found the default connection dependence of finding 1 and the missing forwarding test of finding 3 by reading; the probes above confirm both, and both are taken.
• Codex: the module dict child does not protect against a class attribute or a default argument in a module whose module object stays alive. Taken only for duckdb.value.constant, which I confirmed, as the second half of finding 2; not in general, since a by-name import, a class attribute and a default argument in duckdb/_expression.py and a class attribute in the spark column module each make the child print the report.
• The code-review skill was not run in this round.

What I verified

• Head 0aa614f, merge base 4583a30, three commits, no merge commit. The delta since the second round touches seven files: the value constant module, the spark types, column and functions modules, the comment in src/duckdb_python.cpp, the new test file and test_map.py.
• Builds: the head tree, editable, CMAKE_BUILD_TYPE=Release, _duckdb._assertions False; my copy of the head with FORCE_ASSERT=1 added to SKBUILD_CMAKE_DEFINE, _duckdb._assertions True, equal to the head for every measurement except the labelled changes below, each undone with an empty git diff HEAD afterwards; the merge base build; the released 2.0.0 wheel (abi3, CPython 3.12). Probes ran in virtual environments that load the head and merge base builds without rebuilding them.
• State of each result: the Release results ran on the head tree at 0aa614f; the FORCE_ASSERT results ran on my copy at 0aa614f with git diff HEAD empty, except under the changes listed below, each applied after the measurements of the unchanged copy and undone before the next; every change was to Python files, so the copy needed no rebuild between them.
• Premise and earlier reproducers, every probe connection at 8 threads and an 8 GB memory limit: the overload with numpy and with torch, the polars conversion, the kept connection type, the kept connection, a connection in a global, a file database connection in a global and behind the overload, the module dict child, an overload naming the spark Column, and a kept spark function, on 2.0.0, the merge base, the head Release build with and without the switch, and the FORCE_ASSERT copy. torch is installed beside neither the released wheel nor the merge base build, so the torch case ran on the head builds only.
• Cross extension: my probe nanobind extension reports its own leak alone and in both import orders on both head builds, duckdb with its type kept prints nothing next to it, the two nanobind state records end in _v22___ and _v22_duckdb__, and another extension's std::bad_alloc raises MemoryError with the head imported.
• Tests, at most 8 workers with a 600 second timeout. The selection (the new file, test_map, the two arrow exit test files, the slow shutdown file, test_value, test_expression, tests/fast/expression, test_appender_values, tests/fast/spark, test_non_default_conn, test_expression_compat_rules, test_object_column_conversion, test_seam, test_duckdb_connection): 4011 passed, 8 skipped, 134 xfailed in 118.52s on Release and 4006 passed, 13 skipped, 134 xfailed in 71.87s on FORCE_ASSERT, the difference being the six silence cases against the one assertion case. The whole fast suite on the Release build: 17617 passed, 114 skipped, 164 xfailed in 794.27s.
• Changes in the FORCE_ASSERT copy, each run on the case scripts and the new test file and then undone: the value module back to its by-name import, 7 failed, 4 passed, 6 skipped, with the module dict child silent; the spark singletons holding a type again, 1 failed (the module dict child); the spark column and functions modules back to their by-name imports, 2 failed (the static walk and the module dict child); all three, 8 failed, 3 passed; a constant imported by name in duckdb/_expression.py, 8 failed; a class attribute there, 7 failed; a default argument there, 7 failed; a class attribute in the spark column module, 1 failed; a class attribute in the value module, 6 failed, with the module dict child silent; a class attribute in the spark functions or types module, everything passing and the real programs silent too; the forwarding removed and the cached property, as in findings 3 and 2; the module owned cache, as in finding 1.
• Spark types on head and merge base: the two default connection states above, identity, equality and hashing (the same answers on both), pickling, assignment, and a closed default connection, after which reads work on both builds because a new default connection opens.
• mypy with the project's configuration on a snippet of the import forms gives the same five errors at the same lines against the head and the merge base.
• The nanobind quotes in the description are in wjakob/nanobind#644 and #1013, and #743 is open.
• Outside reviews: Codex at xhigh effort, read only, over the range, three findings, triaged above.
• CI at 19:44 UTC on 0aa614f: code quality, submodule sanity, the sdist build, the assertion job, both cp311 sanity legs, the six seeds, the twelve abi3 test legs on Linux and Windows and the pandas 2 leg green; the four macOS abi3 test legs pending.

What held

• The Value classes read sqltypes.NAME at call time; IntegerValue(3).type and the others give the types they gave before, and duckdb.value.constant binds no DuckDBPyType.
• The TYPE_CHECKING split leaves type checkers where they were: the same five mypy errors for an explicit import of a constant, attribute access, TIME_NS, an unknown name and a misspelt class name, against both trees.
• The forwarded set is exactly the 27 names the module imported before; TIME_NS and VARIANT, which sqltypes has and the module never imported, raise.
• The spark singletons keep their identity (StringType() is StringType()), and equality and hashing of spark types give the same answers as on the merge base.
• A deliberately kept connection is reported as exactly one instance with its keep_alive record on both head builds; the abi3 2.0.0 wheel prints the counts without instance lines, as the comment in the new test says.
• The comment above the silencing (src/duckdb_python.cpp:720) now names the condition under which a kept connection is reported, and the call sits at the start of NB_MODULE, as the description says.

[el-marvin]

Reading a spark type now parses its name on duckdb's default connection, so the spark API fails while that connection is busy or after its transaction failed, and nested createDataFrame is more than twice as slow (duckdb/experimental/spark/sql/types.py:79)

Severity: blocking. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Build each DuckDBPyType once per type name and keep it in a dict owned by a module object that the spark types hold as a module, as the Value classes now hold sqltypes, instead of building it on every read; add tests that read an existing spark type and cast to it while the default connection is busy on another thread and after a statement failed in its transaction; and give before and after figures for createDataFrame with a nested column.

    @property
    def duckdb_type(self) -> DuckDBPyType:  # noqa: D102
        # Built on access: an instance held here would sit in the singleton cache, which outlives finalization.
        return DuckDBPyType(self._duckdb_type_name)

DuckDBPyType(str) is not a plain allocation. FromString in src/typing/pytype.cpp:128 takes DuckDBPyConnection::DefaultConnection() and calls DuckDBPyType::ParseTypeName(*pycon->con.GetConnection().context, type_str); GetConnection() refuses while the connection runs a call (ThrowIfBusy, src/include/duckdb_python/pyconnection/pyconnection.hpp:132), and ParseTypeName (src/typing/pytype.cpp:333) runs inside context.RunFunctionInTransaction, which throws when the connection's transaction is aborted. On the merge base each singleton parsed its name once, at its first construction, and ArrayType, MapType and DecimalType built their types with duckdb.list_type, duckdb.map_type and duckdb.decimal_type, which touch no connection state (src/pyconnection/type_creation.cpp). Now every read parses: an atomic type directly, DecimalType through its name DECIMAL(10,2), and ArrayType, MapType, StructType and StructField through their leaves.

A spark session has its own connection, so its work should not depend on the state of the default connection. With the session, a DataFrame, IntegerType() and DecimalType(10, 2) created and read first, and every probe connection at 8 threads and an 8 GB memory limit, I put the default connection in two states. "Busy" is duckdb.sql("select count(*) from range(40000000000) t(i) where i % 7 = 3").fetchall() running on another thread; "failed" is duckdb.execute("BEGIN") followed by duckdb.execute("SELECT error('boom')").

• IntegerType().duckdb_type: merge base INTEGER in both states; head ResourceInUseException when busy, TransactionException when failed.
• DecimalType(10, 2).duckdb_type, read once before: merge base DECIMAL(10,2) in both states; head the same two exceptions.
• F.col("a").cast(IntegerType()): merge base Column<'CAST(a AS INT)'> in both states; head the same two exceptions.
• df.select(F.col("a").cast(IntegerType())).collect(): merge base [Row(a=1)] in both states; head the same two exceptions.
• spark.createDataFrame([(2, [3])], schema).collect() with the schema (a int, b array<int>): merge base [Row(a=2, b=[3])] in both states; head the same two exceptions.

The messages are Resource In Use Error: connection is busy with another call; it refuses instead of waiting, ... and TransactionContext Error: Current transaction is aborted (please ROLLBACK). After ROLLBACK the head works again.

The cost: createDataFrame converts each nested cell with duckdb.Value(value, data_type.duckdb_type) (duckdb/experimental/spark/sql/session.py:69, called per cell from line 287), so the parse now runs once per cell. For 20,000 rows with the schema (a int, b array<int>), one thread, best of five, Release builds of head and merge base made the same way, alternating: merge base 433 and 421 ms, head 975 and 955 ms (two earlier runs of this round: 484 and 430 ms against 994 and 1074 ms); user space instructions of the calling thread, fewest of five: 4,353 million on the merge base against 7,781 million on the head, the same in both runs. With a flat schema (a int, b string) the builds are equal: 387 and 388 ms against 392 and 390 ms, 4,089 million instructions on both. One read of IntegerType().duckdb_type went from 0.04 to about 22 microseconds, and one of a three field StructType from about 1.9 to about 77 microseconds.

What I prototyped in my FORCE_ASSERT copy and then threw away: a module _type_cache.py beside types.py holding CACHE: dict = {}, from . import _type_cache in types.py, and the property returning _type_cache.CACHE[name], built on the first read of that name. With it, all five calls above work in both states. A program that casts to IntegerType and StringType and then keeps F.length alive past exit, a kept F.length alone, an overload naming the spark Column with numpy imported, the module dict child and the reporter's overload exit silently with the report on, and the kept connection still prints its one instance. The nested createDataFrame takes 537 ms on that build against 1,102 ms for the head as it is (flat: 464 against 451 ms), and the new test file with tests/fast/spark gives 1862 passed, 13 skipped, 2 xfailed. The dict belongs to a module whose module object the types module holds, so when finalization finds that module alive it clears its dict; that is the same reason the sqltypes reference releases the Value classes' constants. The first read of a name still parses on the default connection, as the merge base did at a singleton's first construction. Caching on the instance instead, with functools.cached_property, brings the leak back; see finding 2.

[el-marvin]

No child uses a spark type before it keeps what it keeps, so a cache filled on the first read passes every new test while it brings the report back, and the module dict child does not see a holder in duckdb.value.constant (tests/fast/test_exit_leak_report.py:111)

Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Add a child that casts to a few singleton spark types and runs a createDataFrame with a nested schema, then keeps a spark function alive past exit with the report on and must print nothing; and narrow the description's "which catches a holder of any shape" and the comment at line 197 to what the module dict child shows.

In my FORCE_ASSERT copy, with DataType.duckdb_type turned into a functools.cached_property (the type built on the first read and then kept on the instance, so the singletons in DataTypeSingleton._instances hold one again), tests/fast/test_exit_leak_report.py and tests/fast/spark pass: 1862 passed, 13 skipped, 2 xfailed. Yet this program prints nanobind: leaked 2 instances!, both of type _duckdb._sqltypes.DuckDBPyType:

import ctypes

import duckdb
from duckdb.experimental.spark.sql import functions as F
from duckdb.experimental.spark.sql.types import IntegerType, StringType

F.col("a").cast(IntegerType())
F.col("b").cast(StringType())
ctypes.pythonapi.Py_IncRef(ctypes.py_object(F.length))

At the head it prints nothing with the report on, and on the merge base 44 instances. EVERY_MODULE_DICT_KEPT imports every module but never reads a type, so nothing it keeps holds a type that is built on use, and no other child touches the spark API. Caching on the instance is the obvious way to stop the reparsing of finding 1, so the tests should tell it apart from a cache that finalization clears.

The description says the module dict child "catches a holder of any shape, including the spark cache", and the comment at line 197 says it "checks the whole dict". It does catch a holder bound at import in several modules: a constant imported by name, a class attribute or a default argument in duckdb/_expression.py, a class attribute in the spark column module, the merge base's spark singleton cache and the spark modules' by-name imports each make it print the report. But it prints nothing when duckdb/value/constant/init.py imports the 27 constants by name again, the defect of the second round, or holds one in a class attribute. Keeping the dict of the duckdb.value package keeps the constant module object alive, and CPython clears the dict of a module object that is still alive at finalization, so the child keeps less than the reporter's programs do. The children of the reported cases catch both, so that module is covered; the sentence and the comment claim more than the child shows.

[el-marvin]

Nothing tests the forwarding that keeps the 27 constants importable from duckdb.value.constant (duckdb/value/constant/init.py:275)

Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

Add a test that from duckdb.value.constant import INTEGER and attribute access give the sqltypes objects for the 27 names, that dir() lists them, that TIME_NS and an unknown name raise AttributeError, and that from duckdb.value.constant import * gives the names of __all__ as before.

With the module __getattr__ and __dir__ removed in my FORCE_ASSERT copy, from duckdb.value.constant import INTEGER raises ImportError: cannot import name 'INTEGER' from 'duckdb.value.constant' and dir() no longer lists INTEGER, and the selection of this round passes with exactly the counts it has without the removal: 4006 passed, 13 skipped, 134 xfailed. The tests that use the module import the Value classes from it and the constants from duckdb.sqltypes, and the classes themselves now read sqltypes, so nothing reaches the forwarding. At the head the forwarding does what the description says: the 27 names resolve to the same objects as in sqltypes, dir() lists them, TIME_NS raises AttributeError, and the star import gives the 29 names of __all__, as on the merge base.

[el-marvin]

Changes requested. I reviewed 5d354fd, two commits on top of round 3's 0aa614f: dfd6055 parses each spark type name once into a dict owned by the new module duckdb/experimental/spark/sql/_type_cache.py and adds the default connection tests, the spark exit child, the planted control and the forwarding tests; 5d354fd changes only tests/fast/spark/test_spark_types_default_connection.py, so that the busy call itself signals once it holds the connection. I ran the head as a Release build and as a build with FORCE_ASSERT (built at dfd6055; the one test file is the whole difference to 5d354fd), against the merge base build and the 2.0.0 wheel. The premise still holds: the reporter's cases and the spark exit programs are silent at the head with the report on, and a kept connection reports its one instance. The three round 3 asks are met where round 3 measured them: a spark type read once keeps working while the default connection is busy on another thread or after a failed statement, nested createDataFrame of array<int> is back to the merge base's time, and the new tests now fail against a cache on the instances and against removed forwarding. The cache sits at the right layer. What blocks is the same defect in two narrower shapes: a spark type still parses its name on the default connection the first time it is read, where the merge base parsed at construction or not at all, and an ArrayType or MapType rebuilds its type on every read, which for an interval or unnamed struct element parses on every read and every row.

Since the last review

• Round 3, 1 (types.py parsed DuckDBPyType(name) on every read): addressed for reads after the first. round-3/probes/verify_default_connection.py, busy and aborted, on the Release head and the FORCE_ASSERT head: all five calls work (round-4/probes/state-probes-5d354fd.txt). Still open in two narrower shapes, findings 1 and 2 below.
• Round 3, 2 (no child used a spark type; the module dict child missed duckdb.value.constant): addressed. Round 3's cached property mutation now fails three tests, the spark child among them (round-4/tests-variant-m9-cached-property-assert.txt); the planted constant control passes on both builds, and the comment and description no longer say the child catches a holder of any shape. A related gap remains, finding 3.
• Round 3, 3 (__getattr__ and __dir__ of duckdb/value/constant untested): addressed. Removing them fails the three tests of TestTypeConstantForwarding (round-4/tests-variant-m8-no-forwarding-assert.txt).
• Round 3 smaller things: duckdb_type is read only and a spark type pickles, as the description now says; two reads of an atomic or decimal type give one object again, but two reads of an ArrayType or MapType now give two (finding 2).
• The race in the busy test that turned the py3.15 macOS arm64 abi3 leg red on dfd6055 (DID NOT RAISE ResourceInUseException): addressed by 5d354fd. round-4/probes/busy_gate_race.py reproduces the old order (signal before the query: with 10 ms between them, 50 of 50 rounds not refused) and shows the new one (signal from inside the function: 0 of 50); the test file passed twenty runs of eight copies at eight workers, and twenty more pinned to two CPUs beside four busy loops (round-4/race-20x-n8.txt, round-4/race-20x-n8-under-load.txt). Every wait is bounded and the thread is asserted to have ended.

Findings

1. A spark type still parses its name on duckdb's default connection the first time it is read, so a type built while the default connection was idle fails when first read while that connection is busy or in a failed transaction, where the merge base worked (duckdb/experimental/spark/sql/types.py:83).
   Severity: blocking. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
   Build the native types without the default connection, so that no read parses, and test first reads inside each state.

2. An ArrayType or MapType now rebuilds its DuckDBPyType on every read, so one that holds a DayTimeIntervalType or an unnamed StructType parses on the default connection on every read and row: it fails in both states after a first read, and createDataFrame of 20,000 rows of array<interval> takes 1,014 to 1,045 ms against 430 to 468 ms on the merge base (duckdb/experimental/spark/sql/types.py:656, and :718 for MapType).
   Severity: blocking. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
   Let a container compose only types that need no parse, and cover the interval and the unnamed struct inside an array and a map in both default connection tests.

3. No test tells the cache module apart from a cache dict kept in types.py itself (tests/fast/test_exit_leak_report.py:174).
   Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
   Add a child that uses spark types before it keeps every module dict; it prints three instances when the dict moves into types.py.

The evidence for each is in its inline comment.

Smaller things

• The description's sentence that a spark session's casts and createDataFrame keep working in the two default connection states, and "Reads of one type give the same object, as before", follow findings 1 and 2; "as a singleton's construction did before" is true for singletons, but a DecimalType never parsed on the merge base.
• CI on dfd6055: the reply names one failure of the assertion job, and its log has two: test_a_lazy_polars_frame_scanned_on_its_own_connection_errors timed out after 15 seconds, and test_a_python_error_in_a_pull_on_an_engine_thread_fails_the_query[False] did not raise. Neither test nor the code it runs is touched by this pull request, and both passed in the assertion job of 5d354fd (18188 passed, 120 skipped, 164 xfailed). Neither is this change's.

Outside findings not taken

• Codex, the busy test signalling before the query holds the connection: correct for dfd6055, fixed by 5d354fd before this review.
• Codex, the default connection tests check a cast's label without running the cast: Column.cast reads dataType.duckdb_type at duckdb/experimental/spark/sql/column.py:860 before it builds the label, so the label assertion does exercise the read whose connection dependence the tests are about, and running the cast happens on the session's own connection.
• Codex, other spark paths that parse on the default connection (column.py:386, 393, 504 and 539, dataframe.py:2333, udf.py:83): untouched by this pull request and the same on the merge base.

What I verified

• Head 5d354fd, merge base 4583a30 (unchanged), range five commits; git diff dfd6055 5d354fd is the one test file, +15 and -9.
• Builds: the tree at 5d354fd, Release (the coordinator's build, BUILD OK); the FORCE_ASSERT clone, built at dfd6055 and checked out at 5d354fd; the merge base build; duckdb 2.0.0 from PyPI.
• Probes, in round-4/probes: verify-default-connection.txt and state-probes-5d354fd.txt (round 3's probe and the built before, read during probe, both states, both builds), cold-first-read.txt, busy-parse-names.txt, singletons-at-import.txt, identity.txt, spark-bench-r4.txt, exit-cases.txt and exit-cases-m15.txt, busy-gate-race.txt.
• Mutations on the FORCE_ASSERT build, each restored, git status clean after: cached property on DataType.duckdb_type 3 failed, 77 passed, 7 skipped; cache dict in types.py 80 passed, 7 skipped; forwarding removed 3 failed, 77 passed, 7 skipped; unmutated 80 passed, 7 skipped (test_exit_leak_report.py, test_value.py, test_spark_types_default_connection.py, eight workers).
• Release head, the touched selection (test_exit_leak_report.py, test_value.py, tests/fast/spark, test_map.py, the two arrow exit files, tests/slow/test_interpreter_shutdown.py, test_expression.py, tests/fast/expression), eight workers: 3216 passed, 8 skipped, 4 xfailed in 58.88s.
• Outside: one Codex review, read only, of dfd6055 (round-4/codex-report.md, prompt beside it); its five findings are triaged above (three taken as findings 1 to 3, two not taken).
• CI on 5d354fd at the time of writing: every completed check green, the assertion job among them; Seed cp312-macosx_x86_64 pending and the abi3 test legs not yet started.

What held

• Warm reads of atomic and decimal types and of arrays and maps of them work in both default connection states, and a failed first parse leaves no cache entry, so the read works again after ROLLBACK.
• The exit cases of rounds 1 to 3 are silent at the head with the report on, in both builds, and a kept connection or a file database kept through the numpy overload registry reports its one DuckDBPyConnection instance.
• The flat createDataFrame is level with the merge base (429.8 and 430.9 ms against 426.6 and 418.7 ms).

[el-marvin]

A spark type still parses its name on duckdb's default connection the first time it is read, so a type built while the default connection was idle fails when it is first read while that connection is busy on another thread or in a failed transaction, where the merge base worked.

Severity: blocking. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

name = self._duckdb_type_name
if name not in _type_cache.TYPES:
    _type_cache.TYPES[name] = DuckDBPyType(name)
return _type_cache.TYPES[name]

DuckDBPyType(name) goes through FromString (src/typing/pytype.cpp:128), which takes the default connection with GetConnection() and parses inside RunFunctionInTransaction (pytype.cpp:336). Every name is refused while the default connection runs a query, on the merge base as on the head (round-4/probes/busy-parse-names.txt), so the question is only when the parse runs. On the merge base it ran when a singleton was constructed, and sixteen singletons are constructed when the spark package is imported (round-4/probes/singletons-at-import.txt); DecimalType never parsed, it called duckdb.decimal_type, which needs no connection. At the head DecimalType.__init__ (line 379) turns that native type into a string, str(duckdb.decimal_type(precision, scale)), and the first read parses the string back.

round-4/probes/built_before_read_during.py builds the types while the default connection is idle and reads them first while it is busy (a long query on another thread) or after BEGIN and a failed statement (round-4/probes/state-probes-5d354fd.txt, the head as a Release build and as the FORCE_ASSERT build; the merge base in built-before-read-during.txt):

merge base, busy, cold: DecimalType(17, 4).duckdb_type -> ok DECIMAL(17,4)
merge base, busy, cold: UnsignedShortType().duckdb_type -> ok USMALLINT
merge base, busy, cold: df.select(cast to the DecimalType(17, 4) built before).collect() -> ok [Row(d=Decimal('1.0000'))]
head,       busy, cold: DecimalType(17, 4).duckdb_type -> ResourceInUseException
head,       busy, cold: UnsignedShortType().duckdb_type -> ResourceInUseException
head,       busy, cold: df.select(cast to the DecimalType(17, 4) built before).collect() -> ResourceInUseException
head,    aborted, cold: the same eight reads -> TransactionException

round-4/probes/cold_first_read.py shows the same for calls that build the type at the call: F.col('a').cast(UnsignedIntegerType()) and df.select(F.col('a').cast(DecimalType(14, 4))).collect() work on the merge base in both states and raise at the head (round-4/probes/cold-first-read.txt). The new tests cannot see this: both call _cast_and_build(spark) before the state change (tests/fast/spark/test_spark_types_default_connection.py:34 and :59), which fills the cache with every name the second call reads. The description's sentence that casts and createDataFrame keep working in those two states holds only for names already read.

Ask: build the native types without the default connection, so that no read parses: for example fill the cache with the fixed names when the types module is imported, as the merge base already parsed sixteen of them at import, and build decimals with duckdb.decimal_type as the merge base did; then test reads that come first inside each state (an atomic type built before, a DecimalType of a precision not read before, a cast that builds its type at the call), and correct the description's sentence.

[el-marvin]

An ArrayType or MapType now rebuilds its DuckDBPyType on every read, so one that holds a DayTimeIntervalType or an unnamed StructType parses on the default connection on every read and every row: it fails in both states even after a first read, and createDataFrame of an array of intervals takes more than twice as long as on the merge base.

Severity: blocking. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

@property
def duckdb_type(self) -> DuckDBPyType:  # noqa: D102
    return duckdb.list_type(self.elementType.duckdb_type)

MapType.duckdb_type at line 718 does the same with duckdb.map_type. The leaves they read are cached for atomic and decimal types, but two leaves still parse on each read: DayTimeIntervalType.duckdb_type returns DuckDBPyType("INTERVAL") (line 602) and an unnamed StructType returns DuckDBPyType(f"TUPLE(...)") (line 859). On the merge base the container built its type once, in its constructor, so those parses ran once; at the head they run on every read, and _sql_value in duckdb/experimental/spark/sql/session.py:69 reads data_type.duckdb_type for every nested value.

round-4/probes/state-probes-5d354fd.txt, types built and read once while the default connection is idle, then read again in each state (Release head and FORCE_ASSERT head alike; the merge base works in all of them, built-before-read-during.txt):

head, busy, warm: ArrayType(DayTimeIntervalType()).duckdb_type -> ResourceInUseException
head, busy, warm: MapType(StringType(), DayTimeIntervalType()).duckdb_type -> ResourceInUseException
head, busy, warm: ArrayType(StructType([StructField('', IntegerType())])).duckdb_type -> ResourceInUseException
head, busy, warm: ArrayType(IntegerType()).duckdb_type -> ok INTEGER[]
head, aborted, warm: the first three -> TransactionException

Cost, round-4/probes/spark_bench_r4.py, createDataFrame of 20,000 rows of (a int, b array<interval>), one thread, best of five, two alternating runs: merge base 468.4 and 430.2 ms, head 1045.4 and 1014.2 ms (round-4/probes/spark-bench-r4.txt). For array<int> the head is level with the merge base (468.5 and 497.8 ms against 523.8 and 508.3 ms in the same runs). A second effect of the same code: two reads of one ArrayType or MapType now give two objects (round-4/probes/identity.txt), so the description's "Reads of one type give the same object, as before" no longer holds for them.

Ask: let a read of a container compose only types that need no parse, which follows if DayTimeIntervalType and the unnamed StructType take their type from the same parse free source as finding 1's fix, and add the interval and the unnamed struct inside an array and a map to both default connection tests.

[el-marvin]

No test tells the cache module apart from a cache dict kept in types.py itself: with the dict moved into types.py every test of the change passes, and only a child that fills the cache before it keeps the module dicts would report it.

Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

The comment at types.py:79 and the docstring of _type_cache.py give the reason for the separate module: the types module's dict outlives finalization through the functions that hold it. The tests do not pin that reason. The spark child (SPARK_TYPES_USED_AND_FUNCTION_KEPT) fills the cache but keeps the types module object alive through functions, so CPython clears that module's dict anyway; the module dict child (EVERY_MODULE_DICT_KEPT) lets the module objects die but never reads a spark type, so the cache is empty.

Mutation on the FORCE_ASSERT build at 5d354fd (round-4/probes/mutate-r4.sh m15-cache-in-types: _TYPES: dict = {} in types.py in place of from . import _type_cache): tests/fast/test_exit_leak_report.py, tests/fast/test_value.py and tests/fast/spark/test_spark_types_default_connection.py give 80 passed, 7 skipped, the same as the unmutated build (round-4/tests-variant-m15-cache-in-types-assert.txt, tests-variant-pristine-assert.txt). round-4/probes/cases/spark_used_then_every_module_dict_kept.py, the spark child's body followed by the module dict child's body, is silent at the head and prints nanobind: leaked 3 instances!, all _duckdb._sqltypes.DuckDBPyType, with the mutation (round-4/probes/exit-cases-m15.txt). The cached property mutation of round 3 is caught now (round-4/tests-variant-m9-cached-property-assert.txt, 3 failed).

Ask: add the combined child, _run(REPORT_ON, SPARK_TYPES_USED_AND_FUNCTION_KEPT, EVERY_MODULE_DICT_KEPT) or a parameter of the list that does the same, so the test pins why the cache has a module of its own; it then also guards whatever cache finding 1's fix keeps.

[el-marvin]

Changes requested. I reviewed 7ab2576, one commit on top of round 4's 5d354fd: every spark type's DuckDBPyType is now put into the _type_cache dict when the spark type is constructed (atomic names parsed, decimals, arrays and maps composed with duckdb.decimal_type, duckdb.list_type and duckdb.map_type and keyed by their SQL text), duckdb_type only reads the dict, DayTimeIntervalType returns duckdb.sqltypes.INTERVAL, an unnamed StructType parses its TUPLE(...) text once per text, and every singleton is built when the types module is imported; the default connection tests are rewritten around first reads, and one exit child is added. No C++ changed, so I ran the head as a Release build and as a build with FORCE_ASSERT (the earlier assertion build moved to the new commit without a rebuild), against the merge base build. The three round 4 findings are fixed where round 4 measured them: every shape of the round 4 probes answers in both default connection states, built before or inside the state, on both builds; arrays and maps give one object across reads; array<interval> createDataFrame is back to the merge base's time; and moving the cache dict into the types module now fails three tests. The premise holds and the approach for parsed names sits at the right layer. What is left comes from the new step of putting composed types into the text keyed cache, and from tests that do not tell three parts of the commit apart: a spark type that is pickled and loaded in another interpreter now fails with a KeyError on use, the cache keeps every distinct decimal, array and map type for the life of the process and doubles the cost of building one, and an unnamed StructType still parses on its first read, against the description's "never when it is read".

Since the last review

• Round 4, 1 (a spark type parsed its name on the default connection on its first read): addressed. round-4/probes/built_before_read_during.py busy and aborted, cold and warm, and round-4/probes/cold_first_read.py busy and aborted, on the Release head and the FORCE_ASSERT head: every read, cast and frame answers, including LongType(), DecimalType(12, 3), ArrayType(ShortType()), MapType(StringType(), DoubleType()) and DayTimeIntervalType() first built inside the state; on the merge base the same cold first reads fail for those five and for the frame (round-5/probes/state-probes-7ab2576.txt). 29 singletons are built at import, 16 on the merge base (round-5/probes/verify-default-connection-and-singletons.txt). The new default connection tests fail on the merge base (2 failed, at line 51, round-5/tests-new-default-connection-on-base.txt) and with types.py at 5d354fd (2 failed, round-5/tests-variant-m21-types-at-5d354fd-assert.txt).
• Round 4, 2 (arrays and maps rebuilt their type on every read): addressed. Two reads give one object for arrays and maps on both builds, as on the merge base (round-5/probes/identity.txt). createDataFrame of 20,000 rows, one thread, best of five, two alternating runs (round-5/probes/spark-bench-r5.txt): array<interval> 439.0 and 441.9 ms against the merge base's 439.3 and 442.0; array<int> 453.1 and 445.6 against 452.8 and 443.7; flat 427.5, 411.4, 409.6, 416.7, 414.6 against 416.3, 401.4, 404.0, 414.3, 410.8, with the same instruction count, 4,088 M on both (round-5/probes/spark-bench-r5-instructions.txt; array<int> 4,362 M against 4,353 M). The unnamed struct inside a container: addressed by decision. It now parses once per text instead of on every read, and the reply's reason for not composing it holds: struct_type names list fields v1, v2 (src/pyconnection/type_creation.cpp:32), and LogicalType::STRUCT makes a TUPLE only when every name is empty (external/duckdb/src/common/types.cpp:1646), which a dict cannot express for two fields.
• Round 4, 3 (no test told the cache module apart from a dict in types.py): addressed, and better than asked. With the dict moved into types.py (round-5/probes/mutate-r5.sh m15-cache-in-types) three tests fail: the new combined child, the plain module dict child, which now sees the 29 entries built at import, and the planted control (round-5/tests-variant-m15-cache-in-types-assert.txt).
• Round 3's cached property on the instances (m9-cached-property, adapted to the new property): 2 failed, the spark child and the combined child (round-5/tests-variant-m9-cached-property-assert.txt).
• A correction to round 4: finding 2 listed "two reads give two objects" as part of the defect. One object per read was the merge base's behaviour and is worth keeping, but it was not a requirement in itself, and finding 2 below asks for a design that keeps it without a cache entry.

Findings

1. A spark type that is pickled and loaded in another interpreter raises KeyError on every use, because it carries only the key of a cache entry that only the interpreter that built it holds (duckdb/experimental/spark/sql/types.py:85). Severity: should fix. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced. Details in the inline comment.
2. Composed types go through the text keyed cache although they need no parse, so the cache keeps every distinct decimal, array and map type for the life of the process and every construction pays a str() of the whole type (types.py:78). Severity: should fix. Category: design. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced. Details in the inline comment.
3. Three parts of the commit can be reverted with every touched test still passing: the import time construction of the singletons, the unnamed struct's cached text, and the map type composed once (tests/fast/spark/test_spark_types_default_connection.py:51). Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced. Details in the inline comment.
4. An unnamed StructType still parses its text on the default connection on its first read, while the description and the test's docstring say a type is never parsed when it is read (types.py:854). Severity: should fix. Category: docs. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: pre-existing (the merge base parsed on every read; the head parses once per text, which is better). Details in the inline comment.

Smaller things

• types.py:1195: "as sixteen of them always were" describes history a reader of the file cannot check. Every name the singletons parse but TIMESTAMPTZ_NS has a duckdb.sqltypes constant of the same text (28 of 29), so passing those constants would leave one parse at import and make the loop over globals() unnecessary; importing the spark types while the default connection is busy fails on the merge base and the head alike, so this is a simplification, not a fix.
• duckdb/experimental/spark/sql/_type_cache.py:1: "parsed or composed once": a container is composed on every construction (duckdb.list_type runs before the dict is checked); only the entry is kept once.
• tests/fast/test_exit_leak_report.py:112: "instead of the third" names a module by its place in the tuple; name the module.
• tests/fast/spark/test_spark_types_default_connection.py:40: "Constructed, never read": an ArrayType or MapType reads its element types when it is constructed, so the array of unnamed structs has parsed its element before the state changes.
• The description's figures put the head below the merge base (416 against 437 ms, 409 against 434 ms). My alternating runs put it level: 439 and 442 ms on both for array<interval>, 453 and 446 against 453 and 444 for array<int>. "Level with the merge base" is the claim the numbers carry.

Outside findings not taken

• An ArrayType built before its StructType element gains a field through add keeps the old type, and the two arrays compare unequal (code-review and Codex): the merge base does the same (round-5/probes/construction-shapes.txt); the commit restores the merge base's construction time composition.
• ArrayType(DayTimeIntervalType(1, 2)) and _parse_datatype_string("array<interval hour>") raise at construction (code-review): the merge base does the same (construction-shapes.txt).
• Keying by str() loses a collation, so DataType(duckdb.string_type("nocase")) gets the plain VARCHAR entry (Codex): not reachable through the spark API. No spark type passes a collated type, and type_utils.py maps a collated column to StringType(), as the merge base did; only a direct call of the base class with a hand built type reaches it.
• "Decimals and containers touch no connection" is literally false because the module functions fetch the default connection object (Codex): nothing runs on it, no busy check and no transaction, as on the merge base (src/duckdb_python.cpp:225 to 304, type_creation.cpp:5 to 85).
• Two more default connection parses, the DuckDB syntax fallback of a cast given as text (column.py:539) and _cast_types (dataframe.py:2333) (Codex): pre-existing, untouched by this pull request, and both take type text rather than a spark type.
• The import time parses as a defect (code-review): taken as a smaller thing; the merge base parsed sixteen there.
• Two isinstance checks in DataType.__init__ (code-review): style; finding 2 changes that constructor anyway.

What I verified

• Head 7ab2576; range 4583a30..7ab2576, six commits; this round's delta 5d354fd..7ab2576, four files, 85 added, 41 removed (round-5/delta.diff). The pull request diff from GitHub matches the local range (round-5/pr-diff-7ab2576.diff).
• Builds: the tree at 7ab2576, Release (build.log ends BUILD OK 23:47:46); the assertion clone moved from 5d354fd to 7ab2576 by fetch and detached checkout, not rebuilt because no C++ changed (four Python and test files), _duckdb._assertions True, clean at the end of the round; the merge base build at 4583a30.
• Probes, all with memory_limit 8GB and 8 or 1 threads, in round-5/probes: state-probes-7ab2576.txt (round 4's built_before_read_during.py and cold_first_read.py on head, assertion head and base), r5_shapes.py and r5-shapes.txt (unnamed, nested unnamed, quoted names, arrays_zip, list_zip in both states), pickle_fresh.py and pickle-fresh.txt (base, head, and types.py at 5d354fd), construction_shapes.py and construction-shapes.txt, cache_growth.py and cache-growth.txt, construction-cost.txt, spark_bench_infer.py, spark_bench_infer_instr.py and their results, identity.txt, spark-bench-r5.txt, spark-bench-r5-instructions.txt, exit-cases.txt, verify-default-connection-and-singletons.txt, and m23-composed-on-instance-prototype.txt.
• Exit children with the report on (round 3's and round 4's run_cases.py; the switch on the Release head, plain on the assertion head): the reporter's cases, polars, the kept type, the three spark cases and the combined child are silent; a kept connection reports its one instance. Round 3's copy of the module dict child prints 29 DuckDBPyType instances on both builds because it predates the cache module and keeps that module's dict too; the tree's child excludes the cache module and is silent.
• Tests, eight workers, a 600 second timeout, from each clone's root: the selection the reply names (round-5/selection.txt), Release 3324 passed, 8 skipped, 4 xfailed in 53.37s, FORCE_ASSERT 3317 passed, 15 skipped, 4 xfailed in 50.45s, both as the reply states. Mutations on the assertion clone, after the pristine runs, each over tests/fast/test_exit_leak_report.py and tests/fast/spark and each undone (round-5/probes/mutate-r5.sh): m15 cache in types.py 3 failed; m9 cached property 2 failed; m16 unnamed struct parsed on every read, passes; m17 map composed on every read, passes; m18 no import time loop, passes (2 failed when the default connection file runs alone); m19 interval parsed, 2 failed; m20 decimal as text, 2 failed; m21 types.py at 5d354fd, 2 failed; m22 array composed on every read, 2 failed; m23, a prototype for finding 2 (composed types held on the instance, names alone in the cache), passes, exit children silent, cache at 29 entries after 100,000 distinct types. Each mutation and the prototype were thrown away.
• Outside reviews: Codex at xhigh, read only, on the delta and its description sentences (round-5/codex-report.md, prompt beside it, log deleted, no question filed); the code-review skill at high effort over 5d354fd..7ab2576 (round-5/code-review-findings.md, ten items).
• CI on 7ab2576: ten checks green, including the full API build with assertions, Build sdist and Sanity cp311 manylinux aarch64; Sanity cp311 manylinux x86_64 and Seed cp312 macOS x86_64 pending; nothing red (round-5/pr-checks-7ab2576-final.txt).

What held

• Struct field names that need quoting survive the text key: STRUCT("a b" INTEGER, "Se""l" VARCHAR) and TUPLE(STRUCT("c d" INTEGER), DOUBLE)[] read back as built, and a nested unnamed struct caches one entry per level (TUPLE(INTEGER), TUPLE(TUPLE(INTEGER))), one object across reads.
• A named StructType composes with duckdb.struct_type on every read and answers in both states; an array or map over an unnamed struct built while idle answers in both states.
• Equality and hashing of decimals, arrays and maps are unchanged (construction-shapes.txt), and copying within one interpreter keeps a usable key.
• The flat and nested createDataFrame paths with a schema cost the same instructions as on the merge base.

[el-marvin]

A spark type that is pickled and loaded in another interpreter raises KeyError on every use, because it carries only the key of a cache entry that only the interpreter that built it holds.

Severity: should fix. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

        self._duckdb_type_key = key

    @property
    def duckdb_type(self) -> DuckDBPyType:  # noqa: D102
        return _type_cache.TYPES[self._duckdb_type_key]

Unpickling restores __dict__ without running __init__, so a loaded DecimalType, ArrayType, MapType or a schema holding one has a _duckdb_type_key that a fresh interpreter's cache lacks: the import fills only the 29 singleton names. round-5/probes/pickle_fresh.py pickles five types in one interpreter and loads them in another (round-5/probes/pickle-fresh.txt):

• merge base: TypeError: cannot pickle '_duckdb._sqltypes.DuckDBPyType' object at pickle.dumps, for all five;
• head: every pickle.dumps works; after loading, LongType() answers, and DecimalType(17, 4).duckdb_type, F.col("x").cast(...) with it, the array, the map, the struct holding a decimal and spark.createDataFrame(rows, unpickled_schema) raise KeyError: 'DECIMAL(17,4)', KeyError: 'BIGINT[]', KeyError: 'MAP(VARCHAR, DOUBLE)';
• types.py at 5d354fd (mutation m21 on the assertion clone): all of it answers, because that read parsed on a miss.

A schema handed to a multiprocessing worker started by spawn, the default on macOS and Windows, now fails late, in the worker, with a bare KeyError, where the merge base refused at once. The description's "a spark type now pickles, where it raised cannot pickle DuckDBPyType before" holds only inside one interpreter.

Ask: either a loaded spark type answers in a fresh interpreter (restore its entry when it is unpickled, by composing it again from its parts or parsing its text there, which is a construction and not a read), or pickling refuses as on the merge base and the sentence goes; with finding 2's type held by the instance the second follows for decimals and containers. Pin whichever you choose with a child interpreter that loads a pickled decimal, array and map and casts with them.

[el-marvin]

Composed types go through the text keyed cache although they need no parse, so the cache keeps every distinct decimal, array and map type for the life of the process and every construction pays a str() of the whole type.

Severity: should fix. Category: design. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

        key = duckdb_type if isinstance(duckdb_type, str) else str(duckdb_type)
        if key not in _type_cache.TYPES:
            _type_cache.TYPES[key] = DuckDBPyType(key) if isinstance(duckdb_type, str) else duckdb_type
        self._duckdb_type_key = key

The cache exists so that a parsed name is parsed once and so that no DuckDBPyType sits in the singleton cache past finalization. A decimal, array or map needs neither: duckdb.decimal_type, duckdb.list_type and duckdb.map_type compose without a parse, and none of duckdb's own modules keeps such an instance past finalization (no module level or default argument instance of them in the spark package). Putting them in the dict costs two things:

• Retention. round-5/probes/cache_growth.py 100000 builds and drops an ArrayType of a struct with a changing field name and a MapType of an array of such a struct, as schema inference over changing field names does: the head keeps 300,030 entries and 196 MB of resident memory, the merge base keeps nothing (round-5/probes/cache-growth.txt). 5d354fd kept names only.
• Construction time. Best of five, 100,000 constructions each (round-5/probes/construction-cost.txt): ArrayType(LongType()) 495 ns on the merge base, 877 ns at the head; MapType(StringType(), ArrayType(LongType())) 1,356 against 2,394 ns; DecimalType(17, 4) 442 against 949 ns. createDataFrame of 20,000 rows without a schema, which builds the types of every row, one thread, fewest of five: lists 6,365 M instructions on the merge base, 6,508 M at the head (+2.2 percent); maps of lists 8,314 M against 8,727 M (+5.0 percent), wall 877 and 879 ms against 902 and 902 ms (round-5/probes/spark-bench-infer-instructions.txt, spark-bench-infer.txt).

A prototype on the assertion clone (m23: a composed type stays on the instance, as on the merge base, and only parsed names go into the dict; thrown away) keeps tests/fast/test_exit_leak_report.py and tests/fast/spark green (1867 passed, 15 skipped, 2 xfailed), the three spark exit cases and the combined child silent, two reads one object, and the cache at 29 entries after the 100,000 types (round-5/probes/m23-composed-on-instance-prototype.txt).

Ask: keep in the cache only what needs a parse (the atomic names, the unnamed struct's text) and let a decimal, array or map hold the type it composed, or give the composed entries a bound; add a test that building and dropping many distinct containers leaves the cache's size where it was.

[el-marvin]

Three parts of the commit can be reverted with every touched test still passing: the import time construction of the singletons, the unnamed struct's cached text, and the map type composed once.

Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

def _first_reads_and_casts(spark, built):
    assert str(UnsignedShortType().duckdb_type) == "USMALLINT"
    assert str(LongType().duckdb_type) == "BIGINT"

Mutations on the assertion clone, each over tests/fast/test_exit_leak_report.py and tests/fast/spark with eight workers, each undone (round-5/probes/mutate-r5.sh):

• m18, the loop at types.py:1195 removed: 1867 passed. Run alone, the default connection file fails twice with ResourceInUseException and TransactionException at this line (round-5/tests-variant-m18-no-import-loop-alone-assert.txt). In a worker that already ran other spark tests LongType() was built before and this is not a first read; UnsignedShortType() is built two functions up, in _built_while_idle, so it is never a first read. CI runs the whole fast suite in workers, so the loop that fixes round 4's first finding is protected only by test order.
• m16, the unnamed struct parsed on every read again (return DuckDBPyType(text)): passes. The only unnamed struct the tests read sits inside an array, which reads it once when it is built.
• m17, MapType.duckdb_type composed on every read again: passes. The identity assertion at line 56 covers the array only.

The other changed parts are pinned: m19 (interval parsed again), m20 (decimal passed as text), m21 (types.py at 5d354fd) and m22 (array composed on every read) each fail both default connection tests.

Ask: run the first reads in a child interpreter, as tests/fast/test_exit_leak_report.py does, so that "first" means first; read an unnamed StructType once while idle and again inside each state; and assert one object across reads for the map too.

[el-marvin]

An unnamed StructType still parses its text on the default connection on its first read, while the description and the test's docstring say a type is never parsed when it is read.

Severity: should fix. Category: docs. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: pre-existing.

            text = f"TUPLE({', '.join(str(f.duckdb_type) for f in self.fields)})"
            if text not in _type_cache.TYPES:
                _type_cache.TYPES[text] = DuckDBPyType(text)
            return _type_cache.TYPES[text]

round-5/probes/r5_shapes.py builds StructType([StructField("", UnsignedShortType())]), a nested unnamed struct and an unnamed struct over a field named a b while the default connection is idle, and reads them first inside each state: all three raise ResourceInUseException while another thread holds the connection and TransactionException after a failed statement, on the head as on the merge base; spark.sql("select list_zip([1], ['a'], [2.5]) z").schema fails the same way on both, because type_utils.convert_nested_type builds an ArrayType over the unnamed struct (round-5/probes/r5-shapes.txt). The head is better than the merge base, which parsed on every read; it fails only for a text it has not parsed before.

The description says a type "goes into that dict when the spark type is constructed and never when it is read", and the test module's docstring says the type is parsed "when the types module is imported ... and never when a type is read". A named StructType also never goes into the dict: it composes a new duckdb.struct_type on every read, two objects for two reads on the merge base and the head (round-5/probes/construction-shapes.txt), against "Reads of one type give the same object, containers included".

Ask: either resolve the unnamed text when the StructType is built and when add changes it, so the rule holds as written, or say in the description, the docstring and the comment at line 853 that an unnamed struct parses its text on the first read of that text, and that a named struct composes on every read.

[el-marvin]

Changes requested. I reviewed db8dd39, one commit on top of round 5's 7ab2576: decimals, arrays and maps now hold the type they compose on the instance and only parsed names go into _type_cache.TYPES; the three composed classes define __reduce__, so a pickle loaded in another interpreter composes its type again; a struct without field names parses its TUPLE(...) text when it is built or a field is added; the default connection tests gain a child interpreter for singletons, a bare unnamed struct, identity for the map, a pickle child and a cache size test. No C++ changed, so I ran the head as a Release build and as a build with FORCE_ASSERT (the assertion build moved to the new commit without a rebuild), against the merge base build, and for the cost figures against 7ab2576's two spark files over the head's Release build. Three of round 5's four findings are fixed and pinned: a pickled decimal, array, map or schema answers in a fresh interpreter, the cache stays at 29 entries after 100,000 distinct containers, and the import loop, the map's identity and the unnamed struct's cache now each fail a test when reverted. The premise holds. What is left comes from the fourth: parsing a struct's text in its constructor, which my round 5 review offered as one option, also runs when a spark DataFrame converts a relation's types, so any session query that returns row(...) now fails while the default connection is busy or after a failed statement in its transaction, where the merge base and 7ab2576 answer; and the tests build the same texts before and inside the state, so they do not tell three weaker versions of the change apart. The full fast suite passes on the Release head. One minor item is new: the __reduce__ methods give a subclass back as its base class.

Since the last review

• Round 5, 1 (a pickled spark type raised KeyError in another interpreter): addressed. round-5/probes/pickle_fresh.py dump and load in fresh interpreters: every type, cast and createDataFrame with the loaded schema answers on the Release and the assertion build; the merge base refuses at pickle.dumps (round-6/probes/pickle-fresh.txt). Copies, deep copies and loads with protocols 0 to 5 compare equal to fresh builds (round-6/probes/pickle-edges.txt). Removing any one of the three __reduce__ fails test_pickled_types_answer_in_a_fresh_interpreter (mutations m24, m25, m26: 1 failed each).
• Round 5, 2 (composed types kept in the text keyed cache): addressed. round-5/probes/cache_growth.py 100000: 29 entries at the head, 300,030 at 7ab2576 (round-6/probes/cache-growth.txt); resident memory after the run swings between 0 and 41 MB on the merge base and the head alike, outside the Python heap, so it carries no signal. Putting composed types back into the cache (m27) fails the new cache size test. Construction cost, the same script on all three builds in two alternating runs, best of five of 100,000, with user space instructions (round-6/probes/construction_cost.py, construction-cost.txt): ArrayType(LongType()) 616 and 616 ns on the merge base, 1,020 and 1,018 at 7ab2576, 744 and 749 at the head (6,700, 10,181, 7,920 instructions); MapType(StringType(), ArrayType(LongType())) 1,589 and 1,634, 2,817 and 2,782, 1,966 and 1,948 (16,537, 27,065, 19,897); DecimalType(17, 4) 474 and 460, 1,071 and 1,069, 509 and 532 (5,193, 10,576, 5,694). The reply's ratios to the merge base (630 against 515, 1,721 against 1,386, 555 against 499) match mine.
• Round 5, 3 (three parts reverted with the tests green): addressed for the three parts named. With the import loop removed (m18), the new child test and the pickle test fail in the selection with eight workers (2 failed) and four tests fail with the file alone; the map composed on every read (m17) and the unnamed struct parsed on every read (m16) each fail both state tests. The new parts have the same kind of gap; finding 2.
• Round 5, 4 (an unnamed struct parsed on its first read): the first option of my ask is what was built, and that option was wrong. It closes the corner for structs the program builds, and opens the session query regression in finding 1, which is worse than what it fixed; I withdraw it. The named struct: addressed by decision. It composes with duckdb.struct_type on each read, two objects for two reads as on the merge base (round-6/probes/identity.txt), and the description now says so; the tests assert identity for the array, the map and the unnamed struct and nothing either way for a named struct, which is coherent with that sentence. An interval and an unnamed struct now give one object across reads, where the merge base gave two.
• Round 5's smaller things: taken (the loop comment, the _type_cache docstring, the TYPE_STORES comment, the test comment about elements read at construction, "level with the merge base" in the description), and the first bullet's switch to duckdb.sqltypes constants declined with a reason that holds: a singleton holding a constant would keep it alive through the singleton cache, which outlives finalization, the shape the exit children report when the type is cached on every instance (mutation m9, 3 failed).
• Earlier rounds, rerun at the head: the exit children are silent with the report on, a kept connection reports its one instance (round-6/probes/exit-cases.txt); round 4's state probes, cold and warm, in both states, answer all 59 reads (round-6/probes/state-probes-db8dd39.txt); round 5's struct shapes answer in both states (round-6/probes/r5-shapes.txt); the cached property on the instances (m9) and the cache dict in the types module (m15) each fail three tests.

Findings

1. Parsing an unnamed struct's text in StructType.__init__ makes every spark DataFrame whose relation holds a struct without field names depend on the default connection, so spark.sql("select row(1, 2) r").collect() now fails while the default connection is busy or after a failed statement in its transaction, where the merge base answers (duckdb/experimental/spark/sql/types.py:871). Severity: blocking. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced. Details in the inline comment.
2. The idle half of the default connection tests builds the same type texts the busy half then reads or builds, so three weaker versions of the change pass every test: an unnamed struct not parsed at construction, an interval parsed once into the cache, and a decimal composed on every read (tests/fast/spark/test_spark_types_default_connection.py:86). Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced. Details in the inline comment.
3. The three new __reduce__ methods name their class, so copy.copy, copy.deepcopy and pickle of a subclass of DecimalType, ArrayType or MapType give back the base class, where the merge base's copy.copy kept the subclass (duckdb/experimental/spark/sql/types.py:395). Severity: minor. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced. Details in the inline comment.

Smaller things

• tests/fast/spark/test_spark_types_default_connection.py:64: RELEASE_THE_DEFAULT_CONNECTION is defined and never used; the child test releases the connection in its own finally.
• tests/fast/spark/test_spark_types_default_connection.py:61: the child asserts that the function was reached but not that the default connection then refuses, which the in process tests check with pytest.raises(duckdb.ResourceInUseException) at line 126. If the engine ever stopped refusing there, the child test would pass without testing anything; the same two lines in the child keep it honest.
• tests/fast/spark/test_spark_types_default_connection.py:171: the pickled array and map use the default nullability, so a __reduce__ that dropped containsNull or valueContainsNull would pass; one non default flag pins the second argument (read, not run).
• Cost of the property, for the record: a read of duckdb_type takes 28 ns on the merge base, a plain attribute, against 53 ns for a composed type and 72 ns for a singleton at the head (round-6/probes/read_cost.py, read-cost.txt), which is part of the construction cost above beside the isinstance and the two stores the reply names. createDataFrame of 20,000 rows without a schema, which builds the types of every row, costs 1.6 percent more instructions than on the merge base for lists and 2.3 percent for maps of lists (6,465 M against 6,363 M, 8,508 M against 8,320 M; 7ab2576 was at 2.2 and 5.0 percent), and about 2 percent more wall time (645 and 657 ms against 634 and 644; 882 and 890 against 860 and 869; round-6/probes/spark-bench-infer-instructions.txt, spark-bench-infer.txt). With a schema it is level: 4,361 M against 4,353 M nested, 4,087 to 4,091 M on both flat (round-6/probes/spark-bench-schema.txt). This is the experimental API, not a core hot path, so I do not count it as a finding; the description's "level with the merge base" holds for a given schema and could say what inference costs.
• The description's "a type built in that state ... keep working" leaves out a struct without field names built inside the state: spark.sql("select list_zip([1], [2]) z") refuses in both states on the merge base and the head alike, because the array over the tuple reads its element when it is built (round-6/probes/session-tuple.txt).

Outside findings not taken

• copy.copy of an ArrayType whose element struct gained a field through add after the array was built rebuilds the array from the current fields, so the copy differs from the original (Codex): the original already holds a stale type after element.add on the merge base (round-6/probes/construction-shapes.txt), the copy is the one that agrees with its fields, and mutating an element after wrapping it is not ordinary use.
• Unnamed struct reads rebuild their TUPLE(...) text on every access (Codex and code-review, cost): the merge base parsed that text on every read, which costs far more, and only the rare struct without field names takes that path.
• The pickled unnamed struct that parses on its first read (Codex): taken into finding 1 as evidence that the construction rule does not hold as written; with the parse on the first read again it follows the rule.
• The construction parse of an unnamed struct untested (Codex): taken into finding 2, confirmed by mutation.
• A decimal, array or map type held on its instance is reported at exit when a program keeps a schema in a module dict that outlives finalization (code-review): true, two instances for a schema of a decimal and an array where 7ab2576 reported none, but the merge base reported 46 for the same script and 44 for a schema of singletons alone, the report is off in builds without assertions, and an object the program keeps is reported as the exit test module says of a kept connection; the cache that avoided it is the growth round 5 removed (round-6/probes/user_module_schema.py, user-module-schema.txt).
• Build the singletons from duckdb.sqltypes constants and the struct without field names without a parse (code-review): the constants were round 5's smaller thing, declined with a reason that holds (see the ledger), and the binding has no constructor for a struct without field names, the separate change finding 1 names.
• The wait_for_gate holder written twice and a second subprocess runner beside the exit tests' (code-review): the child needs the holder as source text and the in process tests need it as code, and the two runners differ on purpose (core dumps off for the exit children, the editable rebuild's output quieted here); style.

What I verified

• Head db8dd39; range 4583a30..db8dd39, seven commits; this round's delta 7ab2576..db8dd39, four files, 165 added, 27 removed (round-6/delta.diff). The pull request diff from GitHub has the same files and line counts as the local range (round-6/pr-diff-db8dd39.diff). The description was read in full and compared with round 5's (round-6/pr-description-db8dd39.md).
• Builds: the tree at db8dd39, Release (build.log ends BUILD OK 00:50:07); the assertion clone moved from 7ab2576 to db8dd39 by fetch and detached checkout, not rebuilt because no C++ changed, _duckdb._assertions True, clean at the end; the merge base build at 4583a30; a copy of the head's Python package with 7ab2576's types.py and _type_cache.py, run over the head's Release extension, for the three way cost figures.
• Probes, every connection with memory_limit 8GB and 8 threads (1 for the benchmarks), in round-6/probes: pickle-fresh.txt, pickle_edges.py and pickle-edges.txt, session_tuple.py and session-tuple.txt (run again in both states on the head and the merge base when the round was finished, with the same rows: session-tuple-rerun.txt), cache-growth.txt with growth_trim.py and growth_where.py, construction_cost.py and construction-cost.txt, read_cost.py and read-cost.txt, spark-bench-infer.txt, spark-bench-infer-instructions.txt, spark-bench-schema.txt, identity.txt, construction-shapes.txt, r5-shapes.txt, state-probes-db8dd39.txt, exit-cases.txt, interval_first_built_busy.py and its result, m16b-lazy-unnamed-prototype.txt, add_prefixes.py and add-prefixes.txt, subclass_copy.py and subclass-copy.txt, user_module_schema.py and user-module-schema.txt.
• Tests, eight workers, a 600 second timeout, from each clone's root, over the reply's selection (round-5/selection.txt): Release 3327 passed, 8 skipped, 4 xfailed in 46.03s; FORCE_ASSERT 3320 passed, 15 skipped, 4 xfailed in 52.74s, both as the reply states; the default connection file alone on the assertion build 5 passed. The full fast suite on the head's Release build, eight workers, a 600 second timeout: 17633 passed, 114 skipped, 164 xfailed in 712.68s (round-6/tests-head-release-fast-suite.txt).
• Mutations on the assertion clone, each over tests/fast/test_exit_leak_report.py and tests/fast/spark and each undone (round-6/probes/mutate-r6.sh and run-mutations.sh, results round-6/mutations.txt): m16 unnamed struct parsed on every read 2 failed; m16b no parse at construction, passes (alone 5 passed); m17 map composed on every read 2 failed; m18 no import loop 2 failed (alone 4 failed, 1 passed); m9 cached property 3 failed; m15 cache dict in types.py 3 failed; m21 types.py at 7ab2576 2 failed; m22 array composed on every read 2 failed; m24, m25, m26 one __reduce__ removed, 1 failed each; m27 composed types back into the cache 1 failed; m28 interval parsed once, passes; m29 decimal composed on every read, passes. m16b run again when the round was finished, with the same result (1870 passed, 15 skipped, 2 xfailed, alone 5 passed; round-6/tests-variant-m16b-rerun-assert.txt), the clone clean at db8dd39 afterwards.
• Outside reviews: Codex at xhigh, read only, on the delta and the description's rules (round-6/codex-report.md, prompt beside it, log deleted); the code-review skill at high over 7ab2576..db8dd39 with the merge base named, nine items returned inline (round-6/code-review-findings.md), each checked here: its first, third and eighth are findings 1 and 2 again, its sixth (add parses every prefix) is folded into finding 1 (round-6/probes/add_prefixes.py, add-prefixes.txt), its second is finding 3 (subclass_copy.py, subclass-copy.txt), the unused constant in its ninth was already a smaller thing, and the rest are under outside findings not taken.
• CI on db8dd39: 22 checks pass, among them the full API build with assertions, six seeds, both sanity legs, the sdist, code quality, the submodule check, the pandas 2 leg and nine of the sixteen abi3 test legs; seven abi3 test legs (py3.12 and py3.15 on macOS arm64 and x86_64, py3.12 and py3.15 on Windows amd64, py3.13 on manylinux aarch64) were still pending at 01:54Z; none is red (round-6/pr-checks-db8dd39-final.txt).

What held

• A pickled decimal, array, map and schema answer in a fresh interpreter in both default connection states, and equal fresh builds for every protocol; copies and deep copies keep their type and compare equal.
• The cache holds 29 entries after 100,000 distinct decimals, arrays and maps, and no DuckDBPyType held on a composed instance shows in any exit child: every composed type the package builds is built in a function, none at module level or in a default argument.
• Every shape of the round 4 and round 5 state probes, built idle or inside the state, answers in both states, and createDataFrame with a schema costs what it costs on the merge base.

[el-marvin]

Parsing an unnamed struct's text in StructType.__init__ makes every spark DataFrame whose relation holds a struct without field names depend on the default connection, so spark.sql("select row(1, 2) r").collect() now fails while the default connection is busy or after a failed statement in its transaction, where the merge base answers.

Severity: blocking. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

        self._needConversion = [f.needConversion() for f in self]
        self._needSerializeAnyField = any(self._needConversion)
        self._parse_unnamed()

A session has a connection of its own, but DataFrame.__init__ builds its schema with duckdb_to_spark_schema (dataframe.py:587), and type_utils.convert_nested_type builds a StructType for every tuple column (type_utils.py:98 to 101). With this line, that construction parses TUPLE(INTEGER, INTEGER) through DuckDBPyType(text) on the default connection, the session's type in hand notwithstanding. round-6/probes/session_tuple.py, results in round-6/probes/session-tuple.txt:

	merge base	7ab2576	db8dd39
spark.sql("select row(1, 2) r").collect(), busy	ok	ok	ResourceInUseException
the same, after a failed statement	ok	ok	TransactionException
.schema of it, both states	ok	ok	refuses
spark.sql("select row(1, 'a') r, 3 x").select("x").collect(), both states	ok	ok	refuses
spark.sql("select {'a': 1} s").collect(), both states	ok	ok	ok
spark.sql("select list_zip([1], [2]) z").collect(), both states	refuses	refuses	refuses

Inside the state it fails every time until that text has once been parsed in the process while the default connection was free, and it is reached by any query that returns row(...) or a SQL tuple, even when the user selects another column. The description's "the session's casts and createDataFrame keep working" was true for these queries on the merge base and at 7ab2576.

This was the first of the two options my round 5 review offered for the unnamed struct; I did not check what builds such structs, and I withdraw that option. The construction rule also does not hold as written for a struct loaded from a pickle, because unpickling restores __dict__ without running __init__: round-6/probes/pickle_edges.py loads StructType([StructField("", UnsignedShortType())]), and a schema whose field is such a struct, in a fresh interpreter, and their first read raises ResourceInUseException while the default connection is busy (round-6/probes/pickle-edges.txt), against the comment at line 888.

Construction time parsing also parses every prefix of a struct built field by field: four add calls put TUPLE(BIGINT) up to TUPLE(BIGINT, VARCHAR, DATE, DOUBLE) into the cache, four parses on the default connection and four entries kept for the life of the process, where 7ab2576 parsed only the last text, on its read (round-6/probes/add_prefixes.py, add-prefixes.txt).

Removing both self._parse_unnamed() calls (here and in add, line 971) on the assertion build, which restores 7ab2576's parse on the first read of a text, makes every row of the table above answer as on the merge base and keeps tests/fast/test_exit_leak_report.py and tests/fast/spark green (1870 passed, 15 skipped, 2 xfailed; round-6/probes/m16b-lazy-unnamed-prototype.txt). What it gives back is round 5's corner: a struct without field names that the program builds while idle and first reads inside the state refuses, where the merge base refused on every read.

Ask: parse the unnamed text on the first read of that text again, and say in the description, the test module's docstring and the comment at line 888 that a struct without field names is the one shape parsed on its first read; or, if construction time parsing stays, let the conversion from an engine type hand that type in so building a DataFrame's schema never parses. Pin it with a session query over row(1, 2) inside each state. A typed constructor for a struct without field names in the binding would remove the last parse, which is a separate change.

[el-marvin]

The idle half of these tests builds the same type texts the busy half then reads or builds, so three weaker versions of the change pass every test: an unnamed struct not parsed at construction, an interval parsed once into the cache, and a decimal composed on every read.

Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

        "unnamed_struct": StructType([StructField("", UnsignedShortType())]),
        "array_of_unnamed_structs": ArrayType(StructType([StructField("", UnsignedShortType())])),

Mutations on the assertion build, each over tests/fast/test_exit_leak_report.py and tests/fast/spark with eight workers, each undone (round-6/probes/mutate-r6.sh, results round-6/mutations.txt):

• m16b, both self._parse_unnamed() calls removed (types.py:871 and :971): 1870 passed, 15 skipped, 2 xfailed, the same as unmutated, and 5 passed with this file alone. Both structs above have the text TUPLE(USMALLINT); ArrayType.__init__ reads its element's type (types.py:665), so the array parses that text while the connection is idle, and the bare struct's first read inside the state finds it cached. StructType.add has no test.
• m28, DayTimeIntervalType.duckdb_type returning _parsed("INTERVAL") instead of duckdb.sqltypes.INTERVAL: passes. ArrayType(DayTimeIntervalType()) at line 84 parses INTERVAL while idle, so the intervals built inside the state at lines 102 and 105 never reach a parse. In a fresh process, the same code refuses an ArrayType(DayTimeIntervalType()) first built inside the state.
• m29, DecimalType.duckdb_type composing duckdb.decimal_type on every read: passes, while the description says two reads of a decimal give the same object; the identity loop at line 99 covers the array, the map and the unnamed struct.

No test runs a session query whose result holds a struct without field names, which is why finding 1 passes the suite.

Ask: build the shapes that must not parse inside the state with texts the idle half does not build, or in the child interpreter you added for the singletons, where nothing ran before; add a struct built through add, a session query over row(1, 2) in each state, and the decimal to the identity loop.

[el-marvin]

The three new __reduce__ methods name their class, so copy.copy, copy.deepcopy and pickle of a subclass of DecimalType, ArrayType or MapType give back the base class, where the merge base's copy.copy kept the subclass.

Severity: minor. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

    def __reduce__(self) -> tuple[type["DecimalType"], tuple[int, int]]:  # noqa: D105
        # The engine type does not pickle; it is composed again where the pickle is loaded.
        return DecimalType, (self.precision, self.scale)

round-6/probes/subclass_copy.py builds class MyDecimal(DecimalType): pass and the same for the array and the map, sets an attribute on each instance, and copies it (results in round-6/probes/subclass-copy.txt):

	merge base	7ab2576	db8dd39
copy.copy	the subclass, attribute kept	the subclass, attribute kept	the base class, attribute gone
copy.deepcopy and pickle	TypeError, the engine type does not pickle	the subclass, attribute kept	the base class, attribute gone

No class in the package derives from these three, so only a program's own subclass reaches this. Ask: return type(self) instead of the class name in the three methods (lines 395, 670 and 733); an attribute set on an instance may stay lost.

[el-marvin]

Changes requested. I reviewed 5f57480, one commit on top of round 6's db8dd39 that touches two files: a struct without field names parses its TUPLE(...) text on the first read of that text again, and neither its constructor nor add parses; the three __reduce__ return type(self); and the default connection tests now build different type texts before and inside the state, a struct built through add and read once, row(1, 2) session queries in each state, the decimal in the identity loop, an array of intervals and a decimal first built in the child interpreter, and an array and a map with non default nullability in the pickle test. No C++ changed, so I ran the head as a Release build and as a build with FORCE_ASSERT (the assertion clone checked out at 5f57480 without a rebuild), against the merge base build, and for add against 7ab2576's spark files. Round 6's blocking finding is fixed: every session query over row(...) answers in both default connection states, as on the merge base, and add parses nothing. Each weaker version round 6 named now fails a test. The premise holds, and the approach sits at the right layer for this pull request; the one parse left, for a struct without field names, needs a constructor in the binding, which is a separate change. What is left is small, but two items must change before merge: two parts of this commit (the parse taken out of add, and type(self)) can be reverted with every test green, and two sentences of the description claim more than the head does, one of them about the cache's size. One minor item: type(self) hands a subclass's own constructor the base class's arguments. The full fast suite passes on the Release head.

Since the last review

• Round 6, 1 (parsing a struct without field names at construction made session queries over row(...) fail in both states): addressed. round-6/probes/session_tuple.py in both states on the head and the merge base (round-7/probes/session-rerun.txt): spark.sql("select row(1, 2) r").collect(), its .schema and spark.sql("select row(1, 'a') r, 3 x").select("x").collect() answer on both builds; list_zip refuses on both, as the description says. Four add calls leave the cache as it was and the first read parses the final text once, as at 7ab2576; a parse counter that also runs on the merge base shows the same (round-7/probes/add-prefixes.txt, add_prefixes_counted.py). Putting the construction parse back (mutation m30) fails both state tests, alone and in the selection. A pickled struct without field names now parses on its first read in another interpreter, which is the rule the description and the comment at types.py:875 state (round-7/probes/pickle-edges.txt).
• Round 6, 2 (the idle half built the texts the state half read, so three weaker versions passed): addressed for the three. m30 (the struct parsed at construction, the inverse of round 6's m16b now that the head no longer does it) 2 failed; m28 (the interval parsed once into the cache) 3 failed, the child among them; m29 (a decimal composed on every read) 2 failed; each alone and in the selection. The session query over row(1, 2) in each state and the decimal in the identity loop are in. The struct built through add is in too, but built and read while idle, so what add must not do has no test; that is part of finding 1.
• Round 6, 3 (__reduce__ named its class): addressed as I asked: a subclass of each of the three comes back as the subclass from copy.copy, copy.deepcopy and pickle, equal to the original (round-7/probes/subclass-rerun.txt). My ask was incomplete: a reduce that returns constructor arguments calls a subclass's own constructor with the base's arguments and carries no attribute set on the instance. That is finding 3, with a fix that keeps all of it.
• Round 6's smaller things: taken. RELEASE_THE_DEFAULT_CONNECTION is gone; the child asserts that the default connection refuses (lines 65 to 70); the pickle test uses containsNull=False and valueContainsNull=False and compares repr, which prints both flags (lines 179, 180 and 185); the description gives the read and inference cost and names the struct without field names as the exception for a type built inside the state.
• Earlier rounds, rerun at the head: a pickle dumped and loaded in fresh interpreters answers for every type and for createDataFrame with the loaded schema (round-7/probes/pickle-fresh.txt); 100,000 distinct arrays and maps leave the cache at 29 entries (cache-growth.txt); the import loop removed (m18) fails 2 tests in the selection and 4 alone; the cached property (m9) 3; the cache dict in the types module (m15) 3, which only the exit tests catch; an array or map composed on every read (m22, m17) 2 each; one __reduce__ removed (m24, m25, m26) 1 each; composed types back in the cache (m27) 1. Round 5's struct shapes and every state of a struct without field names (round-7/probes/r5-shapes.txt, unnamed-states.txt) follow the stated rule and are never worse than the merge base, which refuses every such read inside the state.

Findings

1. Two parts of this commit can be reverted with every test green, the parse taken out of StructType.add and type(self) in the three __reduce__, and no test reads a singleton twice, so the description's "two reads of one atomic type ... give the same object" is not pinned either (tests/fast/spark/test_spark_types_default_connection.py:85). Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced. Details in the inline comment.
2. Two sentences of the description claim more than the head does: "the cache stays at 29 entries however many distinct containers a program builds and drops" does not hold for an array or map over a struct without field names, and "the session's casts, createDataFrame and queries keep working" in both states does not hold for a cast to a struct with field names, createDataFrame from pandas with a schema, a query over a list or map of row(...), or a frame built from a tuple column's own schema (the description). Severity: should fix. Category: docs. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced (the claims; every refusal in the second half is the merge base's too). Details below.
3. return type(self), (self.precision, self.scale) hands the base class's constructor arguments to a subclass's own constructor, so copy.copy, copy.deepcopy and pickle of a subclass that defines __init__ raise TypeError, where the merge base's copy.copy answered; and all three drop an attribute set on any instance (duckdb/experimental/spark/sql/types.py:397, also 672 and 735). Severity: minor. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced. Details in the inline comment.

Finding 2 in detail

The description says decimals, arrays and maps hold their type on the instance, "so the cache stays at 29 entries however many distinct containers a program builds and drops." An array or map reads its element's type when it is built (types.py:665 and 726), so one built over a struct without field names puts that struct's text into the cache through _parsed at types.py:878. round-7/probes/unnamed_growth_paths.py, from a fresh import (unnamed-growth-paths.txt):

what the program does	entries added at the head
1,000 ArrayType(StructType([StructField("", StructType([StructField(f"f{i}", LongType())]))]))	1,000
200 session queries over row(i, 'a'), one shape	0
38 session queries select row(1::DECIMAL(p, 0), 2) r, .schema and collect()	0
38 session queries select [row(1::DECIMAL(p, 0), 2)] r, collect()	38
200 session queries select [row({'k<i>': 1})] r with a changing alias, collect()	200

100,000 distinct texts read and dropped leave 100,029 entries and 101 MB more resident memory, against 42 to 43 MB on the merge base, which parses on every read and keeps nothing: about 600 bytes a text (round-7/probes/unnamed_growth.py 100000, unnamed-growth.txt, two runs of each build, alternating). The cache test at line 211 builds only structs with field names, so it does not see this. I do not ask for a bound: one parse per distinct text is the rule this change chose for the one type no constructor builds, the entries grow only with the distinct tuple shapes a program reads, of which an ordinary program has few, and parsing on every read instead is the merge base, which refuses every such read inside the state. But the sentence is false as written, and the cost of the rule belongs in the description: each distinct text of a struct without field names that a program reads, also through an array or map built over one, stays in the cache for the life of the process.

The description also says that a type built before the state and first read inside it, a type built inside it (the struct without field names excepted), "and the session's casts, createDataFrame and queries keep working". That holds for what the types module builds and composes, and not for these, each of which refuses in both states on the merge base and the head alike (round-7/probes/session_casts.py, tuple_paths.py and code_review_items.py; results in session-rerun.txt, tuple-paths.txt and code-review-items.txt):

• frame.select(F.col("s").cast(StructType([StructField("a", LongType())]))): the cast checks the struct's size with a cast to a TUPLE(VARCHAR, ...) it parses (column.py:504);
• spark.createDataFrame(pandas_frame, schema): _cast_types parses each target's text (dataframe.py:2333);
• spark.sql("select [row(1::SMALLINT, 2)] r").collect() and spark.sql("select map([1], [row(1::BIGINT, 2)]) m").collect(): the array or map built for the frame's schema reads its tuple element;
• for df = spark.sql("select row(1::TINYINT, 2::TINYINT) r") built before the state, spark.createDataFrame(df.collect(), df.schema) and df.select(F.col("r").cast(df.schema["r"].dataType)) inside it: the first read of a tuple's text, the rule's exception in its other direction;
• spark.sql("select row(TIMESTAMPTZ '2020-01-01', 1) r").show(): the cast to the session time zone builds a new tuple and reads it (dataframe.py:687).

None of these is a regression: the head answers in places where the merge base refused, spark.sql("select row(1::HUGEINT, 2) r").collect() (a singleton first built inside the state on the merge base) and a union of two frames over row(...). Ask: replace the cache sentence with what holds, and narrow the casts sentence to the types the module builds, naming the struct without field names in both directions (built inside the state, or built before it and first read inside it) and the struct casts and pandas frames that still parse type text.

Smaller things

• tests/fast/spark/test_spark_types_default_connection.py:84: "Composed, never read" comes right before the read of the struct built through add at line 86. The read is on purpose (it is what makes a second read parse nothing, and m16 fails without the cache), so the comment could say that struct is read once.
• tests/fast/spark/test_spark_types_default_connection.py:157: "no type text was parsed" is not so; the import parsed the 29 singleton names before the hold. "No text but the singletons' names" says what the test relies on.

Outside findings not taken

• Codex's severity for the reduce methods (should fix): taken as finding 3, minor, because only a program's own subclass of these classes, or an attribute it sets on an instance, reaches it; no class or code in the package does either.
• Codex's ask to separate the finite singleton cache from a retention policy for tuple texts, and the code-review skill's unbounded cache item: confirmed and taken as the first half of finding 2, not as a design finding, for the reasons given there.
• Codex: the child interpreter has no tuple case. A tuple's first read inside the state refuses by the stated rule, so a child could only pin that refusal; m30 already fails without one.
• The code-review skill's items on an array or map of tuples, a tuple built before the state and first read inside it, and show() of a tuple holding a TIMESTAMPTZ: confirmed, and the merge base behaves the same in each; they are evidence for the second half of finding 2, not regressions.
• The code-review skill's altitude item (a constructor for a struct without field names in the binding, or handing the relation's type into the schema conversion): the separate change round 6 recorded, which the author names as the follow-up.
• The code-review skill: each read rebuilds the tuple text, and a nested tuple parses each level once. The merge base parsed every level on every read.
• The code-review skill: two threads reading a new tuple text at once could both parse it. Not reproduced: in 300 trials of eight threads reading a new text behind a barrier, every thread got the same object (round-7/probes/code-review-items.txt); and the merge base gave a new object on every read.
• The code-review skill: the comment at types.py:875 is three sentences long. The repository has no rule on comment length.

What I verified

• Head 5f57480; range 4583a30..5f57480, eight commits and no merge commit, 19 files, 931 added and 139 removed; this round's delta db8dd39..5f57480, two files, 57 added and 58 removed (round-7/delta.diff). The pull request diff from GitHub has the same files and added lines (round-7/pr-diff-5f57480.diff); GitHub reports the pull request mergeable into the current v2.0-cyanoptera. The description was read in full (round-7/pr-description-5f57480.md).
• Builds: the tree at 5f57480, Release (build.log ends BUILD OK 02:20:58); the assertion clone checked out at 5f57480, not rebuilt because no C++ changed, _duckdb._assertions True, clean at 5f57480 at the end; the merge base build at 4583a30, Release; a copy of the head's package with 7ab2576's types.py and _type_cache.py over the head's Release extension (round-6/r5pkg).
• Probes, every connection with memory_limit 8GB and 8 threads, in round-7/probes: subclass-rerun.txt (subclass_shapes.py, round-6's subclass_copy.py and state_prototype.py on both builds), add-prefixes.txt (round-6's add_prefixes.py on the head and 7ab2576, add_prefixes_counted.py on all three), add-inside-state.txt, session-rerun.txt, tuple-paths.txt, code-review-items.txt, unnamed-growth.txt, unnamed-growth-paths.txt, pickle-fresh.txt, cache-growth.txt, p35-prototype-shapes.txt, priming-.txt. (with plugin/cache_priming.py), and the outputs the first round 7 reviewer saved before it stopped (pickle-edges.txt, r5-shapes.txt, unnamed-states.txt, session-casts.txt, interval-first-built-busy.txt).
• Tests, from each clone's root, eight workers and a 600 second timeout where more than one file runs: the default connection file alone, Release 5 passed in 2.04s, assertion build 5 passed in 1.94s; tests/fast/test_exit_leak_report.py and tests/fast/spark, Release 1877 passed, 8 skipped, 2 xfailed in 24.88s, assertion build 1870 passed, 15 skipped, 2 xfailed in 21.40s. The full fast suite on the Release head, eight workers, a 600 second timeout: 17633 passed, 114 skipped, 164 xfailed in 671.12s (0:11:11), no failure or error (round-7/tests-head-release-fast-suite.txt).
• Mutations on the assertion clone, each over the default connection file alone and over the exit file and tests/fast/spark, each undone (round-7/probes/mutate-r7.sh and mutate-r7b.sh with run-mutations-r7.sh and run-mutations-r7b.sh; round-7/mutations.txt, mutations-r7b.txt; one result file per case, round-7/tests-variant--assert.txt and -alone-assert.txt): m16 (tuple parsed on every read) 2 failed alone and in the selection; m30 2 failed, 2 failed; m33 (parse in add) 5 passed, 1870 passed; m31 (map parsed on first read) 2 failed, 2 failed; m34 (every composed type parsed on first read) 4 failed, 4 failed; m32 (__reduce__ naming the class) 5 passed, 1870 passed; m28 3 failed, 3 failed; m29 2 failed, 2 failed; m17 and m22 2 failed each, both ways; m18 4 failed alone, 2 failed in the selection; m24, m25, m26, m27 1 failed each, both ways; m9 1 failed alone, 3 failed in the selection; m15 5 passed alone, 3 failed in the selection (the exit tests); m36 (a singleton read returning a copy) 5 passed, 1870 passed; p35 (the __getstate__ and __setstate__ prototype of finding 3) 5 passed, 1870 passed.
• Outside reviews: Codex at xhigh, read only, on the delta with the description's rules (round-7/codex-report.md, prompt beside it; three findings, all confirmed by probes here and taken as findings 1 to 3 in part, the rest listed above); the code-review skill at high over db8dd39..5f57480 with the merge base named, ten items returned inline and triaged with probes (round-7/code-review-findings.md).
• CI on 5f57480: 28 checks pass, among them the full API build with assertions, six seeds, both sanity legs, the sdist, code quality, the submodule check, the pandas 2 leg and fifteen of the sixteen abi3 test legs; the abi3 test leg py3.15 on macOS x86_64 was still pending at 03:35Z, and the unexpanded wheel matrix template skips; none is red (round-7/pr-checks-5f57480-final.txt).

What held

• Every session query over a bare row(...) answers in both default connection states, and so do the reads, casts and frames of the types module's own singletons, decimals, arrays and maps, built before the state or inside it; add parses nothing.
• A pickled decimal, array, map and schema answer in a fresh interpreter; the cache holds 29 entries after 100,000 distinct decimals, arrays and maps; the exit tests pass on both builds, and fail when the cache moves into the types module or onto the instances.
• No earlier test in the same pytest worker primes a text the head's state tests read: the cache holds only the import's names when the first state test starts, alone and in the selection.

[el-marvin]

Two parts of this commit can be reverted with every test green, the parse taken out of StructType.add and type(self) in the three __reduce__, and no test reads a singleton twice, so the description's "two reads of one atomic type ... give the same object" is not pinned either.

Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

    unnamed = StructType([StructField("", UnsignedShortType())]).add("", LongType())
    str(unnamed.duckdb_type)

This is the only add in the tests, and it runs while the default connection is idle, with a read right after. Mutations on the assertion build, each run over this file alone and over tests/fast/test_exit_leak_report.py and tests/fast/spark with eight workers, each undone (round-7/probes/mutate-r7.sh and mutate-r7b.sh, results in round-7/mutations.txt and mutations-r7b.txt):

• m33, the round 6 head's parse in add put back (if self.names and not any(self.names): self.duckdb_type before return self): 5 passed alone and 1870 passed, 15 skipped, 2 xfailed in the selection, the same as unmutated. With that parse, StructType().add("", ByteType()).add("", FloatType()) inside either state raises ResourceInUseException or TransactionException, where the head and the merge base answer (round-7/probes/add_inside_state.py, add-inside-state.txt).
• m32, the three __reduce__ naming DecimalType, ArrayType and MapType again, which is the round 6 head: 5 passed alone and 1870 passed in the selection. The pickle test pickles instances of the three classes themselves, never a subclass.
• m36, a singleton's read returning DuckDBPyType(cached), a new object that needs no connection (one built while another thread holds the default connection answers: round-7/probes/singleton_copy_busy.py, singleton-copy-busy.txt): 5 passed alone and 1870 passed in the selection. The identity loop at line 101 covers the decimal, the array, the map and the unnamed struct, not a singleton.

The weaker versions my round 6 review named now fail: the struct without field names parsed at construction again (m30) fails both state tests, alone and in the selection; the interval parsed once into the cache (m28) fails them and the child; a decimal composed on every read (m29) fails both. So do a map or every composed type parsed on its first read (m31 2 failed, m34 4 failed). A pytest plugin that lists the cache at the start of each test of this file (round-7/probes/plugin/cache_priming.py, results round-7/probes/priming-.txt.) shows the head's cache holding only the import's names when the first state test starts, alone and in the selection, so nothing at the head primes what the state half reads. Under a mutation it can: with m31, MAP(VARCHAR, DOUBLE), the text of the map the idle half builds, was already in the cache from tests/fast/spark/test_spark_create_dataframe.py in the same worker when the state tests started, and m31 failed only through the map built inside the state, whose text no earlier test in that worker had read; in the first m31 run the busy test also got past the idle map's read at line 99 and failed at line 105 (round-7/tests-variant-m31-map-parsed-on-first-read-assert.txt). The child interpreter is the one place where nothing runs first, and it has no map.

Ask: inside each state, build a struct without field names through add from leaves built before the state; copy, deep copy and pickle a subclass with its own constructor and an attribute (which pins the fix in the comment at types.py:397); read a singleton twice in the identity loop; and build a map in the child interpreter with the array and the decimal.

[el-marvin]

return type(self), (self.precision, self.scale) hands the base class's constructor arguments to a subclass's own constructor, so copy.copy, copy.deepcopy and pickle of a subclass that defines __init__ raise TypeError, where the merge base's copy.copy answered; and all three drop an attribute set on any instance, so the copy no longer compares equal to the original.

Severity: minor. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

    def __reduce__(self) -> tuple[type["DecimalType"], tuple[int, int]]:  # noqa: D105
        # The engine type does not pickle; it is composed again where the pickle is loaded.
        return type(self), (self.precision, self.scale)

round-7/probes/subclass_shapes.py and round-6/probes/subclass_copy.py, run on both builds (results in round-7/probes/subclass-rerun.txt):

	merge base	5f57480
class MyDecimal(DecimalType): pass, and the same for the array and the map: copy.copy	the subclass, equal	the subclass, equal
the same, copy.deepcopy and pickle	TypeError: cannot pickle '_duckdb._sqltypes.DuckDBPyType' object	the subclass, equal
class Money(DecimalType) whose __init__(self, currency="EUR") calls super().__init__(18, 2), and class Tags(ArrayType) whose __init__(self) takes nothing: copy.copy	the subclass, currency kept, equal	TypeError: Money.__init__() takes from 1 to 2 positional arguments but 3 were given
the same, copy.deepcopy and pickle	TypeError: cannot pickle ...	the same TypeError as for the copy
a DecimalType(5, 2) or a MyDecimal(5, 2) with note = "kept" set on it: copy.copy	note kept, equal	note gone, copy == original is False
the same, copy.deepcopy and pickle	TypeError: cannot pickle ...	note gone, not equal

The copy is unequal because DataType.__eq__ compares __dict__ (line 104). A reduce that names a constructor and its arguments carries no instance state and cannot know a subclass's constructor; type(self) was what my round 6 review asked for, and that ask was incomplete. Restoring the instance's own state and composing only the engine type covers both. round-7/probes/state_prototype.py removes the three __reduce__ on the head and adds, per class,

def __getstate__(self):
    return {k: v for k, v in self.__dict__.items() if k != "_duckdb_type"}

def __setstate__(self, state):
    self.__dict__.update(state)
    self._duckdb_type = duckdb.decimal_type(self.precision, self.scale)  # list_type and map_type for the other two

and then Money("USD"), Tags(), a decimal with an attribute, a map of an array of decimals and a schema each come back from copy.copy, copy.deepcopy and pickle as their own class, with their attributes, equal to the original and with the right engine type (round-7/probes/subclass-rerun.txt). The same change made in the source on the assertion build (mutation p35 in round-7/probes/mutate-r7b.sh) keeps this file and tests/fast/spark with the exit tests green (5 passed alone, 1870 passed, 15 skipped, 2 xfailed; round-7/mutations-r7b.txt), gives the same rows for every subclass above, and a pickle dumped and loaded in two fresh interpreters still answers for every type of round-5/probes/pickle_fresh.py (round-7/probes/p35-prototype-shapes.txt).

No class in the package derives from these three or sets an attribute on an instance, so only a program's own subclass or attribute reaches this. Ask: __getstate__ and __setstate__ in place of the three __reduce__, with the subclass test the comment on the test file asks for.

[el-marvin]

This is ready to merge. I reviewed 473aa15, one commit on top of round 7's 5f57480 that touches two files: DecimalType, ArrayType and MapType replace their __reduce__ with a __getstate__ that leaves the engine type out of the pickled state and a __setstate__ that restores the instance's attributes and composes the engine type again; and the default connection tests add a struct without field names built through add inside each state and never read, a singleton read twice in the identity loop, a map in the child interpreter, and a test that copies, deep copies and pickles three subclasses. No C++ changed, so I ran the head as a Release build and as a build with FORCE_ASSERT (the assertion clone checked out at 473aa15 without a rebuild), against the merge base build, and for the outside reviewers' copy cases against 5f57480's types module. Round 7's three findings are addressed: each weaker version it named now fails a test, the description's cache and casts sentences hold as written, and a subclass keeps its class, its attributes and what its own constructor set through copy, deep copy and pickle. The premise holds and the approach sits at the right layer. Nothing here blocks merging. The item below is a nit the author may take or leave: no test copies or loads a decimal, array or map inside a default connection state, so __setstate__ could parse its text with every test green. Outside the code, a maintainer still has to see the last six abi3 test legs on 473aa15 finish; they were pending and none was red at 04:40Z. The full fast suite on the Release head passed but for one filesystem test outside this change, which failed once under a machine load of about 250 and passes alone and in its file.

Since the last review

• Round 7, 1 (two parts of the commit could be reverted with every test green, and no singleton was read twice): addressed. Mutations on the assertion build, each run over the default connection file alone and over tests/fast/test_exit_leak_report.py with tests/fast/spark at eight workers, each undone (round-8/probes/mutate-r8.sh, run-mutations-r8.sh; results in round-8/mutations.txt and round-8/tests-variant-[-alone]-assert.txt): the parse put back in add (m33) fails both state tests at line 108, 2 failed alone and in the selection; a __reduce__ naming the class put back in front of the state methods (m32, the round 6 head) fails all nine cases of the subclass test, 9 failed both ways; the type(self) reduce of round 7 (m32b) fails the same nine; a singleton read returning DuckDBPyType(cached), a copy that needs no connection (m36), fails both state tests at line 106, 2 failed both ways. The map in the child now pins a map's first read: a map parsed on its first read (m31) fails the child as well as both state tests, 3 failed both ways, where round 7's run caught it only through the map built inside the state.
• Round 7, 2 (two sentences of the description claimed more than the head did): addressed. The cache sentence now states the rule, one entry per parsed name or tuple text for the life of the process, and its examples hold: a cast to a struct without field names, createDataFrame with a tuple schema and a query over [row(...)] of a new shape each add one entry, a cast to a struct with field names over a tuple column adds none, and 100,000 distinct arrays and maps of named structs leave 29 entries (round-8/probes/module-casts.txt, reruns-misc.txt). The casts sentence is narrowed to the module's own types, and it holds for them: eleven casts, to a long, a string, decimals, an array of longs, an array of decimals, a map and an interval, given as spark types and as text, and createDataFrame with a schema of a decimal, an array of intervals and a map, and with a schema given as text, all answer in both states at the head, where the merge base refuses eight of these thirteen (module-casts.txt); every row of round 7's session, cast and tuple probes is the same on both builds (session-rerun.txt), so the paths the sentence now lists as refusing on both still do.
• A correction to my round 7 figures, in the open. I wrote that 100,000 distinct tuple texts left "101 MB more resident memory, against 42 to 43 MB on the merge base", and you wrote "about 50 MB"; the two measure different things, and yours is right for what the sentence says. The 101 MB is the growth of resident memory, which includes 42 MB that the allocator keeps after the loop whether or not anything is cached: the merge base, which caches nothing, and the head with each new entry removed right after its read both grow by the same 42 MB. What the 100,000 entries themselves hold, measured as what removing them frees, is 35.4 MB of engine types (live bytes of the C allocator, mallinfo2, in a run without tracemalloc, whose own records the C allocator also counts) and 12.8 MB of Python keys and wrappers (tracemalloc), 48 MB in all, about 480 bytes a text for TUPLE(STRUCT(f<i> BIGINT), VARCHAR) (round-8/probes/cache_bytes.py, cache-bytes.txt). Resident memory ends about 59 MB higher than without the cache, which is the "about 600 bytes a text" I gave.
• Round 7, 3 (type(self) in the reduce handed a subclass's constructor the base class's arguments and dropped attributes): addressed. round-7/probes/subclass_shapes.py and round-6/probes/subclass_copy.py on both builds (round-8/probes/subclass-rerun.txt): Money("USD") with its own constructor, Tags() without arguments, plain subclasses of all three classes, and a DecimalType or subclass with an attribute set on the instance come back from copy.copy, copy.deepcopy and pickle as their own class, with their attributes, equal and with the right engine type; the merge base answers copy.copy and refuses the other two with cannot pickle '_duckdb._sqltypes.DuckDBPyType' object. The new subclass test fails six of its nine cases on the merge base, the deep copies and pickles (round-8/tests-new-subclass-test-on-base.txt). A pickle dumped in one interpreter and loaded in a fresh one answers for every type of round-5/probes/pickle_fresh.py on the Release and the assertion build (pickle-fresh.txt), and for the three subclasses also inside both default connection states (setstate-states.txt). Removing one class's two state methods (m24r, m25r, m26r) fails 5, 3 and 3 tests; removing only the three __setstate__ (m37) fails 10; a __getstate__ that keeps the engine type (m39) fails 7.
• Round 7's smaller things: taken (the comments at lines 87 to 88 and 164).
• Earlier rounds, rerun at the head: the struct without field names parsed at construction (m30) 2 failed both ways; the interval parsed once into the cache (m28) 3 failed alone, and in the selection 1 failed (the child) in the main run and 3 failed in a rerun: the two state tests pass under m28 only when an earlier spark test in the same worker has already read an interval, which depends on how the workers share out the files, and the child interpreter, which nothing runs before, fails in every run; a decimal composed on every read (m29) 2 failed; composed types in the cache (m27) 1 failed. Session queries over row(...) answer in both states as on the merge base, list_zip refuses on both (session-rerun.txt), and a struct built through two add calls inside either state answers on both builds (reruns-misc.txt).

Findings

1. No test copies, deep copies or loads a decimal, array or map while the default connection is busy or after a failed statement, so a __setstate__ that parses its type text (m38) passes every test, though copy.copy of a decimal then refuses in both states where the merge base and this head answer (tests/fast/spark/test_spark_types_default_connection.py:106). Severity: minor. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced. Details in the inline comment.

Smaller things

• duckdb/experimental/spark/sql/types.py:395 to 401, 674 to 680 and 741 to 747: the three __getstate__ are the same line, and each class now composes its engine type in two places, its __init__ and its __setstate__. A later change to one of them (a fixed size array, say) would leave the other building the old type, and nothing would fail until a copy. One __getstate__ on a shared base, and one method per class that composes the type and that both __init__ and __setstate__ call, would keep a single expression per class.
• The description: "built inside that state, or built before it and first read inside it, its text parses on the default connection and refuses" reads as if building such a struct inside the state refuses. Building parses nothing (lines 107 to 109 build one in each state); what refuses is the first read of a text no earlier read parsed. "first read inside that state, whether built before it or inside it" says what the code does.

Outside findings not taken

• Codex: a subclass that declares __slots__ (for the base class's own precision and scale, or for an attribute of its own) loses those attributes or raises AttributeError on copy, deep copy and pickle, because __getstate__ returns only the instance dict. Confirmed (round-8/probes/outside-items.txt), and the merge base's copy.copy kept them; not taken, as input nobody writes: these classes' instances keep a __dict__, so slots on a subclass save nothing, and the merge base refused deep copy and pickle for every subclass.
• Codex: the subclass test does not assert that the copy is a new object, or that a deep copy detaches a child type, so a __copy__ returning the original would pass. The head's copy is a new object sharing the element and its deep copy detaches it (outside-items.txt), as Python's own copying does; the classes define neither __copy__ nor __deepcopy__, and the weaker versions named are code nobody would add.
• The code-review skill: an array or map over a struct without field names reads, and so parses, its element when it is built, which makes list_zip refuse while the default connection is busy, and loading such an array in a fresh interpreter parses at load. Confirmed on all three builds (round-8/probes/code-review-items.txt, setstate-states.txt); it is the stated exception in its "built inside that state" direction, and the merge base refuses the same queries.
• The code-review skill: after add changes a struct that an array already holds, the array's own engine type is stale, and a copy or pickle now composes the new one and compares unequal. Confirmed; the stale original is the merge base's too, whose copy.copy shared the stale type, and the copy is the one that agrees with simpleString(). Changing a struct after placing it in a container is not ordinary use.
• The code-review skill: compose the engine type lazily in the duckdb_type property instead, which needs no state methods. Holding the composed type on the instance is the design settled in rounds 5 and 6, behind the description's sentence that two reads of a decimal, array or map give the same object.
• The code-review skill: a subclass that replaces the private _duckdb_type after construction (an ARRAY of fixed size) gets the parent's type back from copy and pickle. Confirmed (code-review-items.txt); adversarial, as no code sets that private attribute, and the merge base refused deep copy and pickle for it.
• The code-review skill: the struct built through add inside the state is checked only through simpleString(). That is the point of the line: the parse put back in add (m33) fails both state tests there, and a first read of a struct without field names inside the state refuses by the stated rule, as on the merge base.
• The code-review skill: the subclass test writes note on parametrized objects shared by its nine cases, and overwrites Lookup's constructor value. Harmless: each case writes the same value before copying, and Money checks a value its constructor set (currency).
• The code-review skill: the comment at test line 87 is two sentences. The repository has no rule on comment length.

What I verified

• Head 473aa15; range 4583a30..473aa15, nine commits and no merge commit, 19 files, 988 added and 139 removed; this round's delta 5f57480..473aa15, two files, 71 added and 14 removed (round-8/delta.diff). The added lines of the pull request diff from GitHub are the same as the local range's (round-8/pr-diff-473aa15.diff); GitHub reports the pull request mergeable. The description was read in full against round 7's (round-8/pr-description-473aa15.md).
• Builds: the tree at 473aa15, Release (build.log ends BUILD OK 04:02:46); the assertion clone fetched from the tree and checked out at 473aa15, not rebuilt because no C++ changed, _duckdb._assertions True, clean at 473aa15 at the end; the merge base build at 4583a30, Release; a copy of the head's package with 5f57480's types.py over the head's Release extension (round-8/r7pkg).
• Probes, every connection with memory_limit 8GB and 8 threads, in round-8/probes: subclass-rerun.txt, setstate-states.txt (copy, deep copy and pickle inside each state, and pickles loaded in fresh interpreters inside each state, head and base), pickle-fresh.txt, pickle-edges.txt, session-rerun.txt, module-casts.txt, reruns-misc.txt (add inside each state, singleton copy, cache growth, the unnamed growth paths, which add the same 1,000, 0, 0, 38 and 200 entries as in round 7), cache-bytes.txt, m38-consequence.txt, outside-items.txt, code-review-items.txt.
• Tests, from each clone's root, eight workers and a 600 second timeout where more than one file runs: the default connection file alone, Release 14 passed in 1.55s, assertion build 14 passed in 1.75s; tests/fast/test_exit_leak_report.py and tests/fast/spark, Release 1886 passed, 8 skipped, 2 xfailed in 26.24s, assertion build 1879 passed, 15 skipped, 2 xfailed in 25.53s. The full fast suite on the Release head: 1 failed, 17641 passed, 114 skipped, 164 xfailed in 1128.37s (0:18:48) (round-8/tests-head-release-fast-suite.txt). The failure is tests/fast/test_filesystem.py::TestPythonFilesystem::test_a_python_error_from_a_read_on_an_engine_thread_reaches_the_caller_with_its_text, DID NOT RAISE at line 138: it needs an engine thread to read the file, and with the load average near 250 the submitting thread read it all. Neither the test nor the filesystem code is in this pull request; it passes alone five times on the head and three times on the merge base, and its file passes at eight workers, 47 passed, 1 skipped, 15 xfailed (round-8/tests-filesystem-rerun.txt).
• Mutations: as listed in the ledger, plus m38 (__setstate__ parses its text) 14 passed alone and 1879 passed, 15 skipped, 2 xfailed in the selection, which is the finding above.
• Outside reviews: Codex at high, read only, on the delta with the description's rules (round-8/codex-report.md, prompt beside it); the code-review skill at high over 5f57480..473aa15 with the merge base named, eight items returned inline and triaged with probes (round-8/code-review-findings.md). Neither filed a question.
• CI on 473aa15 at 04:40Z (round-8/pr-checks-473aa15-0440.txt; earlier reads beside it): 23 pass, among them the full API build with assertions, the six seeds, both sanity legs, the sdist, linting and type checks, the submodule check, the pandas 2 leg and ten of the sixteen abi3 test legs; six abi3 test legs pending (py3.12 and py3.15 on macOS arm64 and x86_64, py3.12 on Windows amd64, py3.15 on manylinux x86_64); the unexpanded wheel matrix template skips; none is red.

What held

• __setstate__ touches no connection for the module's own types: a decimal, an array, a map, a map of an array of decimals, a schema and the three subclasses copy, deep copy and pickle inside both states with no cache entry added, and loaded in a fresh interpreter inside either state they load and read (setstate-states.txt). An array over a struct without field names loaded in a fresh interpreter parses its element's text at load, and a struct without field names, alone or as a field, parses on its first read after the load; both follow the stated rule, and the merge base could not pickle any of them.
• The struct built through add inside each state (line 108) is built after the state is entered and only simpleString() is read, which reads no engine type; its first read inside the state would refuse by the stated rule, so it is left out on purpose, and the reads of an add-built struct that must answer (first read while idle at line 90, read again inside the state at line 104) are covered.
• Every cast to the module's own types and createDataFrame with a schema of them answer in both states; the description's cache figure matches the memory the entries hold.

[el-marvin]

No test copies, deep copies or loads a decimal, array or map while the default connection is busy or after a failed statement, so a __setstate__ that parses its type text (m38) passes every test, though copy.copy of a decimal then refuses in both states where the merge base and this head answer.

Severity: minor. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.

    for kind in (*built.values(), UnsignedShortType()):
        assert kind.duckdb_type is kind.duckdb_type

copy.copy, copy.deepcopy and pickle now go through __setstate__ (types.py:399 to 401, 678 to 680, 745 to 747), which composes the engine type again; on the merge base copy.copy shared the instance dict and never built a type. The head's composition needs no connection: a decimal, an array and a map built while idle copy, deep copy and pickle inside both states (round-8/probes/setstate-states.txt). Nothing pins that. Mutation m38 on the assertion build composes by parsing instead (round-8/probes/mutation-m38-setstate-parses-probe.patch):

        self._duckdb_type = DuckDBPyType(f"DECIMAL({self.precision},{self.scale})")

with the same for the array (f"{self.elementType.duckdb_type}[]") and the map. It gives 14 passed over this file alone and 1879 passed, 15 skipped, 2 xfailed over tests/fast/test_exit_leak_report.py and tests/fast/spark at eight workers, as unmutated (round-8/mutations.txt), while copy.copy(DecimalType(17, 4)) of a decimal built while idle raises ResourceInUseException while another thread holds the default connection and TransactionException after a failed statement, and so do the array, the map and the subclasses, and a deep copy or pickle of a schema holding them (round-8/probes/m38-consequence.txt). The subclass test and the fresh interpreter pickle test both run while the default connection is idle, and lines 110 to 112 pin the composition only at construction.

Ask, if you take it: copy, deep copy and pickle the idle built types inside the state here, for example assert copy.deepcopy(kind) == kind and assert pickle.loads(pickle.dumps(kind)) == kind in this loop, so the composition on load is pinned the way lines 110 to 112 pin it at construction.