Skip to content

fix(merge-insert): protect concurrent inserts on ordinary join keys - #9238

Open
Xuanwo wants to merge 2 commits into
mainfrom
xuanwo/merge-insert-key-conflicts
Open

Xuanwo wants to merge 2 commits into
mainfrom
xuanwo/merge-insert-key-conflicts

Conversation

@Xuanwo

@Xuanwo Xuanwo commented Sep 15, 2026

Copy link
Copy Markdown
Member

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_ids rather than adding a transaction field.

Cross-binding validation

Using LanceDB ffe94a65a1881f0dbcf4ff84ab26d05883009279 with its pinned Lance v12.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.

@github-actions github-actions Bot added the bug Something isn't working label Sep 15, 2026
@Xuanwo
Xuanwo marked this pull request as ready for review September 15, 2026 08:08

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

@lance-gatekeeper lance-gatekeeper Bot added the K-decision Latest Gatekeeper review requires a maintainer decision. label Sep 15, 2026

This branch has not been deployed

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

Labels

bug Something isn't working K-decision Latest Gatekeeper review requires a maintainer decision.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant