Skip to content

Cleanups in the PyO3 binding #120

Description

@Xof

This issue groups 2 related findings.

PYTHON-10 — errors.rs states no test enumerates the Python exception classes; three test modules do, one of them named for the very issue cited

Location: python/src/errors.rs:17 · Severity: SMELL · Category: comment-accuracy

What the code does. errors.rs:17-19: "No test enumerates the Python classes (ISSUES.md I139); the engine-side is_fatal() exhaustiveness test (I104) guards the classification this fallback depends on." tests/test_exception_contract.py is headed "parametrized typed-exception contract tests (I139)" and pins ~15 concrete classes end-to-end; tests/test_errors.py:8-45 enumerates 13 operational and 10 fatal class names by string and asserts each subclasses the right tier; test_exception_contract.py:288-297 adds the three classes that file was missing.

Why it is a problem. The comment tells a maintainer that the Rust→Python exception mapping is unverified from the Python side, which invites either redundant new tests or an unchecked reorganization of to_py_err's arms on the belief nothing would catch a swap. The tests that would catch it exist.

Direction of a fix. Update the comment to reference tests/test_exception_contract.py and tests/test_errors.py, and state precisely what is still uncovered (per-variant coverage of the fatal arms, which test_exception_contract.py:301-310 itself flags).

PYTHON-11 — Nothing in the Python suite would fail if drain_insertion were dropped on the floor

Location: python/tests/test_runtime_config.py:51 · Severity: SMELL · Category: tests

What the code does. test_open_with_drain_insertion_lru_tail and ..._mru (tests/test_open.py:111-124) do with chisel.open(str(tmp_db), drain_insertion=...) as db: assert db is not None; test_open_in_memory_with_spillway_and_drain (:127-134) is the same shape; test_set_drain_insertion_accepts_both_variants (tests/test_runtime_config.py:66-72) only checks that a following allocate does not raise. Deleting .drain_insertion(drain_insertion.into()) from the builder chain (db.rs:265) or making set_drain_insertion (db.rs:601-603) a no-op keeps every one of them green. The sibling kwargs are not in this state: read_only got an end-to-end assertion for exactly this reason (tests/test_open.py:43-57, 'a dropped/inverted read_only kwarg ... would be caught'), and spillway_max_bytes is pinned by stats().spillway_max_bytes == spillway_max (tests/test_stats_defrag.py:31-45).

Why it is a problem. The kwarg is plumbed through two conversion layers (PyDrainInsertion -> From impl -> Options/set_drain_insertion); a transposed match arm in impl From<PyDrainInsertion> for chisel::DrainInsertion (db.rs:141-148) would silently invert the cache-drain policy for every Python user with no test failing anywhere in the suite.

Direction of a fix. Assert an observable difference — e.g. drive a spillway overflow under each policy and compare counters().cache_hits on a re-read of the drained pages — or, at minimum, expose the effective policy on stats() and assert the round-trip.


Filed from the clean-slate deep review of 2026-07-29. Full context, verification notes, and the delta against ISSUES.md are in docs/reviews/review-20260729-183138.md. Baseline at review time: 681 tests passing, clippy and fmt clean — none of these are toolchain-visible.

Metadata

Metadata

Assignees

No one assigned

    Labels

    review-2026-07-29Found by the clean-slate deep review of 2026-07-29severity:smellWorks but unidiomatic, duplicated, or hard to maintaintype:docsDocs contradict code; stale or wrong commentstype:testsCoverage gaps, tests that cannot fail

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions