Add commit-existing-index-segments C API - #84
Conversation
852926f to
021a626
Compare
|
|
||
| let ds = unsafe { &*dataset }; | ||
| ds.with_mut(|dataset| { | ||
| block_on(dataset.commit_existing_index_segments(index_name, column, segments)) |
There was a problem hiding this comment.
Reject vector segments with incompatible metrics before committing them as one logical index. I reproduced this by building fragment 0 as IVF_FLAT/L2 and fragment 1 as IVF_FLAT/Cosine, then passing both metadata blobs here; this call returns success. The pinned Lance validator compares only index_details.type_url, which is VectorIndexDetails for both segments, while scan planning derives one metric from the first segment and uses it for the whole logical index. The second segment can therefore be searched/ranked with the wrong metric, silently producing incorrect nearest-neighbor results. Please fix the core compatibility validation (metric and any other logical-index-wide vector settings), bump the Lance revision, and add a mixed-metric C API regression test. (severity: 9/10)
There was a problem hiding this comment.
Fixed in 5c91604. The validation now lives in Lance core: commit_existing_index_segments validates 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 — metric, dimension, sub-index type, and 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. A complete replacement may still change the metric, since replaced segments no longer constrain the set.
The core fix is on top of v11.0.0 (release/v11.0 line) at 356acb0d, which this PR now pins; I'll link the companion Lance PR here once it is open. On the C side this surfaces as LANCE_ERR_INVALID_ARGUMENT, and the header/C++ docs describe the rule.
New regression coverage here: mixed-metric commit rejected both in a single commit (L2 + Cosine over disjoint fragments) and against a retained existing segment — asserting the version, manifest, and persisted index are untouched and the retained segment still answers indexed and unindexed k-NN identically — plus a same-metric delta commit and a full-coverage metric replacement that both succeed. The core PR adds matching Rust tests.
There was a problem hiding this comment.
Companion Lance PR: lance-format/lance#9351 (targets release/v11.0; the pin here will move to the upstream commit once it merges).
Addresses review on lance-format#84: the pinned Lance validator compared only index_details.type_url, so IVF_FLAT/L2 and IVF_FLAT/Cosine segments could be committed as one logical index and silently ranked under the wrong metric. Lance core now validates the coexisting segment set (incoming plus retained existing segments, after replacement selection) with the same metric/dimension/sub-index-type/quantizer-kind compatibility check the query path requires (lance-format/lance fix on top of v11.0.0, pinned here as 356acb0d). Incompatible commits fail with LANCE_ERR_INVALID_ARGUMENT before any manifest change. - docs: lance.h / lance.hpp document the vector compatibility rule and that a complete replacement may change the metric - tests: mixed-metric commit rejected (fresh and vs retained segment, version + manifest untouched), same-metric delta commit, and full-coverage metric replacement accepted
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The mixed-metric blocker from the earlier discussion is fixed. The pinned Lance core change validates incoming plus retained vector segments after replacement selection and before the manifest transaction, while still allowing a complete replacement to change its query contract. No blocking issues remain.
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-encodedpb::IndexMetadata, as produced bylance_index_segment_builder_execute_uncommitted) as one logical index in a single dataset version — the same loop the Java SDK exposes asDataset.commitExistingIndexSegments.Thin FFI over
DatasetIndexExt::commit_existing_index_segments(lance v11.0.0ab6b5bbe,rust/lance/src/index.rs:1995); the prost decode + range validation is shared withlance_index_segment_metadata_parsevia a commondecode_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
replaceflagThe sketch predates the v11 bump (#77). At v11 there is no
replaceparameter — replacement is automatic and coverage-driven, and the header documents the exact semantics (verified againstindex.rs:2077-2130and locked by tests):columnas its keyed field (i.e., have been built forcolumn); mismatched or unknown columns fail.Bindings
include/lance/lance.h.Dataset::commit_index_segments(index_name, column, segment_metadata)ininclude/lance/lance.hpp.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 intests/cpp/.cargo test: 319 passed, 0 failed (all suites)cargo clippy --all-targets -- -D warnings/cargo fmt --check: cleancargo test --test compile_and_run_test -- --ignored: 3 passedDoes 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'svalidate_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 withInvalidInputbefore any manifest change. Companion Lance PR: lance-format/lance#9351 (targetsrelease/v11.0); the pin moved fromab6b5bbe(v11.0.0) to356acb0d, 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.hppdescribe the compatibility rule.