[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; the text of a struct without field names, the one shape with no typed constructor, is parsed when such a struct is built or a field added. 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, so the cache stays at 29 entries however many distinct containers a program builds and drops. A spark session has a connection of its own, so 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, and the session's casts and createDataFrame keep working, where the merge base failed for a singleton first built in that state. 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. 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 composes its type again where the pickle is loaded, so a schema handed to a multiprocessing worker works there. 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 for the first time while its default connection is held, another loads pickled types and casts with them, and building a thousand distinct containers 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.