Add distributed index segment build APIs - #57
Conversation
4981a4e to
a19ce65
Compare
## Summary Upgrade the pinned Lance revision from `e0e977a6` (7.0.0-beta.7) to `e934cc2c` (9.1.0-beta.3), the core baseline used by the distributed index build track (#55). Split out of #57 so the engine upgrade lands (and can be reverted) independently of the feature work. ## What the upgrade forced - **Error-constructor migration (the bulk of the diff).** lance-core error variants gained an implicit `backtrace` field, so direct struct construction (`Error::InvalidInput { source, location }`) no longer compiles. All FFI validation errors migrate to the new `Error::invalid_input` / `invalid_input_source` / `index_not_found` constructors. Variants and user-facing messages are unchanged; location (and now backtrace) are captured implicitly by snafu. - **Three error-code assertions in c_api_test.rs.** Lance 9.1 classifies SQL parser failures and unknown expression columns as invalid user input (`InvalidArgument`) instead of `Internal`. The affected tests assert the new classification. - **datafusion dev-dependency 53 → 54** to match the pinned Lance. - **Auto-cleanup default preserved (gatekeeper finding).** Lance 9.1 flipped `WriteParams::auto_cleanup` from `Some(default)` to `None`. Because lance-c exposes neither cleanup configuration nor an explicit cleanup operation, that flip would silently remove the only C-visible reclamation path for datasets created through the C writer. The wrapper now sets `auto_cleanup` explicitly to the pre-9.1 default (every 20 versions, older than 14 days), so C-visible behavior is unchanged. Covered by `test_write_preserves_auto_cleanup_default`. A C cleanup API (`lance_dataset_cleanup_old_versions`, optional `LanceWriteParams` knobs) can be proposed as its own PR. No public API, ABI, or behavior changes beyond the three reclassified error codes above. ## Validation - `cargo check --all-targets` - `cargo clippy --all-targets -- -D warnings` - `cargo test` (257 tests) - `cargo test --test compile_and_run_test -- --ignored --test-threads=1` (real C and C++ dynamic-link runs) Tracks #55. The distributed index segment build PR (#57) is stacked on this change. ## Notes for reviewers - The three reclassified error paths (`delete` malformed SQL / unknown column, `add_columns_sql` unknown column: `LANCE_ERR_INTERNAL` → `LANCE_ERR_INVALID_ARGUMENT`) are behavior changes visible to C callers; the public docs in `lance.h` / `lance.hpp` are updated accordingly. A `breaking-change` label may be warranted — maintainer's call. - Related: #56 attempted a jump to 10.0.0 and was withdrawn. This PR stops at 9.1.0-beta.3, the baseline verified in #55 — which already contains the nullable FixedSizeList inner-null crash fix (lance-format/lance#7498, merged 2026-06-30) that motivated #56. A further move to 10.x can be evaluated as its own PR.
a19ce65 to
3d6c545
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The worker-side uncommitted-build design is a sound fit for the core primitives, but shared PQ models must preserve the metric's quantization contract. For DOT distance, train the PQ codebook on raw vectors while retaining the IVF model only for identity/provenance; residual training remains appropriate for L2 and cosine.
| dim, | ||
| metric.to_distance(), | ||
| ¶ms, | ||
| Some(&ivf), |
There was a problem hiding this comment.
DOT PQ is trained on residuals here even though Lance's DOT encoder/search path does not use residuals. Passing Some(&ivf) unconditionally makes build_pq_model_in_fragments subtract the nearest centroid; the resulting schema is marked DOT, then the segment builder accepts the codebook and quantizes raw DOT vectors with it, which can silently produce a mismatched index and degrade recall.
Please pass the IVF model only for L2/Cosine (matching core build_ivf_model_and_pq and PQBuildParams::use_residual) and pass None for DOT. The centroids can still be required for shared-model identity/provenance.
Reproducer
I ran cargo test --test gate_dot_pq -- --nocapture against this head with a focused integration test that builds 128 positive Float32 vectors, trains DOT IVF/PQ through these C entry points, and compares that codebook with build_pq_model_in_fragments(..., DistanceType::Dot, ..., None, None) on the same dataset. The regression assertion requires the C codebook to remain on the raw-vector scale: assert!(c_mean_abs >= core_mean_abs * 0.5). It fails with C mean_abs=23.066938 versus core mean_abs=101.07422; the C output is residual-centered.
There was a problem hiding this comment.
Fixed in 3f04af3: DOT PQ training now uses raw vectors, while L2 and cosine retain IVF residual training; the focused regression test verifies the C codebook against the canonical core DOT trainer.\n\n
There was a problem hiding this comment.
Fixed in 3f04af3: DOT PQ training now uses raw vectors, while L2 and cosine retain IVF residual training; the focused regression test verifies the C codebook against the canonical core DOT trainer.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The DOT PQ path now matches the pinned core metric contract: DOT trains on raw vectors, while L2 and cosine retain IVF residual training. The focused regression test covers the prior failure, and the worker-side uncommitted-build design remains sound.
| } | ||
| // Core's train=false means "create an empty index". Model presence | ||
| // itself controls whether IVF/PQ training is skipped. | ||
| block_on(core_builder.train(true).execute_uncommitted())? |
There was a problem hiding this comment.
Could we ensure precomputed DOT PQ builds use a DOT quantizer here? Whole-dataset builds take core’s normal path, which creates the supplied codebook with DistanceType::L2. That leaves the stored codes L2-encoded while metadata/search use DOT, silently hurting recall.
There was a problem hiding this comment.
Fixed in 3d82c70. lance-c now fails fast for supplied DOT PQ codebooks when the fragment selection covers the full dataset, preventing the L2-encoded mismatch. A TODO and C API note document that complete support requires an upstream Lance fix.
|
Change LGTM, could you rebase to resolve conflicts |
Implements PR 1 from lance-format#55: the foundational E1 + E2 primitives for distributed index builds. - adds snapshot-owned, single-use scalar and vector segment builders - builds over explicit fragment subsets without committing a dataset manifest - supports caller-assigned segment UUIDs where Lance core permits them - trains reusable shared IVF centroids and residual PQ codebooks - injects shared models through the Arrow C Data Interface - serializes and parses protobuf IndexMetadata with C getters - adds move-only C++ RAII wrappers for builders, models, metadata, and returned bytes Stacked on the lance 9.1.0-beta.3 bump.
3d82c70 to
873e0c6
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The new supplied-DOT safeguard is inverted: full-dataset V3 builds already preserve the required L2 PQ-assignment contract, while strict fragment subsets take the distributed path that rewraps the codebook as DOT. Preserve full-dataset use and fail closed only for strict-subset supplied DOT PQ until the upstream distributed builder uses L2.
| LanceVectorIndexType::IvfPq | LanceVectorIndexType::IvfHnswPq | ||
| ) && params.metric == LanceMetricType::Dot | ||
| && codebook.is_some() | ||
| && selected_fragment_count == dataset_fragment_count |
There was a problem hiding this comment.
This guard is inverted: it rejects the safe full-dataset path and leaves the mismatched distributed path enabled. In pinned Lance e934cc2, implicit or explicit full coverage normalizes to build_vector_index, whose quantizer builder deliberately calls Q::build(..., DistanceType::L2, ...). A strict subset with supplied IVF instead enters build_distributed_vector_index; make_global_pq rewraps the same L2-trained codebook with DistanceType::Dot, and ProductQuantizer then uses DOT when assigning codes. That can silently change stored codes and reduce recall.
Please keep full-dataset supplied DOT PQ enabled and reject effective strict subsets in AUTO/PRECOMPUTED for both IVF-PQ variants until upstream make_global_pq reconstructs with L2. The header contract and regression expectations need the same inversion.
Reproducer
With this head checked out, I ran a bounded assignment reproducer matching Lance’s L2/DOT argmin:
node -e 'const v=[1.1,0], c=[[1,0],[10,0]]; const l2=c.map(x=>(v[0]-x[0])**2+(v[1]-x[1])**2); const dot=c.map(x=>1-v[0]*x[0]-v[1]*x[1]); const pick=a=>a.indexOf(Math.min(...a)); console.log({l2,dot,l2_assignment:pick(l2),dot_assignment:pick(dot)})'It reports l2_assignment: 0 and dot_assignment: 1. The same vector/codebook therefore receives a different code solely because distributed reconstruction switches the quantizer metric. The focused repository test also passes while asserting that the affected one-of-two-fragments case executes, so it does not detect the mismatch.
There was a problem hiding this comment.
Fixed in 8c15d19 — the guard is inverted as suggested: full-coverage selections (implicit NULL or an explicit list of every fragment) now pass, and only an effective strict subset with a supplied DOT PQ model set is rejected, in AUTO and PRECOMPUTED for both IVF-PQ variants. The subset check compares fragment-id sets (mirroring effective_vector_fragments) rather than counts, and the header contract and regression test are inverted to match; the test now executes an implicit full-coverage DOT PQ build end to end to cover the safe path.
I verified each link of the analysis against pinned e934cc2 before making the change: create.rs:845 normalization, builder.rs:517 hard-coded L2 in load_or_build_quantizer, vector.rs:642 make_global_pq rewrapping with DOT, and pq/storage.rs:350 persisting the quantizer's distance type into the index metadata. The analysis holds — thank you for the careful catch.
There was a problem hiding this comment.
Fixed in 8c15d19: the guard now preserves implicit and explicit full-dataset coverage and rejects supplied DOT PQ only for effective strict subsets in AUTO and PRECOMPUTED across both IVF-PQ variants; the header contract and regression coverage match that verified behavior.
|
@jja725 heads-up on 8c15d19: after re-reading the pinned Lance source, I believe lance-gatekeeper's latest analysis is correct and my previous fix (873e0c6) had the guard inverted. What I verified in e934cc2:
So 8c15d19 flips the fail-fast: full-coverage DOT PQ builds are enabled again, and effective strict subsets with a supplied DOT PQ model set are rejected (AUTO/PRECOMPUTED, IVF-PQ and IVF-HNSW-PQ) until upstream This reverses the direction of your earlier suggestion, so I'd rather not proceed on the bot's analysis alone — could you take another look and make the final call? If we agree on the direction, I'm happy to file the upstream Lance issue for |
There was a problem hiding this comment.
✅ Gate recommendation: approve.
Both prior Gate findings are addressed. DOT PQ training remains on raw vectors, while supplied DOT PQ segment builds preserve the safe full-dataset path and reject only effective strict subsets across AUTO/PRECOMPUTED and both IVF-PQ variants. The C contract and focused regression coverage match the pinned Lance routing and quantizer semantics.
jja725
left a comment
There was a problem hiding this comment.
merge for now, we can fix the potential panic later on
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.
Summary
Implements PR 1 from #55: the foundational E1 + E2 primitives for distributed index builds.
IndexMetadatawith C gettersDesign notes
9.1.0-beta.3(e934cc2c), the core baseline used by RFC: C-level primitives for distributed index builds (fragment-scoped uncommitted builds, segment metadata, progress, cancellation) #55; the pin upgrade and its error-constructor compatibility migration landed separately in chore(deps): bump lance to 9.1.0-beta.3 #58.modefield (AUTO,LOCAL_TRAIN,PRECOMPUTED) instead of a boolean so zero-initialized C options have unambiguous defaults.mallocand released withlance_free_bytes.Impact
C and C++ workers can now train a shared model once, build physical index segments over disjoint fragment assignments, and ship protobuf metadata to a coordinator without changing the dataset manifest. Commit-existing-segments remains the next PR in the tracker.
Validation
cargo check --all-targets --lockedcargo clippy --all-targets --locked -- -D warningscargo test --locked(268 C API tests, 3 model tests, 1 helper test)-Werrorcargo test --locked --test compile_and_run_test -- --ignored --test-threads=1Tracks #55.