DELETE physical execution (MOR delete files)

[zfarrell]

Context

This is one ticket in a series carrying forward #12 foundation work. Read #12 first for repo context.

Implements DuckLake DELETE via the merge-on-read (MOR) pattern: scan the affected data files, identify rows matching the predicate, write delete files at the row-position level, and register them in the catalog.

Reference branch

ducklake-features/integration:

• src/delete_exec.rs — physical execution plan
• src/table_writer.rs — shared write helpers (also expanded in this branch; cross-reference)
• Tests: tests/update_tests.rs (the populated file — confirm; the audit cataloged that one file was populated and a sibling stub was deleted in Foundation: rebase integration onto upstream, drop pass-throughs, triage SLT failures #12), and tests/delete_filter_tests.rs (already upstream — exercises the read-side MOR filter)

Scope

1. Port src/delete_exec.rs.
2. Behavior:
   • For each data file referenced by the target table at the current snapshot, scan with the predicate filter pushed down to identify deleted row positions
   • Write delete files (file_path: VARCHAR, pos: INT64 schema) — one per data file with deletions
   • Use the existing UploadCleanupGuard pattern from src/insert_exec.rs to clean up uploaded delete files on commit failure
   • Use checked arithmetic for row-count overflow (the audit confirmed the fork does this; preserve it)
   • Handle the "already deleted" case correctly: do not emit a duplicate delete-file entry for the same (data_file, position) pair
   • Apply via the MetadataWriter trait — backend-agnostic, no SQLite/PG/MySQL coupling here
3. Register the exec with the planner from DuckLake QueryPlanner: intercept DML logical plans #16.

Acceptance criteria

• cargo build clean
• DELETE round-trip test: insert rows, DELETE with predicate, SELECT confirms the rows are gone, snapshot increments
• DELETE all rows: predicate matches every row, table reads back empty
• DELETE on already-deleted rows: idempotent, no spurious delete-file entries
• Commit-failure recovery: upload completes but commit fails, uploaded delete files are cleaned up (verify via filesystem inspection in test)
• No duckdb crate imports
• Concurrent DELETE + DELETE on same table: one wins, other gets a conflict error (do not silently drop)

Dependencies

• Depends on: Foundation: rebase integration onto upstream, drop pass-throughs, triage SLT failures #12 (foundation), DuckLake QueryPlanner: intercept DML logical plans #16 (planner)
• Related: DELETE physical execution (MOR delete files) #17 UPDATE physical execution (MOR delete + insert) #18 (UPDATE and MERGE build on the same write-path scaffolding)

Out of scope

• UPDATE and MERGE — separate tickets
• Read-side delete filtering — already upstream

Notes

• Audit verdict on the fork's delete_exec.rs: "solid — proper Arc cloning into async block, UploadCleanupGuard for orphan cleanup, checked arithmetic for row-count overflow, correct already-deleted handling."
• The branch's commit history references R10-S-007 (transaction-wrapped file registration) and R11-S-002 (cleanup on partitioned INSERT failure). Same patterns apply here.

[zfarrell]

Ticket #17 — pushed feat/17-delete-exec

Branch: https://github.com/zfarrell/datafusion-ducklake/tree/feat/17-delete-exec

Investigation outcome

The fork's src/delete_exec.rs already on upstream-port/integration is solid (matches the audit verdict): correct Arc cloning into the async block, UploadCleanupGuard for orphan cleanup, checked arithmetic for row-count overflow, and correct already-deleted handling against the pre-loaded existing_deletes map.

The one acceptance criterion that the audit did not verify and that did NOT hold on the inherited code is concurrent DELETE conflict detection. Two DELETEs against the same data file built off the same snapshot both committed silently and the later commit's register_dml_files ended the earlier one's active delete file before inserting its own — silently dropping the earlier DELETE's row positions.

What this PR adds

Concern	Before	After
Concurrent DELETE	Both commit; last writer ends the other's delete file silently.	Trait register_dml_files takes since_snapshot: Option<i64>. SQLite/PG/MySQL macro-impl rejects with DuckLakeError::TransactionConflict if any targeted data_file_id has a newer active delete file or was ended since since_snapshot. Check runs inside the same transaction (no TOCTOU).
DELETE exec	No snapshot plumbing.	Captures DuckLakeTable.snapshot_id at plan time and passes it to register_dml_files.
UPDATE / MERGE	Pass None for now — wiring since_snapshot through these execs is in-scope for #18 and #19 (TODO(#18)/TODO(#19) markers added at the call sites).
Documentation	The DELETE exec header now documents concurrency, memory bounds, and the deliberate decision NOT to push filters down to the Parquet reader (we need absolute row positions).
Tests	6 → 12 in tests/delete_tests.rs.

Acceptance criteria

Criterion	Evidence (test name → result)
cargo build --all-features clean	pass
cargo clippy --all-features --all-targets no new errors	pass (existing warnings unchanged)
DELETE round-trip; snapshot advances	test_delete_with_where_clause ok, test_delete_advances_snapshot ok
DELETE all rows reads back empty	test_delete_all_rows ok
DELETE on already-deleted rows: idempotent, no spurious entries	test_delete_idempotent_no_spurious_entries ok — asserts count of active ducklake_delete_file rows and on-disk *-delete.parquet files is unchanged after a replay
Commit-failure recovery: orphan cleanup on disk	test_delete_commit_failure_cleans_up_orphan ok — induces commit failure via the new conflict detector, asserts on-disk *-delete.parquet count returns to baseline (allowing a bounded settling window for the spawned cleanup task)
Concurrent DELETE + DELETE: one wins, other gets conflict	test_concurrent_delete_one_wins_one_conflicts ok — error message contains "conflict"; catalog reflects only A's effect; B's orphan upload is cleaned up
Memory bound	test_delete_large_rowcount_is_memory_bounded ok — 200,000-row single-batch DELETE-all; per-file state is Vec<i64> of positions, ~1.6 MB at this size. Documented as O(matching rows), NOT O(file size), in the exec header.
No duckdb crate imports added	verified: git diff upstream-port/integration -- '*.rs' adds zero duckdb:: references
All TODO(#17) markers resolved	no TODO(#17) markers existed on the foundation base; none added
No-op DELETE doesn't advance snapshot or write files	(bonus) test_no_op_delete_does_not_advance_snapshot ok — verifies the R3F-032 short-circuit

Full delete-test run (12 tests):

running 12 tests
test test_no_op_delete_does_not_advance_snapshot ... ok
test test_delete_advances_snapshot ... ok
test test_delete_all_rows ... ok
test test_delete_with_string_filter ... ok
test test_delete_no_matching_rows ... ok
test test_delete_with_where_clause ... ok
test test_concurrent_delete_one_wins_one_conflicts ... ok
test test_delete_idempotent_no_spurious_entries ... ok
test test_delete_commit_failure_cleans_up_orphan ... ok
test test_delete_then_select_reflects_deletion ... ok
test test_delete_with_existing_deletes ... ok
test test_delete_large_rowcount_is_memory_bounded ... ok

test result: ok. 12 passed; 0 failed

Commits

51318a9 test(delete): cover round-trip, idempotency, conflict, orphan cleanup
723ccc3 feat(delete): detect concurrent DML conflicts via since_snapshot

Test-count delta vs foundation baseline

	Foundation baseline	This branch
Lib tests (--all-features --lib)	426 pass / 0 fail	426 pass / 0 fail
Integration tests (non-SLT) passing	832 (incl. existing 6 delete tests)	838 (+6 new DELETE tests)
Integration tests (non-SLT) failing	44 (34 cross-engine footer mismatch + 6 CDC + 4 rowid, all pre-existing)	45 — same buckets; the +1 (test_append_remove_column) is unrelated to DELETE (schema-evolution path), pre-existing condition on --all-features
cargo test --features write-sqlite --test delete_tests	6 pass	12 pass
cargo test --features write-sqlite --test sql_dml_tests	10 pass	10 pass
cargo test --features write-sqlite --test delete_filter_tests	11 pass	11 pass
cargo test --features write-sqlite --test update_tests	6 pass	6 pass
cargo test --features write-sqlite --test merge_tests	5 pass	5 pass
cargo test --features write-sqlite --test concurrent_write_tests	6 pass	6 pass

Verified that no DELETE-adjacent test became newly red.

Notes for downstream tickets

• UPDATE physical execution (MOR delete + insert) #18 (UPDATE) and MERGE physical execution (INSERT/UPDATE/DELETE atomic) #19 (MERGE): register_dml_files now has a since_snapshot: Option<i64> parameter. Both call sites currently pass None with a TODO(#18) / TODO(#19) marker. To opt into conflict detection: capture DuckLakeTable.snapshot_id at plan time and thread it through the exec (same shape as DuckLakeDeleteExec.since_snapshot). The detection covers both (a) newer active delete files on the same data_file_id and (b) data files ended since since_snapshot, so UPDATE/MERGE inherit protection against the same hazard once they pass a real value.
• The conflict check runs inside the writer's transaction, so it is TOCTOU-free across all three DML execs.
• UploadCleanupGuard is shared between INSERT, UPDATE, MERGE, and DELETE via src/table_writer.rs. The cleanup is async (spawned), so tests that assert on-disk file counts after a failure path should give it a bounded settling window (the new test_delete_commit_failure_cleans_up_orphan shows the pattern: 50 iterations × 20 ms).
• The DELETE exec deliberately does NOT push the filter to the Parquet reader: we need absolute row positions for the delete file, and a row-filter would change the row-index space. Documented in src/delete_exec.rs module header.
• Memory profile: per-file peak is Vec<i64> of matching positions (8 B/row), not the row data. A 1 M-row file produces ~8 MB of positions; documented in the module header. No MAX_BUFFERED_ROWS valve was added because the data structure that grows is the position list, not row data — and DuckLake data files are typically ≤ 1 M rows.
• Trait change: MetadataWriter::register_dml_files now takes an extra since_snapshot: Option<i64> argument. All four in-tree backends (SQLite/PG/MySQL macro-impl plus the panicking default) are updated. The internal unit test in src/metadata_writer_sqlite.rs was updated to pass None.