Skip to content

feat(index): compose FRI mapping readers with planner safety boundaries - #9068

Merged
LuQQiu merged 1 commit into
mainfrom
lu/fri-query
Sep 23, 2026
Merged

LuQQiu merged 1 commit into
mainfrom
lu/fri-query

Conversation

@LuQQiu

@LuQQiu LuQQiu commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

What this adds

This PR adds the dataset-level reader for FRI index version 1. Datasets without FRI and datasets using version 0 continue through the existing legacy read path.

FRI index metadata
        │
        ▼
decode validated ledger
        │
        ▼
bind one MappingReader per supported transition
        │
        ▼
compose mappings across fragment lineage
        ├── derive query coverage
        └── translate physical row IDs

Ordered-compaction transitions use the existing bitmap/rank mapping. Stable-partition transitions bind their immutable Lance row-map file and load label blocks only when translation reaches that transition.

Coverage planning

Stored index bitmaps describe the fragments from which each index segment was built. The reader groups segments by logical index, combines their usable source coverage, and walks lineage to determine which current destination fragments are completely covered.

The planner receives an in-memory copy of each segment’s derived coverage; persisted index metadata is not changed. Empty coverage is removed so the query scans those fragments. Complete coverage for one destination is retained even when another destination cannot be covered.

When a newer segment directly covers an intermediate or destination fragment, older segments stop contributing along that path. The same exclusion is applied during address translation to avoid duplicate results when lineage branches reconverge.

Address translation

For each physical row ID, the reader follows the relevant mappings forward until the address reaches a current live fragment. Deleted rows return None. Paths containing an unsupported mapping cannot claim derived coverage and fall back to scanning.

Cache and compatibility boundaries

Decoded immutable histories are cached by FRI UUID. Mapping readers can be reused across history updates through their content fingerprint and storage binding, while live fragments remain specific to the dataset snapshot.

The first version-1 FRI commit sets and preserves the paired reader/writer feature flag. Manifest publication verifies that version-1 metadata and the flags agree. Writers and maintenance operations that cannot preserve version-1 history fail with an upgrade error. Restore preserves the selected snapshot’s index metadata and external references.

When the ledger holds any transition this reader cannot interpret, no segment takes the identity fast path: the dropped record's fragments are unknown, so an untouched segment cannot be told apart from one it rewrote. Those segments are excluded and their fragments scanned. This costs scans on tables with such records and is deliberate.

Tagging is one-way. Once a table carries a version-1 history, clients up to 13.0.0-beta.6 refuse it for reads and writes, restore keeps the sticky feature flag, and draining the history does not clear it.

This PR does not modify scalar or vector index loaders. Until consumer integration in #9107, segments requiring version-1 address translation are excluded and their fragments are scanned.

Validation

Tests cover legacy-path isolation, inline and external history, multi-step and branching lineage, partial destination coverage, direct destination coverage, duplicate suppression, deleted rows, unsupported mappings and versions, cache reuse and snapshot isolation, feature-flag publication, restore and clone boundaries, and rejection of unsupported maintenance operations.

Validated with workspace Clippy and cargo fmt --all.

@LuQQiu
LuQQiu added this pull request to stack #9066 September 9, 2026 01:11
@github-actions github-actions Bot added the enhancement New feature or request label Sep 9, 2026
@github-actions github-actions Bot added the A-index Vector index, linalg, tokenizer label Sep 9, 2026
@LuQQiu
LuQQiu removed this pull request from stack #9066 September 9, 2026 19:15
@LuQQiu LuQQiu changed the title feat(index): query tagged fragment reuse histories feat(index): compose FRI mapping readers with planner safety boundaries Sep 9, 2026
@LuQQiu
LuQQiu added this pull request to stack #9108 September 9, 2026 19:16
@LuQQiu
LuQQiu removed this pull request from stack #9108 September 10, 2026 03:15
@LuQQiu
LuQQiu added this pull request to stack #9117 September 10, 2026 03:15
@LuQQiu
LuQQiu removed this pull request from stack #9117 September 10, 2026 18:17
@LuQQiu
LuQQiu added this pull request to stack #9137 September 10, 2026 18:17
@LuQQiu
LuQQiu marked this pull request as ready for review September 10, 2026 18:21

@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: request changes.

Keep table-level system metadata outside fragment-coverage filtering, and reserve coverage remapping for queryable user-index segments. Also move the existing unknown-flag fixtures above the new supported bit. These two changes preserve MemWAL availability on tagged snapshots and keep the existing unsupported-feature guards meaningful.

}
if index.name == lance_index::frag_reuse::FRAG_REUSE_INDEX_NAME {
result[position] = Some(index.clone());
} else {

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.

This branch also sends the __lance_mem_wal system entry through the fragment-coverage groups. MemWAL metadata intentionally has fragment_bitmap: None, so may_need_translation(None) is true and the entry is silently omitted at lines 60–63. Every MemWAL API built on load_index_by_name then sees an initialized dataset as uninitialized and refuses writers. Preserve the MemWAL system entry unchanged, or otherwise restrict coverage rewriting to queryable user-index segments.

Reproducer

I added this regression beside the existing FRI reader tests, using their fixture, prepare, and install helpers:

let mut dataset = fixture().await;
let mem_wal = lance_table::system_index::mem_wal::new_mem_wal_index_meta(
    dataset.manifest.version,
    Default::default(),
).unwrap();
dataset.apply_commit(
    Transaction::new(
        dataset.manifest.version,
        Operation::CreateIndex {
            new_indices: vec![mem_wal],
            removed_indices: vec![],
        },
        None,
    ),
    &Default::default(),
    &Default::default(),
).await.unwrap();
assert!(dataset.load_index_by_name(
    lance_table::system_index::mem_wal::MEM_WAL_INDEX_NAME,
).await.unwrap().is_some());

let (transition, destinations) = prepare(&dataset).await;
let content = InlineContent {
    legacy_versions: vec![],
    transitions: vec![transition],
}.encode_to_vec();
install(&mut dataset, content, destinations, false).await;
assert!(dataset.load_index_by_name(
    lance_table::system_index::mem_wal::MEM_WAL_INDEX_NAME,
).await.unwrap().is_some(), "tagged FRI must not hide the MemWAL system index");

Command: cargo test -p lance index::frag_reuse_reader::tests::tagged_history_keeps_mem_wal_system_index_visible -- --exact

The first lookup succeeded; the final assertion failed with tagged FRI must not hide the MemWAL system index.

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.

Fixed in 63ddfb6: tagged FRI now passes all system indices through the query listing unchanged, and the MemWAL regression covers this path.

Comment thread rust/lance-table/src/feature_flags.rs Outdated
@@ -68,7 +68,8 @@ const _: () = assert!(FLAG_MIXED_DATA_FILE_VERSIONS == FLAG_UNKNOWN);
/// preserves them during maintenance. Legacy-only FRI does not set this bit.
pub const FLAG_FRAGMENT_REUSE_INDEX: u64 = 1 << 9;

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.

FLAG_UNKNOWN << 1 is no longer an unsupported sentinel: it is exactly this newly supported bit (512). Three existing unsupported-writer fixtures still use that expression, so they now construct a one-sided FRI flag pair instead of an unknown feature. In both clone regressions, manifest setup returns CorruptFile before the tests can exercise the intended NotSupported clone boundary. Move all of those fixtures to a genuinely unsupported bit above the highest supported flag, such as FLAG_FRAGMENT_REUSE_INDEX << 1.

Reproducer

Command: cargo test -p lance clone_rejects_unsupported_writer_before -- --nocapture

Expected: both clone guard tests pass. Observed: both failed while writing the fixture manifest with FRI requires both reader and writer feature flags. The same stale sentinel also appears in rust/lance-namespace-impls/src/dir/manifest.rs.

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.

Fixed in c8f1821: FLAG_UNKNOWN is now bit 11, above the supported FRI bit 10, so the existing FLAG_UNKNOWN << 1 fixtures again exercise unsupported-feature guards instead of a one-sided FRI flag pair.

@LuQQiu

LuQQiu commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Adjudicated both points.

System metadata in coverage filtering: confirmed, fixed in 433d8c4 on #9139 (this branch is frozen while the format vote runs, so the fix rides the stack tip). frag_reuse_reader::load_indices exempted only the FRI by name; MemWAL's metadata carries no fragment bitmap and no remap plugin, so tagged snapshots silently dropped it, taking open_mem_wal_index, catch-up, and statistics with it. The exemption is now is_system_index, so table-level system metadata bypasses coverage filtering and remapping entirely. Regression test: a real MemWAL entry survives tagged filtering and opens, while a user index without a bitmap is still dropped.

Unknown-flag fixtures: not applicable as stated. FLAG_UNKNOWN remains 1 << 8 and the supported set is carved explicitly as (FLAG_UNKNOWN - 1) | FLAG_FRAGMENT_REUSE_INDEX, so the existing unknown-flag guards still exercise rejected bits (256, 512) and none is vacuous. The bit move did leave four stale & 512 == 0 assertions in frag_reuse_reader tests (trivially true, wrong flag); those now assert against the FLAG_FRAGMENT_REUSE_INDEX constant in the same commit.

LuQQiu added a commit that referenced this pull request Sep 15, 2026
FRI keeps existing indices usable after fragment rewrites by translating
old physical row addresses to new ones. Today it supports
order-preserving compaction. This proposal extends the same system index
to support stable partitioning, with both mapping types sharing one
fragment-lineage history.

Following [the unified FRI
proposal](#8972 (comment)),
source/destination lineage stays in FRI details, while large mapping
payloads remain in separate immutable files.

### One history, multiple mappings

Continue storing FRI information in a single `__lance_frag_reuse`
system-index entry. Keep the existing `InlineContent` / `ExternalFile`
envelope and the original field number for legacy versions. Add tagged
transitions alongside them:

```text
FragmentReuseIndexDetails
└── InlineContent, stored inline or in external details.binpb
    ├── legacy_versions[]
    └── transitions[]
        ├── ordered sources[]
        ├── ordered destinations[]
        └── mapping
            ├── OrderedCompaction: surviving-row bitmap
            └── StablePartition: immutable row-map reference
```

Sources and destinations define the common rewrite graph. Each mapping
defines how to translate row offsets. Legacy groups can be read as
ordered-compaction transitions; mixed histories follow fragment lineage,
not the order of records or dataset version numbers.

### Lightweight metadata, external row maps

Ordered compaction retains its compact bitmap representation. Stable
partition assigns each physical source row a nullable `uint16`
destination label, preserving source order within each destination. A
null label means the row was deleted. A counts matrix lets readers
reconstruct destination offsets without reading all preceding labels.

Stable-partition metadata records `map_id`, `map_size_bytes`, and
optional `base_id`. The labels and counts are stored in
`_fri/<map_id>/stable_partition.lance`. Mapping identity is independent
of the FRI index UUID: updating the history rewrites its metadata, but
does not rewrite existing row-map files. The history can be opened
without loading labels; address translation reads the required blocks.

### Publication and compatibility

`AppendFragmentReuseTransitions` expresses a transition delta. Combined
atomically with a fragment rewrite, it lets the commit apply the delta
to the current history and publish destination fragments and their
mappings together. The persisted FRI details remain a snapshot of that
history.

- **Index version 0:** existing compaction format and read/write
behavior remain unchanged.
- **Index version 1:** supports legacy groups and tagged transitions in
one history.
- The first commit publishing index version 1 sets reader and writer
flag **512**. The reader flag prevents old clients from partially
interpreting the history; the writer flag prevents them from dropping
mappings during metadata maintenance. Subsequent manifests retain both
bits.

### Scope and validation

This PR contains protobuf definitions, the corresponding format
documentation, the proposed flag constant, and minimal compile adapters.
It does not enable tagged-history reads or writes. Mapping
implementations and reader integration follow in #9106 → #9064 → #9067 →
#9068 → #9107.

Replaces #9065 as the standalone spec at the bottom of native stack
#9137, based on main `31d78d170`.

`cargo fmt --all` and whitespace checks pass. Clippy and tests are
blocked by dependency resolution: main requires `object_store_opendal
0.60.1`, while the crates.io index currently offers only up to 0.60.0.

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@LuQQiu
LuQQiu force-pushed the lu/fri-query branch 2 times, most recently from 89aa27f to 96be481 Compare September 18, 2026 18:08
@LuQQiu LuQQiu closed this Sep 18, 2026
@LuQQiu LuQQiu reopened this Sep 18, 2026
@LuQQiu LuQQiu closed this Sep 18, 2026
@LuQQiu LuQQiu reopened this Sep 18, 2026
@LuQQiu LuQQiu closed this Sep 18, 2026
@LuQQiu LuQQiu reopened this Sep 18, 2026
@LuQQiu LuQQiu closed this Sep 19, 2026
@LuQQiu LuQQiu reopened this Sep 19, 2026
@LuQQiu LuQQiu closed this Sep 20, 2026
@LuQQiu LuQQiu reopened this Sep 20, 2026
@LuQQiu LuQQiu closed this Sep 20, 2026
@LuQQiu LuQQiu reopened this Sep 20, 2026

@jackye1995 jackye1995 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.

Read through the reader and the write-path fences end to end. Two substantive points below: the MemWAL exemption currently riding a different PR in the stack, and the identity grant for segments when the ledger has dropped unknown transitions. The coverage backtrack, worklist termination, cache keying, and the flag fencing all looked right to me.

if !super::index_is_usable(index) {
continue;
}
if index.name == lance_index::frag_reuse::FRAG_REUSE_INDEX_NAME {

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.

The by-name exemption here only covers the FRI entry. __lance_mem_wal carries fragment_bitmap: None by design, so it falls into the coverage groups below, may_need_translation(None) is true, and the entry is silently dropped from the listing. Everything built on load_index_by_name then sees an initialized dataset as uninitialized: open_mem_wal_index loses the fresh tier on reads, the writer refuses with "not initialized", and the already-initialized guard would wave through a second init.

I saw the earlier gatekeeper report, and that the fix (passing all is_system_index entries through untouched) is riding #9139 because this branch was frozen during the format vote. Now that the branch is moving again, any reason not to pull that change into this PR directly? At this commit the reader is broken for any dataset carrying both MemWAL and a tagged FRI, and nothing guarantees the two PRs land together. Relatedly, the & 512 assertions in the tests further down are still pointing at the wrong bit now that the tagged-FRI flag is 1024 — also fixed only on #9139.

/// Dead fragments may belong to an omitted unknown mapping; those must not
/// get an identity remapper merely because they are absent from the graph.
pub fn may_need_translation(&self, provenance: Option<&RoaringBitmap>) -> bool {
provenance.is_none_or(|bitmap| {

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.

contains_fragment only knows the retained lineage, but decode drops transitions with unknown fields wholesale, so a live fragment produced by an unknown mapping is invisible to this check. Concretely: a future v1 writer adds a new mapping type as an extra field inside a transition (still index_version 1 — exactly the evolution the unknown-field machinery exists for) and, following the projected-coverage model, projects an older segment's bitmap onto that transition's live destinations while the segment's stored addresses stay pre-transition. This reader then sees an all-live bitmap disjoint from the retained graph, grants identity, and hands back stale addresses as current rows — no error anywhere.

The contains_fragment clause exists precisely because projection is possible, but its protection evaporates for dropped transitions, and once a transition is dropped we can't tell which fragments it touched at all — so "not on a path containing an unsupported mapping" can't be established, which is the invariant the PR description promises ("paths containing an unsupported mapping ... fall back to scanning"). I'd have has_unsupported_transitions() force translation for everything. Today every surviving segment is an identity segment anyway, so the blunt version costs nothing until the consumers land — and the tip branch still carries this function unchanged, so it seems worth settling here rather than later.

Comment thread rust/lance/src/index.rs Outdated
// Legacy FRI index version 0 already had its fragment coverage
// remapped in load_all_indices().
0 => {}
1 => return frag_reuse_reader::load_indices(self, fri, &indices).await,

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.

This branch reopens the reader and recomputes segment_coverage plus the metadata rewrites on every call — the ledger and the mapping readers are cached, but the derived listing is not. load_indices sits on the query-planning path and on merge_insert's per-batch path, so on a wide dataset this is per-batch work proportional to groups × live fragments, redone on every call.

The metadata-only callers are the part I'd want addressed regardless: frag_reuse_index_uuid just wants a uuid, and the legacy open_frag_reuse_index and index_statistics_frag_reuse both reach the full reader through load_index_by_name and then throw the result away (Ok(None) / NotSupported). They also newly fail on a corrupt ledger where they previously couldn't. What do you think about caching the derived listing per (manifest version, FRI uuid), mirroring how IndexMetadataKey caches the raw listing, and pointing the bookkeeping lookups at load_all_indices? That would also preserve the bookkeeping-vs-query split the load_all_indices doc comment draws.

Comment thread rust/lance/src/lib.rs
// type nests past the default 128 in the lib test build. benches/streaming_ivf_training.rs
// raises the limit for the same stack.
#![cfg_attr(test, recursion_limit = "256")]
#![recursion_limit = "256"]

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.

The comment above still describes the test-only rationale ("in the lib test build"), but the attribute now applies to every build and nothing records why. Could we update it to name the actual non-test cause (the reader's future nesting?), or box the deep future so the crate-wide limit doesn't have to grow?

Base automatically changed from lu/fri-reader to main September 22, 2026 01:37

@jackye1995 jackye1995 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.

All four review comments are addressed with regression tests (system-index pass-through, identity denial under unsupported transitions, derived-listing cache with bookkeeping off the query reader, and the flag/comment cleanups). The coverage backtrack, cache keying, and flag fencing all look right to me. Thanks for the thorough test additions.

@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 current revision closes the earlier system-index and feature-flag findings. It keeps table-level metadata outside query coverage rewriting, denies unproven identity mappings, and falls back to scanning when translation is unsupported. The snapshot-derived listing is cached without burdening metadata-only lookups. I found no remaining acceptance concern.

@lance-gatekeeper lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. and removed K-approved Latest Gatekeeper recommendation permits acceptance. labels Sep 23, 2026
Adds a reader that composes tagged FRI mapping readers, derives segment coverage/exclusions, and backtracks lineage across the reuse chain. Includes the tagged-FRI feature-flag bit (supported + sticky, paired reader/writer validation) and the frag_reuse metadata guards (is_tagged, validate_flags, ensure_clone_supported). Maintenance and clone paths reject tagged histories they cannot interpret; append carries future FRI versions without interpreting them.

Review follow-ups folded in: system indexes pass through the coverage rewrite untouched and no segment takes the identity fast path while the ledger holds a transition this reader cannot interpret (it falls back to scanning); the derived FRI listing is cached per snapshot and bookkeeping callers use the raw listing instead of the query reader.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 23, 2026
@LuQQiu

LuQQiu commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

just squash all the commits into 1 commit and prepare for merge, code does not change

@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 squashed change preserves the FRI reader safety boundaries: system metadata remains visible, unknown transitions cannot grant identity, and untranslatable segments fall back to scans. Snapshot-derived coverage stays cached without affecting metadata-only lookups. I found no remaining acceptance concern.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 23, 2026
@LuQQiu
LuQQiu merged commit 3d69826 into main Sep 23, 2026
50 of 51 checks passed
@LuQQiu
LuQQiu deleted the lu/fri-query branch September 23, 2026 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-index Vector index, linalg, tokenizer enhancement New feature or request K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants