feat(duckdb): append and upsert

[carolsimone]

Fourth runtime PR of the write-modes series (spec R4). Depends on the engine-port PR (#75, contract 0.8.0); stacked on #76 (Postgres) locally — base is feat/write-modes-postgres; retarget to main as the stack merges.

• Risk check first (spec section 8): DuckLake on the compose stack reports NOT NULL through duckdb_columns() and information_schema; pinned by a probe test.
• LakeWarehouse(WarehouseAdapter, AppendCapable, UpsertCapable): table_columns through the new LakeGateway.columns(), append (insert_arrow in one transaction), _upsert (MERGE INTO from the registered Arrow view in one transaction; no WHEN MATCHED when every column is in the key). Zero rows open no transaction.
• ensure_table(..., primary_key=...) is accepted and ignored: DuckLake rejects PRIMARY KEY/UNIQUE.
• The engine-contract conformance suite runs in the duckdb integration job.

🤖 Generated with Claude Code

https://claude.ai/code/session_0137E9XmyUXhwPTRrtNfivDw

[carolsimone]

we don't use relative import, only absolutes. Let's avoid this inconsistencies.

[carolsimone]

LiveColumn is a cross-package import — it comes from continuo_engine_contract, a separate distribution — so it can't be relative; it's absolute for the same reason the postgres adapter writes from continuo_engine_contract.port import LiveColumn. The relative imports in this file (from ..application..., from ..domain...) are intra-package and match the whole duckdb adapter, which uses relative imports throughout its layers (application/domain/infrastructure, ~9 files); the test suite imports the package absolutely.

So there isn't a one-line fix on this line: either the file is already consistent (cross-package absolute, intra-package relative), or you want the adapter's intra-package imports converted to absolute package-wide — a change to shipped 0.8.0 code that I'd land as its own commit/PR rather than fold into the write-modes feature. Which did you mean?

[carolsimone]

Why mixing up relative and absolute in a single repository? What's easier to understand a standardised codebase or something that mix up things all the time?

[carolsimone]

Agreed — standardised, not mixed. Done in 0525ffd: converted all 27 relative intra-package imports in the duckdb adapter to absolute (from continuo_duckdb_adapter...). It was the only package in the repo using relative imports; the runtime (52), the engine contract (3) and the postgres/trino adapters all use absolute, so the adapter now matches. No behaviour change; mypy, ruff and the duckdb suite (264) pass.

The cross-package LiveColumn line stays absolute as before — it has to be.

[carolsimone]

AI Code Review (OCR delegation mode)

Scope: main..HEAD (merge-base 722f4f3, 10 commits). 53 files changed; 21 reviewable after exclusions (docs, Dockerfiles, requirements pins, and test paths excluded by policy).

Coverage: 21/21 reviewable files examined (100%). No files skipped.

Review rules applied: correctness, security, error handling, concurrency, resource management, boundary cases, performance, maintainability (per ocr delegate rule checklists for Python sources and config files).

Findings

No High, Medium, or Low findings to report. Points specifically verified:

• SQL construction — DdlRenderer.merge_from routes every identifier through quote_identifier (double-quote escaped) and the MERGE source is a UUID-named registered view; postgres _insert_rows/_on_conflict use psycopg2.sql.Identifier and correctly escape literal % for execute_values' template reparse, including in EXCLUDED."pct%" (covered by the awkward-identifiers conformance case).
• Key invariant centralization — ensure_unique_key in keys.py runs in the UpsertCapable.upsert template method before any engine sees rows; empty/missing/float keys rejected, positional k{n} renames avoid collision with the count_all aggregate, and the pre-group_by NULL concern is moot since the contract requires key columns nullable: false and conform() rejects NULLs upstream.
• Contract validation — _validate_primary_key enforces upsert-only keys, exact column spelling (correct for postgres's quoted identifiers), case-insensitive duplicate rejection, and nullable: false on key columns. Wire entries in merge.py only carry mode_type/primary_key when non-default, keeping hashes and contract_version = 1 stable for replace nodes.
• Transaction discipline — postgres _transaction contextmanager preserves the prior BEGIN/COMMIT/ROLLBACK shape (rollback failure only logged, original error re-raised); duckdb append/_upsert skip 0-row writes so DuckLake records no empty snapshot.
• Capability model — TableIntrospection/AppendCapable/UpsertCapable are opt-in ABCs; both adapters implement all abstract members of the capabilities they declare. Trino (bumped to contract 0.8.0 without new capabilities) is unaffected: ensure_table's new primary_key parameter defaults to (), and the harness currently rejects non-replace modes before any adapter call.
• Harness interim guard — non-replace modes raise LoadError before the script runs or anything is written; cannot silently replace a table meant for append/upsert.

Verification

• tests (excl. integration): 650 passed. 3 failures in tests/test_image_smoke_validation.py are environment-only — they shell out to docker run for images not present locally (repository does not exist / TLS handshake timeout); unrelated to this diff.
• contract/tests: 163 passed.
• adapters/duckdb/tests (excl. integration): 263 passed.
• Real-engine integration suites (postgres/duckdb in Docker) not run locally; per CLAUDE.md they should pass in CI before merge.

[carolsimone]

AI Code Review — Low-severity follow-ups (non-blocking)

Posted at the author's request; these were below the reporting threshold of the main review. Nothing here blocks merge.

1. continuo_python_runtime/contract/loader.py — In the "mode not allowed for kind" error, mode_type is interpolated without !r (got {mode_type}) while the neighboring messages use !r. Cosmetic only; a weird value would render unquoted.

2. adapters/duckdb/continuo_duckdb_adapter/infrastructure/ddl.py (merge_from) — An empty key renders invalid SQL (ON ()). Currently unreachable: UpsertCapable.upsert → ensure_unique_key rejects an empty key before any engine is called. A one-line ValueError guard here would make the renderer self-defending if it's ever called outside the port.

3. adapters/duckdb/continuo_duckdb_adapter/infrastructure/session.py (columns) — The with self._guarded(): wrapper nests a second _guarded inside _run. Harmless (and it mirrors the existing table_exists shape); could call _raw directly if the double guard ever bothers anyone.

4. Both introspection queries match views — duckdb_columns() (ddl.py COLUMNS_QUERY) and information_schema.columns (postgres table_columns) also return rows for views. A view sharing the table's name would report columns instead of raising LookupError. No realistic trigger given how the runtime names objects.

5. contract/continuo_engine_contract/keys.py (ensure_unique_key) — The float-key check matches str(field.type) against {"halffloat", "float", "double"} instead of pa.types.is_floating(). Correct under the pinned pyarrow 25.0.1, but string-matching is fragile across pyarrow versions; the pa.types predicate would be version-proof (at the cost of the module's current "no pyarrow import" property, so a trade-off).

6. adapters/postgres/continuo_postgres_adapter/adapter.py (ensure_table docstring) — Still references "the contract 0.6.0 port signature"; the pinned contract is now 0.8.0. Pre-existing line outside this diff, noted while reading.

[carolsimone]

Dispositions:

2 — ddl.py merge_from empty key — fixed in 00bdb57: raises ValueError on an empty key (renders invalid ON ()), with a unit test. Unreachable via the port (ensure_unique_key guards first), now self-defending if called directly.

1 — loader.py !r consistency and 6 — postgres ensure_table docstring (0.6.0 → 0.8.0) — both are in code already merged to main (#74 and #76), outside this duckdb PR's files. Batching them into one small cleanup PR off main rather than widening this PR.

3 — session.py columns() double _guarded — leaving as-is. It deliberately mirrors table_exists (both call _run, which already wraps _guarded, inside an outer _guarded); switching only columns() to _raw would make the two siblings inconsistent for no behavioural gain. The inner translate re-raises a non-duckdb.Error, which the outer guard passes through — harmless.

4 — introspection matches views — deferring. The runtime creates tables, never views in the output schema, so a view colliding with the table name isn't something it produces. It also spans both duckdb (duckdb_columns()) and the already-merged postgres table_columns; if we want the table/view distinction it should be fixed in both together, not duckdb-only here. (Noted: table_exists uses duckdb_tables(), so there is a real tables-vs-views asymmetry with columns() worth aligning in that follow-up.)

5 — keys.py float check uses str(field.type) not pa.types.is_floating() — keeping the string match. keys.py imports pyarrow under TYPE_CHECKING only, by design: the contract package must not import pyarrow at runtime. pa.types.is_floating() needs a runtime import and would break that property. The set is correct under the pinned pyarrow 25.0.1, and the float-key regression test catches a change if the pin ever moves.