feat: add distributed REFRESH INDEX for incremental index maintenance - #784
feat: add distributed REFRESH INDEX for incremental index maintenance#784ivscheianu wants to merge 8 commits into
Conversation
CREATE INDEX rebuilds every fragment, so keeping an index current on an append-heavy table means re-indexing the whole table. REFRESH INDEX builds only the fragments an index does not cover and adds them to it, distributed across executors the same way CREATE INDEX is. Also pin the dataset version for distributed index builds. Tasks open the dataset themselves and pinLoadedBranch only pins branch refs, so the driver planned over one version while each task resolved the latest. A concurrent OPTIMIZE could retire a planned fragment, leaving a segment whose coverage intersects to nothing at commit time while CREATE INDEX still reported it as indexed. A commit-time check now fails loudly instead. SHOW INDEXES gains indexed_percent, num_segments and size_bytes so index coverage is visible without computing it by hand.
REFRESH INDEX could commit an FTS index that no full-text query could read. Segments carry their build configuration, and an inverted index is read with one configuration for all of them, so a refresh that did not repeat the original options built segments Lance accepted at commit and then refused at read: "inconsistent inverted index details across segments". SHOW INDEXES reported full coverage throughout. The refresh now compares the details of the segments it built against the ones they would join and fails before committing, for index types whose read path requires them to agree. The comparison is on what was built rather than what was requested, and the segments are still uncommitted, so a rejection leaves the index untouched. Lance exposes no Java contract for reading a built index's parameters back -- only optimizeIndices, which derives them internally and runs on a single node -- so options still come from the WITH clause, and the docs now say which methods that matters for. Refreshing an index named <column>_idx failed on every executor task. The segment build let Lance derive its own name, which is exactly that, so the name-collision pre-check fired against the index being refreshed. That is the name Lance assigns an unnamed index, and the name used throughout the docs. Naming the segment after the index it will join, with replace set, skips a check that means nothing for an uncommitted build; the driver's single commitExistingIndexSegments transaction still decides what to keep. Re-running CREATE INDEX with such a name failed the same way, and the explicit name also moves the genuine "already exists with different fields" check ahead of the distributed build instead of after it. Uppercase WITH-clause options were silently ignored. ANTLR reports identifier text as written and every consumer matches lower-case literals, so WITH (NUM_SEGMENTS = 2) built with default parallelism and WITH (TRAIN = false) refreshed instead of being rejected. Option names are now normalized at the parser boundary, which fixes CREATE INDEX, OPTIMIZE and VACUUM the same way. Fragment batches are contiguous runs again. Lance can only compact fragments covered by an identical set of index segments, so the row-balanced assignment added in lance-format#758 left every adjacent pair under a different segment and made OPTIMIZE a no-op for the whole table. Balancing by row count and covering contiguous runs are compatible: boundaries are placed where an even split would fall, moved to the nearest fragment, which stays deterministic and still yields exactly num_segments batches. The commit-time liveness check is replaced by coverage accounting. A fragment leaves the manifest either because its rows moved or because they were all deleted; only the first leaves data unindexed, and in both cases the segments for what remains are correct. Discarding a finished distributed build was the wrong response, and it made a routine concurrent DELETE fatal. The commit now restricts declared coverage to the live fragments, warns about the rest, reports the count it actually achieved, and fails only when nothing would be covered. One rule covers both build paths, so range-mode BTree is no longer judged against a fragment list its coverage does not come from. Range-mode BTree also goes back to unpinned read options. It reads the table back through the catalog, which resolves its own version, so pinning made the segment record a dataset version older than its own contents. A DROP INDEX during a refresh no longer resurrects the index. The commit re-resolves the index on a fresh handle, because committing segments under a name Lance no longer knows creates that index rather than extending it. Fragment enumeration goes through getFragmentStatistics(), which returns primitive arrays, instead of getFragments(), which materializes a Java object per fragment and per data file on every driver-side pass. Docs: scope the compaction guidance to the methods that actually lose coverage, record that a partially covered zonemap index can return incomplete results rather than merely scanning uncovered fragments, describe how accumulated segments limit OPTIMIZE, and drop the claim that Lance does not expose a built index's parameters.
RefreshIndexExec re-resolves its target index on the commit handle, which removes the wide window -- the whole distributed build -- but the check and the commit are still two operations. Lance's rebase treats a concurrent CreateIndex as conflicting only when the other transaction also carries new_indices, and a drop carries only removed_indices, so a DROP INDEX landing between the check and the commit is rebased over rather than rejected. The index then reappears holding only the segments the refresh built. No connector-side fix exists: commitExistingIndexSegments takes no expected predecessor, CommitBuilder has no conflict policy, and a post-commit compensating drop would carry the same race while risking the deletion of an index a concurrent CREATE INDEX legitimately made. The guarantee has to come from the transaction authority, so add a disabled regression against the core primitive -- matching how this repo records other Lance-core gaps -- and document the limitation for operators. Enable the test once lance-format/lance#6806 is released and pinned.
|
First two are fixed. The concurrent-drop one is real, but I don't think the connector can close it: |
Spark SQL maps only the eight scalar index methods, so a vector index exists only because something outside Spark created it. Refreshing one was rejected with "Rebuild the index with ALTER TABLE ... CREATE INDEX instead", which is a dead end: CREATE INDEX resolves methods through the same mapping and cannot build a vector index either. Say so, and point at the SDK the index came from. SHOW INDEXES lists a vector index like any other, filtering only the indexes Lance maintains itself, and the new coverage columns make it more visible than before. So its guidance to refresh whatever reports under 100 percent now scopes itself to the methods Spark SQL can build.
…mating it Batching placed each boundary where an even split of the row count would fall, moved to the nearest fragment. That is a heuristic, and it loses the row-balance contract when indivisible leading fragments have already overshot their shares: on [95, 93, 89, 8, 1, 4, 74, 88, 38] across six segments it leaves a batch of 162 rows where fragment 0 alone forces 95, so one task is 70 percent heavier than it needs to be. The least-loaded-first assignment it replaced reached 95. Contiguity and balance are not actually in tension -- the optimal contiguous partition also reaches 95 -- so find it exactly. Binary search the smallest row budget a contiguous packing can respect: for a fixed budget, extending each run as far as it will go uses the fewest runs, so the smallest feasible budget is the optimal maximum, with the widest single fragment as the floor. Packing there can use fewer runs than were asked for, which would cost parallelism, so the remainder are split at their balance points; a split only lowers the heaviest run, so optimality survives it. Tests pin the property rather than one of its consequences: the case above is a regression, and a second test compares the result against every contiguous partition of seven workload shapes at every segment count. Both fail on the heuristic, the second one also catching a case at three segments that gives 200 where 188 is optimal.
…before it Both commands computed their fragment count before calling commitExistingIndexSegments, then reported it. That is a prediction, not what landed, and it can overstate in two ways. The check reads the manifest its handle was opened at, while the commit lands on whatever version is current by then, so a fragment retired in between is still counted. And Lance prunes an incoming segment's coverage not only for a fragment that is gone but also for one whose indexed field was rewritten under the same id, which no comparison of fragment ids can see at any moment. The count now comes from the metadata the commit returns, intersected with the fragments live once it has landed; the returned bitmaps are post-pruning and the committing handle has advanced to the manifest it wrote, so between them they answer both cases. Only the segments this build produced are counted, since existing segments disjoint from them survive the commit and their coverage is not this command's to report. Refusing a set that would establish no coverage stays where it was, before the commit, since that is the last point at which the build can be declined. Its reasoning was wrong, though, and is corrected: coverage is not lost by committing such a set. Lance accepts an empty fragment bitmap, and an existing segment is trivially disjoint from one, so it survives. The transaction would simply publish segments that index nothing.
0099f47 to
e9f6637
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The post-commit coverage revision now truthfully reports the fragments the transaction established. One transaction-safety blocker remains: REFRESH INDEX still resolves its target separately from the Lance segment commit, so stale work can undo a successful concurrent DROP INDEX.
The safe sequence remains to land and release lance#6806, or an equivalent same-name transaction conflict, update the pinned Lance dependency, and enable the regression before accepting this command.
| segments) | ||
| IndexUtils.requireCommittableCoverage(liveFragmentIds, segments, plan.resolvedName) | ||
| val committed = | ||
| dataset.commitExistingIndexSegments( |
There was a problem hiding this comment.
This commit is still made on a handle whose target-existence check occurred above and outside the transaction. With the pinned Lance 11.0.0-beta.10, a same-name drop carries only removed_indices, so this CreateIndex commit can rebase over it and recreate the successfully dropped index with partial coverage. This is the current-diff successor to the earlier finding.
The safe fix is to make same-name create/drop conflict at the Lance transaction boundary, then pin that release here and enable the regression.
Reproducer on this head
JAVA_TOOL_OPTIONS='-Djava.io.tmpdir=/home/agent/tmp/coverage_verify_e9f6637d_tmp' MAVEN_OPTS='-Dmaven.repo.local=/home/agent/tmp/coverage_verify_e9f6637d_m2' ./mvnw -pl lance-spark-3.5_2.12 -am -DsecondaryCacheDir=/home/agent/tmp/coverage_verify_e9f6637d_sbt -Dspark.local.dir=/home/agent/tmp/coverage_verify_e9f6637d_spark -Dsurefire.failIfNoSpecifiedTests=false -Djunit.jupiter.conditions.deactivate=org.junit.jupiter.engine.extension.DisabledCondition -Dtest=RefreshIndexTest#testStaleSegmentCommitDoesNotResurrectDroppedIndex test
The forced regression ran 1 test and failed at BaseRefreshIndexTest.java:654: A stale segment commit must not resurrect a concurrently dropped index ==> expected: <true> but was: <false>.
The comment justifying the pinned build described the wrong mechanism. Tasks receive the fragment ids to index from the driver, so the covered set is fixed either way; what an unpinned open changes is the version behind those fragments and the version stamped on the segment. It also pointed at a function that has since been renamed. The zonemap caveat read as a property of the Lance version rather than of partial coverage itself.
Range mode handed its executors unpinned read options, so each builder opened whatever version was latest after draining its shuffle partition. Core stamps a segment with the version of the handle that built it and validates a segment's coverage only when that stamp predates the commit, so a rewrite of the indexed column landing inside the build window left stale keys stamped at the current version and trusted. Predicates on the indexed column then missed rows while an unfiltered scan returned them. Pinning keeps the stamp no newer than the rows behind it, where core prunes the coverage rather than trusting it, so the failure costs coverage instead of correctness. The comments arguing the unpinned build was deliberate had it backwards. A pinned stamp can indeed predate the rows the scan read and lose its coverage to pruning, but that is the safe direction, and leaving the build unpinned does not avoid it: tasks that open before a concurrent rewrite still stamp an older version and are pruned anyway, so unpinned produced both failure modes at once.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The range-mode version pin closes the stale-key validation gap, but it does not change the remaining transaction-safety blocker: REFRESH INDEX still resolves its target separately from the Lance segment commit, so stale work can undo a successful concurrent DROP INDEX.
The safe sequence remains to land and release lance#6806, update the pinned Lance dependency, and enable the regression before accepting this command.
Part of #789. Design: #788.
What
CREATE INDEXrebuilds every fragment, so keeping an index current on an append-heavy table means re-indexing the whole table. This adds an incremental counterpart:It indexes only the fragments the index does not cover, distributed the same way
CREATE INDEXis: the driver diffs coverage against the table and balances the remainder by row count, executors build one uncommitted segment per batch, and the driver commits them as one logical index. Lance core keeps existing segments whose fragments are disjoint from the incoming ones, so prior coverage survives the commit.A deferred index (
train = false) covers nothing, so refreshing one builds the whole table through the same distributed path — replacing the current guidance to callDataset.optimizeIndices, which runs on a single node.Also included
Version pinning for distributed index builds. Tasks open the dataset themselves, and
pinLoadedBranchonly pins when the ref is a branch, so the driver planned over one version while each task independently resolved the latest. The segmented build path now pins the planning version, and the commit accounts for the coverage it actually establishes: what the segments declare, restricted to the fragments still live, with anything retired mid-build named in a warning and excluded from the reported count. It fails only when nothing would be covered.Reporting the achieved coverage rather than aborting is deliberate. A fragment leaves the manifest either because its rows moved (compaction, an in-place rewrite) or because they were all deleted; only the first leaves data unindexed, and in both cases the segments for what remains are correct. Discarding a finished distributed build would be the wrong response, and it would make a routine concurrent
DELETEfatal on a long build.Range-mode BTree stays on unpinned read options: it reads the table back through the catalog, which resolves its own version, so pinning would make the segment record a dataset version older than its own contents. The same coverage rule covers both build paths, so range mode is not judged against a fragment list its coverage does not come from.
Index coverage in
SHOW INDEXES:indexed_percent(truncated, so it never overstates; null for an empty table),num_segments,size_bytes— all derived from metadata the command already fetches, so no extra driver cost.Contiguous segment coverage. Lance only compacts fragments covered by an identical set of index segments, so batches whose fragment ids interleave leave
OPTIMIZEnothing to group — measured: a 2-segment index blocked compaction of a table that compacted fine unindexed. Batches are contiguous runs again (as they were before #758) while still balanced by row count: boundaries go where an even split would fall, moved to the nearest fragment. Deterministic, and exactlynum_segmentsbatches even under skew.WITH-clause option names are normalized to lower case at the parser boundary, so
WITH (NUM_SEGMENTS = 8)is the option it looks like. ANTLR reports identifier text as written and every consumer matched lower-case literals, so an uppercase spelling silently took the default and was forwarded to the index backend as a parameter. This fixesCREATE INDEX,OPTIMIZEandVACUUMthe same way.Uncommitted segment builds name their index and set
replace. Letting Lance derive the segment's name yields<column>_idx— the name it assigns an unnamed index — so its collision pre-check fired against the very index being refreshed, andREFRESH INDEX user_id_idxon columnuser_idcould never succeed. On the uncommitted pathreplacegates only that pre-check; the driver's singlecommitExistingIndexSegmentstransaction still decides which segments to keep. The explicit name also moves the genuine "already exists with different fields" check ahead of the distributed build instead of after it.A
DROP INDEXduring a refresh no longer resurrects the index. The commit re-resolves the index on a fresh handle, because committing segments under a name Lance no longer knows creates that index rather than extending it.Fragment enumeration uses
Dataset.getFragmentStatistics()(primitive arrays) instead ofgetFragments(), which materializes a Java object per fragment and per data file on each driver-side pass.ScalarSegmentIndexJobwas made exec-agnostic so both commands share one build path instead of duplicating it.Not in scope
Distributed segment merge. Each refresh adds segments; consolidating them still requires a full
CREATE INDEX. Because Lance only compacts fragments under an identical segment set, accumulated refreshes progressively narrow whatOPTIMIZEcan coalesce. Documented, with the guidance to runOPTIMIZEbeforeREFRESH INDEXrather than after.Vector indexes. Still not creatable or refreshable from Spark SQL; unchanged here.
Inheriting build parameters. Lance records a built index's parameters inside the index, and its Java API exposes no way to read them back —
Dataset.optimizeIndicesderives them internally but runs on a single node, which is the path this replaces. SoREFRESH INDEXtakes options from theWITHclause and falls back to type defaults, and the docs say to repeat the original options.For most methods each segment is queried independently, so a mismatch changes performance rather than results (verified: a
btreerefreshed with a differentzone_sizestill answers point and range queries correctly).fts/invertedis the exception: it is read with one configuration for all of its segments, and a set that disagrees fails every full-text query whileSHOW INDEXESstill reports full coverage. So the refresh compares the index details of the segments it built against the ones they would join and fails without committing when they differ — the segments are uncommitted at that point, so the index is left exactly as it was, and repeating the original options succeeds. A follow-up in lance core exposingderive_index_params()through the Java binding would let a refresh inherit the configuration and remove the caveat entirely.A pre-existing
zonemapread bug. A partially coveredzonemapindex prunes the fragments it does not cover, so a predicate on the indexed column can return fewer rows than the table holds (COUNT(*)over the table stays correct). It is the connector's own driver-side pruning: zone statistics come from the committed segments only, andZonemapFragmentPrunerbuilds its surviving fragment set purely from the fragments those zones name, with no gate on coverage, so a fragment no segment covers contributes no zones and is pruned away. Reproducible withCREATE INDEXalone, so not introduced here, and a coverage gate on the pruner would close it. Documented for now, sinceindexed_percentexists precisely to tell operators when a refresh is needed.btree,bitmapandbloomfilterreturn complete results while partially covered.Multi-field/covering indexes and legacy segments that cannot be partially placed are rejected with a message pointing at
CREATE INDEX.