Skip to content

[epic] DELETE via PRIMARY KEY (reworked after adversarial review, replaces PR #19) #63

Description

@fightBoxing

Background

DELETE support for the Flink → Lance connector is tracked by #8. An initial attempt in #19 built the predicate from all non-null fields of every buffered RowData; that PR was closed after review (see #19 comments) because "all fields equal" is not a valid substitute for row identity — it silently over-deletes historical duplicates and drops rows whose types are not in the connector's type switch.

This epic re-plans the DELETE work along the grain of the wider Flink ecosystem (Iceberg, Paimon, StarRocks all treat declared primary key as the row-identity contract for CDC sinks) and against Lance v7's actual capabilities.

Design decisions (call them out up front so review is focused)

  1. Primary key is the row-identity contract, not _rowid. In real CDC pipelines (MySQL/PG → Flink → Lance), the upstream -D message carries the upstream row's identifiers, not Lance's internal _rowid. Only in the narrow "Lance A → Flink → Lance B replication" scenario is _rowid a viable pipeline-level identifier. PK-first is therefore the mainline path.

  2. Stable row IDs are a Lance-internal performance optimization, not a DELETE feature. Per the Rust docstring: "Experimental: if set to true, the writer will use stable row ids. These row ids are stable after compaction operations, but not after updates." Enabling stable row IDs helps compaction avoid rebuilding secondary indexes; it does not give us a stable delete key across CDC updates. Move write.enable-stable-row-ids out of the DELETE critical path.

  3. _rowid IN (...) as a Dataset.delete(predicate) argument is UNVERIFIED. Lance's filter parser accepts IN, but there is no official example of using the virtual _rowid column inside dataset.delete(...). In the DuckDB integration, _rowid IN (...) is a special-cased path routed to dataset_take, not a filter-parser feature (see lance-duckdb pushdown docs). We must validate this with a spike before designing around it.

  4. Full CDC changelog, not just DELETE. Real CDC streams emit +I / -U / +U / -D. Handling only RowKind.DELETE leaves UPDATE_BEFORE and UPDATE_AFTER semantically undefined, which causes duplicate PK rows in the Lance dataset. This epic must define behaviour for all four.

Non-goals

  • MemWAL sink migration — Lance's own answer to high-throughput streaming CDC, but blocked on upstream Java bindings. Tracked (and closed as not-planned) in [epic] Adopt Lance v7 MemWAL for streaming sink (blocked on upstream Java bindings) #64. This epic must stay forward-compatible with it: the PK-as-identity contract works for both Dataset.delete(predicate) today and a future ShardWriter.delete(pk).
  • Merge-insert / upsert atomicityDataset.delete + Dataset.append in two separate transactions is acceptable for v1. True atomic upsert is a follow-up.

Phased delivery

Phase 0 — Spike: validate Dataset.delete predicate surface (BLOCKING everything else)

Estimated effort: half a day. Blocker for Phase A/B/C.

Write a JUnit test in this repo (throwaway, not merged) that answers three concrete questions on org.lance:lance-core:7.0.0:

  • Q1: Given a dataset created with WriteParams.enableStableRowIds(true), does dataset.delete("_rowid IN (0, 1, 2)") succeed, or does it throw (ColumnNotFoundError / equivalent)?
  • Q2: Given a dataset with a user column id BIGINT, does dataset.delete("id IN (1, 2, 3)") succeed, and does it delete exactly those rows?
  • Q3: For a composite PK (user_id BIGINT, event_ts TIMESTAMP), what is the syntax accepted by Lance's filter parser? Options to test: (user_id, event_ts) IN ((1, ts'2026-01-01'), ...), or the fallback (user_id = 1 AND event_ts = ts'...') OR (...). Record what actually works.

Deliverables:

  • A short comment on this issue with the Q1/Q2/Q3 results (pass / fail + exception message if failed).
  • Based on Q1: if _rowid does not work as a delete predicate column, all references to _rowid in this epic must be treated as post-Q1 optional work, not Phase A.

Phase A — DDL: PRIMARY KEY declaration (moved from old Phase B)

Depends on: Phase 0 (need to confirm PK predicate shape works).

  • Accept PRIMARY KEY (col, ...) NOT ENFORCED in LanceDynamicTableFactory via Flink's ResolvedSchema#getPrimaryKey.
  • Store PK column names on LanceOptions (or a new LanceTableSchema wrapper). Include ordering (composite PK order matters for the predicate builder).
  • DDL validation:
    • PK columns must exist in the schema.
    • Nullable PK columns must be rejected with a clear error message.
    • Types not supported by the predicate builder (see Phase B) must be rejected at DDL time, not at first delete.
  • Unit tests around DDL parsing (single-col PK, composite PK, missing col, nullable col, unsupported-type col).

Phase B — DELETE by PRIMARY KEY (the real replacement for #19)

Depends on: Phase A and Phase 0.Q2/Q3.

  • LanceSink#flushDeletes requires a declared PK (from LanceOptions).
    • If no PK is declared → reject at operator open time with a clear error: "DELETE requires PRIMARY KEY NOT ENFORCED to be declared in DDL".
  • Build the predicate from PK columns only. The exact shape follows Phase 0.Q3:
    • Preferred: pk IN (v1, v2, ...) for single-col PK, (pk1, pk2) IN ((...), ...) for composite PK.
    • Fallback (if composite tuple IN is unsupported): (pk1 = v1a AND pk2 = v2a) OR (pk1 = v1b AND pk2 = v2b) OR ....
  • Route through LanceOpener.open(path, allocator, options) so DELETE composes with read.version / read.as-of-timestamp from feat(read): support time travel via read.version and read.as-of-timestamp (#5) #56.
  • Fix predicate builder bugs uncovered in feat: Support DELETE operation on Lance datasets #19:
    • NULL PK values → reject at DDL time (nullable PK is banned per Phase A); never reach this builder.
    • Unsupported types → reject at DDL time; never reach this builder.
    • String escaping: double single-quote, plus reject values containing NUL / newline / control chars until we have a filter-parser round-trip test that says otherwise.
    • Value formatting per Arrow type: TIMESTAMP → timestamp 'YYYY-MM-DDTHH:MM:SS[.SSS]', DATE → date 'YYYY-MM-DD', DECIMAL → decimal(p,s) 'v' (see Lance filter syntax docs at http://www.lance.org/guide/read_and_write).
  • Handle full Flink CDC changelog, not just DELETE:
    • RowKind.INSERT → append (existing behaviour).
    • RowKind.UPDATE_BEFORE → drop (Lance is not row-versioned by upstream ROWID; the corresponding +U will land as delete-by-PK + insert).
    • RowKind.UPDATE_AFTER → delete-by-PK (using the PK values from this row itself) + append. This gives correct last-write-wins per PK, at the cost of two Lance ops per update. Document this trade-off.
    • RowKind.DELETE → delete-by-PK.
    • getChangelogMode must declare all four kinds when a PK is present; INSERT-only when no PK.

Phase C — Idempotency & checkpoint semantics

Depends on: Phase B.

  • Flush order at checkpoint: delete-batch first, insert-batch second (already correct in feat: Support DELETE operation on Lance datasets #19's structure, preserve it).
  • Analyse and document idempotency: replaying the same pk IN (...) predicate is naturally idempotent as long as no other writer has inserted a fresh row with the same PK in between. Document this assumption; if two writers race, that's an application-level bug not a connector bug.
  • When Java bindings expose Transaction.builder().operation(Operation.Delete).affected_rows(RowAddrTreeMap): wrap the DELETE in an explicit transaction with retry on TransactionRebase conflicts. Until then, log the resolved predicate + pre/post row counts at INFO for post-hoc debugging.

Phase D (optional, deferred) — Stable row IDs as a compaction optimization

Not on the DELETE critical path. Only pursue if benchmarking shows compaction+index-rebuild latency is a real user problem.

  • Add write.enable-stable-row-ids: Boolean (default false) to LanceOptions, plumb to WriteParams.enableStableRowIds(...) at dataset creation.
  • Document clearly in the option description that stable row IDs help compaction, not DELETE identity, and that they are stable across compaction but not across updates.
  • Add read.include-row-id: Boolean to LanceSource for the "Lance → Flink → Lance" replication use case (out-of-scope for typical CDC).

Acceptance criteria (quantified)

The epic is considered done when all of the following are demonstrated in CI:

# Scenario Success criterion
AC1 Small: 3-row seed, 3 DELETEs by PK Only those 3 rows are gone; no historical duplicates touched; no residual filter left for Flink; total wall-time < 5s
AC2 Medium: 100k-row seed, 1k DELETEs Correctness verified by SELECT COUNT(*) before/after; total wall-time < 30s
AC3 Composite PK: (user_id, event_ts), 1k DELETEs Predicate shape matches Phase 0.Q3 finding; correctness verified
AC4 Full CDC: +I, -U, +U, -D on 100 PKs Final dataset contains exactly the last +U state per PK; -D'd PKs are gone
AC5 Concurrent: 2 sink subtasks, disjoint PK ranges Both commit; no conflict; final row count = sum of two ranges' final states
AC6 Failure recovery: kill TM mid-delete, restore from checkpoint No double-delete on replay; row count matches the pre-crash committed state
AC7 Time-travel composition: DELETE against read.version=N-1 Rejected with a clear error (deleting against a historical version is nonsensical), OR delete happens at latest with a warning — decide and document either way
AC8 DDL: nullable PK column DDL rejected with clear error at plan time, not at first delete
AC9 No PK declared: attempt DELETE via CDC source Operator-open-time error, not a silent success

CI must include AC1–AC5 and AC8–AC9 as fast tests (< 1 min total). AC6 and AC7 can be nightly.

Related

References

Change log (this issue)

  • v2 (2026-08-14): Reworked after adversarial review. Key changes: added Phase 0 spike as a blocking prerequisite; demoted stable row IDs from Phase A to optional Phase D; corrected the "stable across compaction and updates" misreading (it's compaction only); made PRIMARY KEY the mainline identity contract; expanded scope to the full +I/-U/+U/-D CDC changelog; added quantified acceptance criteria AC1–AC9. Old v1 kept in issue history.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    epicMulti-phase tracking issue that coordinates several PRsfeat/deleteDELETE / changelog-mode / CDC handling in the sink

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions