fix(index): keep an index this build cannot read out of the erase path - #8427
Conversation
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
95f6ca2 to
e2dc2e5
Compare
|
CI has been red on this PR only in unrelated infrastructure jobs, never in its own tests. The run before the current one (33092361390) failed Merged |
|
Both red jobs are known infrastructure flakes, not regressions from this branch. In this same run (33136954128, head
Neither test's file is touched by this diff, which is confined to eight files under I do not have write access here, so I cannot press "Re-run failed jobs". Could a maintainer re-run |
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The complete/visible metadata split still holds across commit, cache, bookkeeping, planning, compaction, and validation after the latest base synchronization. Legacy no-bitmap coverage is reconstructed at its write version and mapped into current fragment IDs before planner exclusion and commit validation, preserving unsupported indices without permitting eager rewrites to strand them.
`migrate_indices` runs on every commit and recalculates a missing `fragment_bitmap` by opening the index, propagating the open with `?`. An index this build cannot open - files removed or never finished, a newer writer, an out-of-reach shallow-clone base - therefore fails that commit and every one after it, leaving the dataset unwritable rather than merely unreadable. The open and the coverage calculation are now caught together and logged at `warn`, like the two neighbouring best-effort steps. The field lookup above them stays fatal: a manifest naming a field the schema lacks is a broken invariant, not an environment condition. Only the pre-0.8.15 trigger drops the coverage to unknown. The other two re-derive from the index metadata and ask again for free, and `calculate_included_frags` is unimplemented for the modern index types, so they keep what they have. The pre-0.8.15 trigger reads the *previous* manifest's writer version, which a successful commit replaces, so a bitmap left in place would look migrated from then on - how the corrupt bitmap in `v0.8.14/corrupt_index`, missing fragment 0, would become permanent. `retain_relevant_indices` must then stop counting a missing bitmap as empty coverage: it runs before migration in the same commit and was deleting the segment before the retry could reach it. An absent bitmap reads as unmeasured, not empty: `index_fragment_coverage` and `PreFilter::new_with_filter_future` both fall back to full coverage, so this only ever widens a scan or a rewrite group. The drop rides `migrate_indices`'s coverage report (#8481), withdrawing any MemWAL catch-up credited off that bitmap. #8427's guard just above is a different case, an index whose *version* has no reader here. --------- Co-authored-by: Vova Kolmakov <wombatukun@apache.org>
Problem
retain_supported_indicesfilters the index list insideload_indices, and the commit path rebuilds the next manifest from exactly that list (build_manifeststarts withlet mut final_indices = current_indices;). For an index whose version this build cannot read, that turns "ignore it" into "delete it" on the next commit of any kind - an append, a delete, a config change. The dataset loses an index a newer Lance wrote and could still use.Invisible inside one process:
commit_transactionseeds the index cache with the unfiltered list, so only a cold reader sees the loss.Fix
Split the two questions
load_indices()was answering.load_all_indicesreturns every index the manifest names;DatasetIndexExt::load_indicesapplies the version filter on the way out. Readers see only what they can read; everything that decides what the next manifest says sees all of it:drop_index, thealter_columnscast guard;plan_compaction,optimize_indices,Dataset::validate- which look like readers but produce the next manifest's index list.Compaction needs one rule beyond the split. Without stable row ids a rewrite moves every row address, and putting an index back in step means opening it, which this build cannot do:
DefaultCompactionPlannerholds back the fragments an unreadable index covers so the rest of the table still compacts, andcommit_compactionrefuses a plan that rewrites them anyway - the boundary a custom planner, a hand-builtCompactionPlanand a distributed driver all pass through.migrate_indicesskips an index it cannot open, andoptimize_indicesskips a name whose segments it cannot all read; both would otherwise have to open it.IndexMetadataKey'sCacheKeySchemagoes 1 -> 2: the cached value's meaning changed while its key fields did not, so on a persistent backend shared with a released client each build would read the other's entry as its own. Precedent:RowIdSequenceKey(#8078).Tests
20 in
rust/lance/src/index.rs, one per site above. Every hunk in the PR was reverted individually to confirm a test fails without it.Out of scope
unsupported_index_versionfalls back toi32::MAXfor an unresolvabletype_url, so a wholly unknown index type is reported as supported. Reversing it needs the system indices exempted first - neither fragment-reuse nor mem-wal details resolve to a scalar plugin - so it belongs in its own PR.