Skip to content

fix(index): declare FM index row address results #8855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lance-gatefixer wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(index): declare FM index row address results #8855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4efa4d7
fix(index): declare FM index row address results
lance-gatefixer[bot] Aug 28, 2026
4a45808
fix(index): preserve FM coverage after rewrites
lance-gatefixer[bot] Aug 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions rust/lance-index/src/scalar/fmindex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1642,6 +1642,11 @@ impl ScalarIndex for FMIndexScalarIndex {
)),
}
}

fn results_are_row_addresses(&self) -> bool {

@lance-gatekeeper lance-gatekeeper Bot Aug 28, 2026

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 fixes query-time translation, but the stable-row-ID maintenance paths do not consult this trait. IndexMetadata::results_are_row_addrs() still recognizes only ZoneMapIndexDetails and BloomFilterIndexDetails, so FM takes the row-ID branch in manifest_build and index_maintenance: after compaction or a pure RewriteRows, its bitmap can claim replacement fragments even though its entries still name old physical addresses. That suppresses fallback scanning and can silently drop matches.

Please add FMIndexDetails to the metadata classifier and add maintenance regression coverage.

Reproducer run on this head

I added this assertion to lance-table:

#[test]
fn test_fm_index_results_are_row_addrs() {
    let mut metadata = index_metadata_with(vec![0], vec![]);
    metadata.index_details = Some(Arc::new(prost_types::Any {
        type_url: "type.googleapis.com/lance.index.pb.FMIndexDetails".to_string(),
        value: Vec::new(),
    }));
    assert!(
        metadata.results_are_row_addrs(),
        "FM search results are physical row addresses"
    );
}

CARGO_TARGET_DIR=/home/agent/tmp/pr8855-implementation-target cargo test -p lance-table test_fm_index_results_are_row_addrs -- --nocapture fails with FM search results are physical row addresses (0 passed, 1 failed).

@lance-gatefixer lance-gatefixer Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 4a45808. FMIndexDetails is now classified in the physical row-address domain, with direct classifier coverage and stable-row-ID RewriteRows and compaction regressions verifying rewritten fragments fall back to scans.

@lance-gatekeeper lance-gatekeeper Bot Aug 28, 2026

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 4a45808c6: FM is now classified as address-domain for manifest maintenance, and the stable-row-ID rewrite and compaction regressions confirm replacement fragments remain uncovered.

true
}

fn can_remap(&self) -> bool {
false
}
Expand Down
16 changes: 16 additions & 0 deletions rust/lance-table/src/format/index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ impl IndexMetadata {
self.index_details.as_ref().is_some_and(|details| {
details.type_url.ends_with("ZoneMapIndexDetails")
|| details.type_url.ends_with("BloomFilterIndexDetails")
|| details.type_url.ends_with("FMIndexDetails")
})
}

Expand Down Expand Up @@ -596,6 +597,21 @@ mod tests {
}
}

#[rstest]
#[case::zone_map("type.googleapis.com/lance.table.ZoneMapIndexDetails", true)]
#[case::bloom_filter("type.googleapis.com/lance.index.pb.BloomFilterIndexDetails", true)]
#[case::fm("type.googleapis.com/lance.index.pb.FMIndexDetails", true)]
#[case::btree("type.googleapis.com/lance.table.BTreeIndexDetails", false)]
fn test_results_are_row_addrs(#[case] type_url: &str, #[case] expected: bool) {
let mut metadata = index_metadata_with(vec![0], vec![]);
metadata.index_details = Some(Arc::new(prost_types::Any {
type_url: type_url.to_string(),
value: Vec::new(),
}));

assert_eq!(metadata.results_are_row_addrs(), expected);
}

#[rstest]
#[case::empty_is_valid(vec![7], vec![], None)]
// mem_wal and frag_reuse both commit no fields at all; a bare
Expand Down
56 changes: 35 additions & 21 deletions rust/lance/src/dataset/optimize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4020,38 +4020,52 @@ mod tests {

/// Regression test for https://github.com/lance-format/lance/issues/8076
///
/// A zone map or bloom filter index reports matches as physical row addresses, so
/// Zone map, bloom filter, and FM indices report matches as physical row addresses, so
/// compaction invalidates it even under stable row ids. Reusing it for the rewritten
/// fragments made a filtered scan fail with an internal error (a fragment referenced
/// by the index no longer existed) or, once translation tolerated that, silently drop
/// every match.
#[rstest]
#[case::zone_map(BuiltinIndexType::ZoneMap, IndexType::ZoneMap)]
#[case::bloom_filter(BuiltinIndexType::BloomFilter, IndexType::BloomFilter)]
#[case::zone_map(BuiltinIndexType::ZoneMap, IndexType::ZoneMap, "i", "i > 0", 199)]
#[case::bloom_filter(BuiltinIndexType::BloomFilter, IndexType::BloomFilter, "i", "i = 0", 1)]
#[case::fm(
BuiltinIndexType::Fm,
IndexType::Fm,
"text",
"contains(text, 'needle')",
100
)]
#[tokio::test]
async fn test_addr_domain_index_after_compaction_with_stable_row_ids(
#[case] builtin: BuiltinIndexType,
#[case] index_type: IndexType,
#[case] indexed_column: &str,
#[case] query: &str,
#[case] expected_rows: usize,
) {
let mut data_gen =
BatchGenerator::new().col(Box::new(IncrementingInt32::new().named("i".to_owned())));
let mut dataset = Dataset::write(
data_gen.batch(200),
"memory://test/table",
Some(WriteParams {
enable_stable_row_ids: true,
max_rows_per_file: 100, // 2 fragments, so compaction has something to merge
..Default::default()
}),
)
.await
.unwrap();
let mut dataset = lance_datagen::gen_batch()
.col("i", lance_datagen::array::step::<Int32Type>())
.col(
"text",
lance_datagen::array::cycle_utf8_literals(&["needle", "haystack"]),
)
.into_ram_dataset_with_params(
FragmentCount::from(2),
FragmentRowCount::from(100),
Some(WriteParams {
enable_stable_row_ids: true,
max_rows_per_file: 100,
..Default::default()
}),
)
.await
.unwrap();

dataset
.create_index(
&["i"],
&[indexed_column],
index_type,
None,
Some("addr_idx".to_string()),
&ScalarIndexParams::for_builtin(builtin),
false,
)
Expand All @@ -4071,7 +4085,7 @@ mod tests {
.await
.unwrap()
.iter()
.find(|index| index.fields == vec![0])
.find(|index| index.name == "addr_idx")
.expect("index must survive compaction")
.clone();
assert!(
Expand All @@ -4084,9 +4098,9 @@ mod tests {
// Every fragment therefore falls back to a full scan, and the filter is answered
// in full.
let mut scanner = dataset.scan();
scanner.filter("i > 0").unwrap();
scanner.filter(query).unwrap();
let matched = scanner.try_into_batch().await.unwrap();
assert_eq!(matched.num_rows(), 199);
assert_eq!(matched.num_rows(), expected_rows);
}

// Regression test for https://github.com/lancedb/lance/issues/6161
Expand Down
76 changes: 76 additions & 0 deletions rust/lance/src/dataset/scanner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11663,6 +11663,82 @@ mod test {
.unwrap();
}

#[tokio::test]
async fn test_fm_index_with_stable_row_ids() {
let batch = arrow_array::record_batch!(
(
"text",
Utf8,
[
"alpha",
"needle in first",
"beta",
"first needle suffix",
"delta",
"needle in second",
"epsilon",
"second needle suffix"
]
),
("id", Int32, [0, 1, 2, 3, 4, 5, 6, 7])
)
.unwrap();
let schema = batch.schema();
let reader = RecordBatchIterator::new(vec![Ok(batch)], schema);
let write_params = WriteParams {
max_rows_per_file: 4,
enable_stable_row_ids: true,
..Default::default()
};
let mut dataset = Dataset::write(
reader,
"memory://test_fm_index_with_stable_row_ids",
Some(write_params),
)
.await
.unwrap();
assert_eq!(dataset.get_fragments().len(), 2);

let params = ScalarIndexParams::for_builtin(lance_index::scalar::BuiltinIndexType::Fm);
dataset
.create_index(&["text"], IndexType::Fm, None, &params, true)
.await
.unwrap();

let mut indexed_scan = dataset.scan();
indexed_scan.filter("contains(text, 'needle')").unwrap();
let indexed_plan = indexed_scan.explain_plan(false).await.unwrap();
assert!(
indexed_plan.contains("ScalarIndexQuery") && indexed_plan.contains("Fm"),
"expected the FM index in the plan, got:\n{indexed_plan}"
);
let indexed = indexed_scan.try_into_batch().await.unwrap();

let unindexed = dataset
.scan()
.use_scalar_index(false)
.filter("contains(text, 'needle')")
.unwrap()
.try_into_batch()
.await
.unwrap();
let indexed_ids = indexed["id"]
.as_primitive::<Int32Type>()
.values()
.iter()
.copied()
.collect::<BTreeSet<_>>();
let unindexed_ids = unindexed["id"]
.as_primitive::<Int32Type>()
.values()
.iter()
.copied()
.collect::<BTreeSet<_>>();

assert_eq!(unindexed_ids, BTreeSet::from([1, 3, 5, 7]));
assert_eq!(indexed_ids, unindexed_ids);
}

#[tokio::test]
async fn test_ngram_regex_index_scan() {
use arrow::array::AsArray;
Expand Down
32 changes: 24 additions & 8 deletions rust/lance/src/dataset/write/update.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1444,17 +1444,30 @@ mod tests {
}

#[rstest]
#[case::zone_map(BuiltinIndexType::ZoneMap, "i < 100", 100)]
#[case::bloom_filter(BuiltinIndexType::BloomFilter, "i = 0", 1)]
#[case::zone_map(BuiltinIndexType::ZoneMap, IndexType::ZoneMap, "i", "i < 100", 100)]
#[case::bloom_filter(BuiltinIndexType::BloomFilter, IndexType::BloomFilter, "i", "i = 0", 1)]
#[case::fm(
BuiltinIndexType::Fm,
IndexType::Fm,
"text",
"contains(text, 'needle')",
50
)]
#[tokio::test]
async fn test_addr_domain_index_does_not_cover_rewritten_update_fragment(
#[case] index_type: BuiltinIndexType,
#[case] builtin: BuiltinIndexType,
#[case] index_type: IndexType,
#[case] indexed_column: &str,
#[case] query: &str,
#[case] expected_rows: usize,
) {
let mut dataset = lance_datagen::gen_batch()
.col("i", lance_datagen::array::step::<Int32Type>())
.col("category", lance_datagen::array::step::<Int32Type>())
.col(
"text",
lance_datagen::array::cycle_utf8_literals(&["needle", "haystack"]),
)
.into_ram_dataset_with_params(
FragmentCount::from(1),
FragmentRowCount::from(100),
Expand All @@ -1469,10 +1482,10 @@ mod tests {

dataset
.create_index(
&["i"],
IndexType::Scalar,
Some("i_idx".to_string()),
&ScalarIndexParams::for_builtin(index_type),
&[indexed_column],
index_type,
Some("addr_idx".to_string()),
&ScalarIndexParams::for_builtin(builtin),
true,
)
.await
Expand Down Expand Up @@ -1500,7 +1513,10 @@ mod tests {
.new_dataset;

let indices = dataset.load_indices().await.unwrap();
let index = indices.iter().find(|index| index.name == "i_idx").unwrap();
let index = indices
.iter()
.find(|index| index.name == "addr_idx")
.unwrap();
assert_eq!(
index
.fragment_bitmap
Expand Down
Loading