Batch committed @unique validation probes per constraint group#363
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e762471657
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if wanted.contains(&key) { | ||
| holders | ||
| .entry(key) | ||
| .or_default() | ||
| .push(ids.value(row).to_string()); |
There was a problem hiding this comment.
When a branch merge has more than 8,192 keys for a composite @unique constraint and chunks reuse component values, the AND-of-IN-lists predicate can return a tuple belonging to a different chunk (the documented Cartesian superset). Checking it against the global wanted set appends that holder once for every such chunk, so holders can grow with chunks × matching committed rows and the later violation loop repeatedly traverses duplicates. Use a per-chunk wanted set here; the global set is unnecessary because every requested tuple is scanned in its own chunk.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — applied in e776e99. The superset cross-chunk match was efficiency-only (the violation consumer breaks at the first foreign holder, and single-column chunks are exact IN-lists), but the per-chunk wanted set is strictly better since every tuple is matched by its own chunk's scan. The global set is gone.
e762471 to
f20c6f8
Compare
Cross-version uniqueness validation probed committed state once per delta row, sequentially, and each probe re-opened the Lance dataset (validation snapshots deliberately carry no read caches). A merge or append load of N rows into a table with a non-key @unique column paid N dataset opens + N filtered scans against the object store, while overwrite (empty committed view) paid zero — the observed ~40x gap. unique_holders now takes the whole key set for one (table, group): the dataset is opened once and each <=8,192-key chunk is one filtered scan. Single-column groups push one typed IN-list (BTREE-served as one IsIn query on the pinned Lance). Composite groups push an AND of per-column IN-lists — an index-served superset — and exact tuple membership is decided in memory via the same composite_unique_key canonicalization the delta uses. Keys are dedup'd before probing. Validation semantics are unchanged: same violations, same message format, same deterministic ordering, same self-exclusion and null-key rules.
write_cost.rs gains unique_probe_io_is_flat_in_delta_rows: a fixed shallow history, one measured append load at delta sizes 4 vs 64 against a non-key @unique table, asserting data_open_count and data_scan_reads flat on the shared helpers::cost harness. Before the batching commit this curve was red (opens grew ~1:1 with rows). validators.rs extends cross_version_unique_rejected_on_append_load with a multi-row second load (one colliding + one clean row): the batched probe still surfaces the violation and the rejected load commits neither row. Dev docs updated (invariants truth matrix, testing map) to describe the batched probe shape.
A composite group's AND-of-IN-lists chunk filter is a superset that can match a tuple belonging to a different chunk; collecting against a global wanted set appended that holder once per matching chunk (duplicate holder ids, growth bounded by chunks x matches on merges with >8,192 distinct composite keys). Violations were unaffected — the consumer breaks at the first foreign holder — but each tuple is guaranteed to be matched by its own chunk's scan, so a per-chunk wanted set collects every holder exactly once.
e776e99 to
cfe86b8
Compare
1 participant
What & why
Cross-version `@unique` validation probed committed state once per delta row — each probe re-opening the Lance dataset — so a merge/append load of N rows into a table with a non-key `@unique` column paid N dataset opens + N filtered scans on object stores (~40× slower than overwrite, which validates against an empty committed view). The probe is now batched: one dataset open per (table, unique group) and one typed filtered scan per ≤8,192-key chunk, with validation semantics fully unchanged.
Backing issue / RFC
Maintainer change (internal process applies); performance fix with no user-facing surface change.
Checklist
Notes for reviewers
Single-column groups push one typed IN-list (BTREE-served as one IsIn query on the pinned Lance); composite groups push an AND of per-column IN-lists — an index-served superset — with exact tuple membership decided in memory via the same `composite_unique_key` canonicalization the delta uses, so a partially-indexed composite group still costs one scan per chunk. Violations, message text, deterministic ordering, self-exclusion, and null-key handling are byte-identical. Full workspace suite left to CI at the author's request; the focused suites to eyeball are `validators`, `consistency`, `write_cost`, and `merge_cost`.
Note
Medium Risk
Changes cross-version uniqueness on merge, mutation, and load hot paths; semantics are intended to stay identical but composite superset filtering and up-to-8,192-key
INlists are subtle and only partially exercised at max chunk size.Overview
Cross-version
@uniquechecks no longer open the Lance dataset and run a filtered scan once per delta row.evaluate_uniquepass 3 now dedupes keys and callsunique_holdersonce per group; that helper opens each table once and scans in chunks of up to 8,192 keys using typed per-columnINfilters (composite groups still match exact tuples in memory viacomposite_unique_key).Tests add a multi-row append case that asserts load atomicity when one row collides, plus
unique_probe_io_is_flat_in_delta_rowsso data-table opens/scans stay flat as delta size grows. Dev docs describe the batched probe invariant and the new cost gate.Reviewed by Cursor Bugbot for commit cfe86b8. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR batches committed-state uniqueness checks without changing validation semantics. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Comments Outside Diff (1)
crates/omnigraph/src/validate.rs, line 411 (link)Maximum Filter Size Is Unverified
A mutation or load can put all 8,192 keys into one
in_list, but the new test only reaches 64 keys. If Lance's planner or scalar index rejects or cannot serve an expression of this size, a valid maximum-size write fails during uniqueness validation; exercise the 8,192-key boundary or lower the chunk size to a verified limit.Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (4): Last reviewed commit: "Scope unique-probe holder collection to ..." | Re-trigger Greptile
Context used: