Conversation
There was a problem hiding this comment.
🟡 Gate recommendation: maintainer decision required.
The duplicate-insert failure is real: because ordinary ON columns currently omit inserted-key metadata, two writers can both add the same absent key and commit. However, this PR changes Lance’s transaction/isolation contract, not just wiring. In #5633, maintainers deliberately limited conflict serialization to declared primary keys and treated duplicates on ordinary ON columns as valid snapshot-isolation behavior. The same generalization in #6018 was directed through a specification/community decision; its spec proposal #6052 closed without acceptance.
Please record which contract Lance wants: every explicit ON list implicitly enables serialized conflict/retry semantics (including conservative conflicts for different key sets and the requirement to upgrade all concurrent writers); protection remains tied to primary-key metadata; or callers opt in explicitly. Then the implementation, key encoding, and rollout can be judged against that chosen contract.
Concurrent merge inserts can both decide that a key is absent and commit duplicate rows when the ON columns are not declared as a primary key. Scalar-indexed and partial-schema execution can also omit the inserted-key conflict information. This change makes these paths record inserted ON keys and reuse commit conflict detection and full merge retries, so a stale insert plan is recomputed against the latest dataset.
Keys that the existing encoding cannot represent conservatively saturate the Bloom filter instead of silently escaping detection; this includes the Float64 keys inferred by the JS binding. Top-level NULL keys keep SQL nonmatching semantics. The wire format is unchanged, but ordinary ON columns acquire stronger conflict semantics. This is not a general uniqueness constraint, does not remove existing duplicates, and requires all concurrent merge writers to use the fixed code.
Design status
Related: lancedb/lancedb#3377, lancedb/lancedb#2463, #8952, and #6018. This Draft overlaps the existing proposals and adds conservative handling of unrepresentable keys plus deterministic coverage across indexed, partial-schema, and composite-key paths. Prior maintainer feedback requested a specification decision and community vote for stronger merge-key semantics; that decision remains a prerequisite for merging. The implementation reuses the existing
KeyExistenceFilter.field_idsrather than adding a transaction field.Cross-binding validation
Using LanceDB
ffe94a65a1881f0dbcf4ff84ab26d05883009279with its pinned Lancev12.0.0-beta.18(1047c4bdbf36331cff2d9e21ab525c45ed741f93), four JS regressions (stale handles / concurrent writers, with / without an index) fail before this patch: stale handles insert instead of update, and concurrent cases produce five rows instead of four. Applying the same patch to that pinned Lance revision and rebuilding the native binding makes all four pass; all 18 selected JS merge-insert tests pass. The LanceDB dependency pin is unchanged, so release integration is still a follow-up.