Skip to content

fix(index): reject incompatible vector segments at segment commit - #9351

Closed
u70b3 wants to merge 3 commits into
lance-format:release/v11.0from
u70b3:fix/commit-segments-vector-compat-v11
Closed

u70b3 wants to merge 3 commits into
lance-format:release/v11.0from
u70b3:fix/commit-segments-vector-compat-v11

Conversation

@u70b3

@u70b3 u70b3 commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Problem

Dataset::commit_existing_index_segments validated only index_details.type_url across the segment set, so vector segments built with different distance metrics (or dimensions, sub-index types, or quantizer kinds) could be committed as one logical index. Scan planning derives the metric from the first segment and applies it to the whole logical index, so the remaining segments would be searched and ranked under the wrong metric — silently returning incorrect nearest-neighbor results.

Reproduced via the distributed build loop (the path lance-c's lance_dataset_commit_index_segments binds): build fragment 0 as IVF_FLAT/L2 and fragment 1 as IVF_FLAT/Cosine, then commit both metadata blobs — the commit succeeds at ab6b5bbe.

Surfaced during review of lance-format/lance-c#84.

Fix

After replacement selection, validate the coexisting segment set — incoming segments plus retained existing segments — with the same validate_vector_query_compatibility check the query/optimize paths already require (metric, dimension, sub-index type, quantizer kind; independently trained IVF centroids and PQ codebooks may still differ). Incompatible sets are rejected with InvalidInput before any manifest change, so the dataset version and existing index are untouched.

Because incoming segments are not in the manifest yet, the validation opens each segment through a new Dataset::open_vector_index_from_metadata helper — DatasetIndexInternalExt::open_vector_index minus the manifest load_index lookup, extracted mechanically with no behavior change for committed segments.

A complete replacement may still change the metric: replaced segments are no longer part of the coexisting set, so they impose no constraint.

Tests

Three new tests in rust/lance/src/index/vector/ivf/v2.rs:

  • two incoming segments with L2 + Cosine metrics over disjoint fragments are rejected (error names the metric; version unchanged, no index created);
  • an incoming Cosine segment conflicting with a retained L2 segment is rejected (retained index untouched);
  • a full-coverage replacement switching L2 → Cosine is accepted.

test_commit_existing_index_segments_commits_multiple_segments previously committed metadata-only fake segments (payload b"seg0"); since compatibility validation now opens coexisting segments, it builds real IVF segments via execute_uncommitted instead — same assertions otherwise.

  • cargo test -p lance --lib: 3069 passed, 0 failed
  • cargo clippy -p lance --lib --tests: clean

Targeting release/v11.0 so lance-c can pin a v11-line revision containing the fix (its current pin ab6b5bbe = v11.0.0). Happy to forward-port to main if you'd like it there first.

@github-actions github-actions Bot added the bug Something isn't working label Sep 17, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 17, 2026
jja725 pushed a commit to lance-format/lance-c that referenced this pull request Sep 17, 2026
Second PR in the #55 sequence (PR 1 / E1+E2 was #57): the C-side commit
half of the distributed build loop.

## Summary

Adds `lance_dataset_commit_index_segments`: commits previously built
uncommitted segments (protobuf-encoded `pb::IndexMetadata`, as produced
by `lance_index_segment_builder_execute_uncommitted`) as one logical
index in a single dataset version — the same loop the Java SDK exposes
as `Dataset.commitExistingIndexSegments`.

Thin FFI over `DatasetIndexExt::commit_existing_index_segments` (lance
v11.0.0 `ab6b5bbe`, `rust/lance/src/index.rs:1995`); the prost decode +
range validation is shared with `lance_index_segment_metadata_parse` via
a common `decode_segment_metadata()` helper. Segment-set validation
(empty set, duplicate UUIDs, overlapping fragment coverage, keyed-field
mismatch) is left to the core; the FFI boundary validates NULL/empty
inputs with descriptive errors.

## Deviation from the issue §6 sketch: no `replace` flag

The sketch predates the v11 bump (#77). At v11 there is no `replace`
parameter — replacement is automatic and coverage-driven, and the header
documents the exact semantics (verified against `index.rs:2077-2130` and
locked by tests):

- Same-type, same name: existing segments whose fragment coverage is
fully covered by the incoming set are replaced; disjoint-coverage
segments are retained as deltas; partial overlap (orphaning fragments)
is rejected.
- Type change: a full-coverage commit whose index type differs from the
existing same-name index replaces that index entirely; a
partial-coverage type change is rejected.
- Every segment must declare `column` as its keyed field (i.e., have
been built for `column`); mismatched or unknown columns fail.

## Bindings

- C: declaration + docs in `include/lance/lance.h`.
- C++: `Dataset::commit_index_segments(index_name, column,
segment_metadata)` in `include/lance/lance.hpp`.
- Rust FFI: `src/index_segment.rs`, alongside the PR-1 segment APIs.

## Tests

9 new Rust tests in `tests/c_api_test.rs`: multi-segment happy path (2
IvfFlat segments over disjoint fragments → one version bump → k-NN
through the committed multi-segment index), duplicate UUIDs, overlapping
coverage, malformed/truncated metadata, 9 NULL/empty boundary cases,
unknown column, wrong column (keyed-field mismatch), full-coverage
replacement, and type-change (full-coverage replace + partial-coverage
rejection). Plus C and C++ compile-and-run tests in `tests/cpp/`.

- `cargo test`: 319 passed, 0 failed (all suites)
- `cargo clippy --all-targets -- -D warnings` / `cargo fmt --check`:
clean
- `cargo test --test compile_and_run_test -- --ignored`: 3 passed

Does not close #55 — PR 3 (E3 progress) and PR 4 (E6 listing) remain
tracked there.

## Open question carried from #55

Physical segment merge (`Dataset::merge_existing_index_segments`) is
still not exposed; per the tracker its exposure scope is to be decided
alongside this PR.


## Update: vector segment compatibility (review follow-up)

The pinned Lance validator compared only `index_details.type_url`, so
mixed-metric vector segments could be committed as one logical index and
silently ranked under the wrong metric. Lance core now enforces the
query path's `validate_vector_query_compatibility` (metric, dimension,
sub-index type, quantizer kind) across the coexisting segment set —
incoming plus retained existing segments, after replacement selection —
rejecting incompatible commits with `InvalidInput` before any manifest
change. Companion Lance PR: lance-format/lance#9351 (targets
`release/v11.0`); the pin moved from `ab6b5bbe` (v11.0.0) to `356acb0d`,
the head of that PR, and will move to the upstream merge commit once it
lands.

New tests: mixed-metric rejection in a single commit and against a
retained segment (version/manifest untouched, retained segment still
answers indexed k-NN identically), same-metric delta commit, and
full-coverage metric replacement. Docs in `lance.h`/`lance.hpp` describe
the compatibility rule.
@u70b3
u70b3 force-pushed the fix/commit-segments-vector-compat-v11 branch from 356acb0 to 9b22033 Compare September 17, 2026 15:42
@github-actions github-actions Bot added A-python Python bindings A-index Vector index, linalg, tokenizer A-java Java bindings + JNI A-deps Dependency updates A-encoding Encoding, IO, file reader/writer A-format On-disk format: protos and format spec docs A-namespace Namespace impls A-ci CI / build workflows format-change A change to the format spec, which requires a vote. Remove if minor (e.g. fixing typo). labels Sep 17, 2026
@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 17, 2026
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Important

Format specification vote

This PR modifies the Lance format specification, so it requires 3 binding +1 votes from PMC members (excluding the proposer), at least one of them on the latest commit, and a minimum 72-hour voting period, weekends excluded, before it can merge. Vote by approving this PR (+1) or requesting changes (−1, a veto). See the voting process.

Approvals carry over across pushes, so a rebase or a typo fix does not send everyone back to re-vote. Whoever approves the latest commit is vouching that nothing substantive has changed since the earlier approvals; if something has, ask for fresh votes.

Status: ❌ Blocked — 0 of 3 required approvals

Approvals none (0/3)
Latest commit approved by none — one PMC member must approve the latest commit
Vetoes none
Voting period ends Tue 2026-09-22 15:42 UTC (08:42 PDT)

Updated automatically by the format-spec vote gate, which re-checks every 15 minutes — just voted? Re-check now (press Run workflow; leave the input blank to re-check every open format PR). A PMC member may apply the format-waived label to waive the vote for a trivial edit (typo, wording, formatting).

lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added K-changes Latest Gatekeeper recommendation requests changes. and removed K-changes Latest Gatekeeper recommendation requests changes. labels Sep 17, 2026
@u70b3
u70b3 force-pushed the fix/commit-segments-vector-compat-v11 branch from 9b22033 to 356acb0 Compare September 17, 2026 16:08
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 17, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

commit_existing_index_segments compared only index_details.type_url
across segments, so vector segments built with different distance
metrics (or dimensions, sub-index types, quantizer kinds) could be
committed as one logical index. Scan planning derives the metric from
the first segment and applies it to the whole logical index, silently
ranking the rest under the wrong metric.

Validate the coexisting segment set — incoming segments plus retained
existing segments, after replacement selection — with the same
validate_vector_query_compatibility check the query path requires,
opening each segment via a new open_vector_index_from_metadata helper
that does not require the segment to be committed yet. A complete
replacement may still change the metric because replaced segments no
longer constrain the set.
rustls 0.23.40 is flagged by RUSTSEC-2026-0285 (TLS 1.3 handshake
messages accepted across encryption level boundaries); 0.23.45 is the
fix release. Refresh all three lockfiles; the root lockfile also picks
up the required aws-lc-rs 1.18.1 / aws-lc-sys 0.45.0 bump.
@u70b3
u70b3 force-pushed the fix/commit-segments-vector-compat-v11 branch from 356acb0 to bd0bbf1 Compare September 17, 2026 17:26
@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 17, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 17, 2026
Backport of the compat-harness change from lance-format#8903 (1391e6c).

pylance 12.0.0 (released 2026-09-17) and the 13.0.0 betas depend on
lance-namespace>=0.11.1,<0.12, which conflicts with the harness's
lance-namespace>=0.8.0,<0.9 pin: venv creation for those versions fails
pip resolution, failing every [12.0.0]/[13.0.0b4] compat case and every
index-maintenance sequence shard (its newer ref resolved to 12.0.0 as
soon as it hit PyPI), which alone burns the 60-minute Compatibility
Tests budget.

Not caused by the index fix in this PR; backported to unblock compat
CI on release/v11.0 (the fix already lives on main).
@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 18, 2026

@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: approve.

The index compatibility fix and synchronized rustls update remain sound. The added compatibility-harness tier is the same change already merged in #8903: it keeps pylance 12.0.0b5 on lance-namespace 0.8 and selects the compatible 0.11 range for later releases.

@lance-gatekeeper lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. and removed K-approved Latest Gatekeeper recommendation permits acceptance. labels Sep 18, 2026
@u70b3

u70b3 commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #9432 (identical commits). A stale format-change label — applied while this PR briefly targeted main, where the branch-vs-main diff included the release branch's proto changes — keeps the format-spec-vote gate blocking merge, and the label can't be removed from a fork.

@u70b3 u70b3 closed this Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ci CI / build workflows A-deps Dependency updates A-encoding Encoding, IO, file reader/writer A-format On-disk format: protos and format spec docs A-index Vector index, linalg, tokenizer A-java Java bindings + JNI A-namespace Namespace impls A-python Python bindings bug Something isn't working format-change A change to the format spec, which requires a vote. Remove if minor (e.g. fixing typo). K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant