diff --git a/rust/lance-table/src/transaction/index_maintenance.rs b/rust/lance-table/src/transaction/index_maintenance.rs index f1810d8795b..06cf9648a88 100644 --- a/rust/lance-table/src/transaction/index_maintenance.rs +++ b/rust/lance-table/src/transaction/index_maintenance.rs @@ -257,6 +257,16 @@ impl Transaction { .collect::(); for (_, same_name_indices) in indices_by_name { + // Unknown coverage is not empty coverage: a segment whose bitmap is + // missing has never been measured, and dropping it deletes an index + // that migration could not open yet. + let (unknown_coverage, same_name_indices): (Vec<_>, Vec<_>) = same_name_indices + .into_iter() + .partition(|index| index.fragment_bitmap.is_none()); + for index in unknown_coverage { + uuids_to_keep.insert(index.uuid); + } + if same_name_indices.len() > 1 { let (empty_indices, non_empty_indices): (Vec<_>, Vec<_>) = same_name_indices.iter().partition(|index| { @@ -626,12 +636,31 @@ mod tests { Transaction::retain_relevant_indices(&mut scalar_indices, &schema, &fragments); Transaction::retain_relevant_indices(&mut vector_indices, &schema, &fragments); - // Both kept: a None bitmap counts as empty coverage, and empty - // definitions are retained regardless of index type. + // Both kept: a None bitmap is unknown coverage, not empty coverage, and + // an unmeasured segment is retained regardless of index type. assert_eq!(scalar_indices.len(), 1); assert_eq!(vector_indices.len(), 1); } + #[test] + fn test_retain_unknown_coverage_alongside_nonempty_sibling() { + let schema = create_test_schema(&[1]); + let fragments = vec![Fragment::new(1), Fragment::new(2)]; + + let mut indices = vec![ + create_test_index("idx", 1, 1, None, false), // Coverage never measured + create_test_index("idx", 1, 2, Some(RoaringBitmap::from_iter([2])), false), + ]; + + Transaction::retain_relevant_indices(&mut indices, &schema, &fragments); + + // The unmeasured segment must survive its non-empty sibling: its bitmap + // is missing because migration could not open the index, and deleting + // the segment would take the only record of it with it. + assert_eq!(indices.len(), 2); + assert!(indices.iter().any(|idx| idx.fragment_bitmap.is_none())); + } + #[test] fn test_retain_multiple_empty_scalar_indices_keeps_oldest() { let schema = create_test_schema(&[1]); diff --git a/rust/lance/src/dataset/tests/dataset_migrations.rs b/rust/lance/src/dataset/tests/dataset_migrations.rs index 7319b0efa8c..60f735a4d2d 100644 --- a/rust/lance/src/dataset/tests/dataset_migrations.rs +++ b/rust/lance/src/dataset/tests/dataset_migrations.rs @@ -281,6 +281,43 @@ async fn test_v0_8_14_invalid_index_fragment_bitmap( assert_eq!(row_count, 1900); } +/// The repair above is triggered by the writer version of the manifest being +/// committed *from*, and a successful commit stamps the current one. So a +/// commit that cannot open the index has exactly one chance at the corrupt +/// bitmap, and carrying it through unverified would hand a later build a +/// bitmap that looks migrated. +#[tokio::test] +async fn test_v0_8_14_invalid_index_fragment_bitmap_repair_is_not_lost() { + let test_dir = copy_test_data_to_tmp("v0.8.14/corrupt_index").unwrap(); + let test_uri = test_dir.path_str(); + + let indices_dir = test_dir.std_path().join("_indices"); + let stashed_dir = test_dir.std_path().join("_indices_stashed"); + std::fs::rename(&indices_dir, &stashed_dir).unwrap(); + + let mut dataset = Dataset::open(&test_uri).await.unwrap(); + dataset.delete("false").await.unwrap(); + + for idx in dataset.load_indices().await.unwrap().iter() { + assert_eq!( + idx.fragment_bitmap, None, + "a bitmap the migration could not verify must be recorded as unknown" + ); + } + + std::fs::rename(&stashed_dir, &indices_dir).unwrap(); + + let mut dataset = Dataset::open(&test_uri).await.unwrap(); + dataset.delete("false").await.unwrap(); + + for idx in dataset.load_indices().await.unwrap().iter() { + assert!( + idx.fragment_bitmap.as_ref().unwrap().contains(0), + "the first build that can open the index must repair the coverage" + ); + } +} + #[tokio::test] async fn test_fix_v0_10_5_corrupt_schema() { // Schemas could be corrupted by successive calls to `add_columns` and @@ -354,6 +391,50 @@ async fn test_fix_v0_21_0_corrupt_fragment_bitmap() { assert_eq!(get_bitmap(&indices[1]), vec![1]); } +/// Unlike the pre-0.8.15 trigger, an overlap is re-derived from the index +/// metadata on every commit, so it asks to be recalculated again on its own. A +/// commit that cannot open the index has nothing to preserve and must leave the +/// coverage alone: `None` is a state modern indices are not built to recover +/// from, since `calculate_included_frags` exists only for old manifests. +#[tokio::test] +async fn test_v0_21_0_corrupt_fragment_bitmap_kept_when_index_cannot_be_opened() { + let test_dir = copy_test_data_to_tmp("v0.21.0/bad_index_fragment_bitmap").unwrap(); + let test_uri = test_dir.path_str(); + + std::fs::rename( + test_dir.std_path().join("_indices"), + test_dir.std_path().join("_indices_stashed"), + ) + .unwrap(); + + fn coverage(indices: &[IndexMetadata]) -> Vec<(String, Option>)> { + let mut coverage = indices + .iter() + .map(|idx| { + ( + idx.uuid.to_string(), + idx.fragment_bitmap + .as_ref() + .map(|bitmap| bitmap.iter().collect()), + ) + }) + .collect::>(); + coverage.sort(); + coverage + } + + let mut dataset = Dataset::open(&test_uri).await.unwrap(); + let before = coverage(&dataset.load_indices().await.unwrap()); + + dataset.delete("false").await.unwrap(); + + assert_eq!( + coverage(&dataset.load_indices().await.unwrap()), + before, + "coverage the overlap check will ask about again must be left as it stands" + ); +} + #[tokio::test] async fn test_v8_decimal_zonemap_missing_extrema() { async fn query_ids( diff --git a/rust/lance/src/io/commit.rs b/rust/lance/src/io/commit.rs index 397087c709f..e2ee10ce8d3 100644 --- a/rust/lance/src/io/commit.rs +++ b/rust/lance/src/io/commit.rs @@ -955,9 +955,12 @@ async fn migrate_indices(dataset: &Dataset, indices: &mut [IndexMetadata]) -> Re if unsupported_index_version(index).is_some() { continue; } + // Also true when the bitmap is missing entirely, so the failure path below + // pairs it with `is_some` to mean "written before the 0.8.15 fix". + let bitmap_missing_or_legacy = + must_recalculate_fragment_bitmap(index, dataset.manifest.writer_version.as_ref()); if needs_recalculating.contains(&index.name) - || must_recalculate_fragment_bitmap(index, dataset.manifest.writer_version.as_ref()) - && !is_system_index(index) + || bitmap_missing_or_legacy && !is_system_index(index) { // A covered index still has exactly one keyed field; the trailing // `covering_fields` are carried, not keyed, so counting them @@ -970,14 +973,49 @@ async fn migrate_indices(dataset: &Dataset, indices: &mut [IndexMetadata]) -> Re ); let idx_field = dataset.schema().field_by_id(index.fields[0]).ok_or_else(|| Error::internal(format!("Index with uuid {} referred to field with id {} which did not exist in dataset", index.uuid, index.fields[0])))?; // We need to calculate the fragments covered by the index - let idx = dataset - .open_generic_index(&idx_field.name, &index.uuid, &NoOpMetricsCollector) - .await?; - let recalculated = idx.calculate_included_frags().await?; - if index.fragment_bitmap.as_ref() != Some(&recalculated) { - recovered_coverage.push(index.name.clone()); + let recalculated = async { + let idx = dataset + .open_generic_index(&idx_field.name, &index.uuid, &NoOpMetricsCollector) + .await?; + idx.calculate_included_frags().await + } + .await; + match recalculated { + Ok(fragment_bitmap) => { + if index.fragment_bitmap.as_ref() != Some(&fragment_bitmap) { + recovered_coverage.push(index.name.clone()); + } + index.fragment_bitmap = Some(fragment_bitmap); + } + Err(e) => { + // Recalculating means opening the index, and failing here fails + // every commit the dataset takes, since migration runs on all of + // them. A missing bitmap and overlapping segment bitmaps are both + // re-derived from the index metadata, so they ask again on their + // own; the pre-0.8.15 trigger reads the previous manifest's writer + // version, which this commit replaces with the current one, and a + // bitmap left in place would look migrated from here on. + let repair_ends_with_this_commit = + index.fragment_bitmap.is_some() && bitmap_missing_or_legacy; + log::warn!( + "Could not recalculate the fragment bitmap for index {} (uuid: {}): {}. {}", + index.name, + index.uuid, + e, + if repair_ends_with_this_commit { + "Dropping its coverage to unknown so a build that can open the index recalculates it." + } else { + "Leaving the repair to a build that can open the index." + } + ); + if repair_ends_with_this_commit { + index.fragment_bitmap = None; + // Derivation ran before this and may have credited a + // catch-up position off the bitmap being dropped here. + recovered_coverage.push(index.name.clone()); + } + } } - index.fragment_bitmap = Some(recalculated); } // We can't reliably recalculate the index type for label_list and bitmap indices and so we can't migrate this field. // However, we still log for visibility and to help potentially diagnose issues in the future if we grow to rely on the field. @@ -1942,6 +1980,117 @@ mod tests { assert!(dataset.checkout_version(4).await.is_err()); } + /// Every commit runs `migrate_indices`, and recalculating a missing + /// `fragment_bitmap` there means opening the index. An index this build + /// cannot open must not take the write path down with it: the dataset would + /// be unwritable, not merely unreadable, and every later commit would fail + /// the same way. + #[tokio::test] + async fn test_commit_survives_an_index_it_cannot_open() { + use lance_index::scalar::{BuiltinIndexType, ScalarIndexParams}; + use lance_table::io::manifest::read_manifest_indexes; + + let test_dir = TempStrDir::default(); + let test_uri = test_dir.as_str(); + + let reader = gen_batch() + .col("id", array::step::()) + .col("payload", array::step::()) + .into_reader_rows(RowCount::from(10), BatchCount::from(1)); + let mut dataset = Dataset::write(reader, test_uri, None).await.unwrap(); + + // The readable companion is what makes the difference visible: with a + // single index, "carried through the one it cannot open" and "stopped + // recalculating altogether" answer every assertion below the same way. + let btree_params = ScalarIndexParams::for_builtin(BuiltinIndexType::BTree); + for column in ["id", "payload"] { + dataset + .create_index_builder(&[column], IndexType::BTree, &btree_params) + .name(format!("{column}_idx")) + .await + .unwrap(); + } + + let broken = dataset.load_index_by_name("id_idx").await.unwrap().unwrap(); + dataset + .object_store + .remove_dir_all(dataset.indices_dir().join(broken.uuid.to_string())) + .await + .unwrap(); + + // Reopened so the fixture is judged on what is on disk rather than on + // what this process still holds from building the index. + let mut dataset = Dataset::open(test_uri).await.unwrap(); + assert!( + dataset + .open_generic_index("id", &broken.uuid, &NoOpMetricsCollector) + .await + .is_err(), + "the fixture is supposed to leave an index this build cannot open" + ); + + // Migration recalculates a bitmap that is missing, and no current writer + // emits one - untrained indices get an empty bitmap, not none at all - so + // the state an old manifest arrives in is set here by hand. + let indices = read_manifest_indexes( + &dataset.object_store, + &dataset.manifest_location, + &dataset.manifest, + ) + .await + .unwrap(); + let without_bitmaps = indices + .iter() + .map(|index| IndexMetadata { + fragment_bitmap: None, + ..index.clone() + }) + .collect::>(); + let transaction = Transaction::new( + dataset.manifest.version, + Operation::CreateIndex { + new_indices: without_bitmaps, + removed_indices: indices, + }, + None, + ); + dataset + .apply_commit(transaction, &Default::default(), &Default::default()) + .await + .unwrap(); + + // And an unrelated commit after it, since the missing bitmap is now what + // the manifest holds and migration retries on every commit. + dataset.delete("false").await.unwrap(); + + let migrated = read_manifest_indexes( + &dataset.object_store, + &dataset.manifest_location, + &dataset.manifest, + ) + .await + .unwrap(); + let coverage = |name: &str| { + migrated + .iter() + .find(|index| index.name == name) + .unwrap_or_else(|| panic!("no index named {name} in the manifest")) + .fragment_bitmap + .as_ref() + .map(|bitmap| bitmap.iter().collect::>()) + }; + assert_eq!( + coverage("id_idx"), + None, + "an index that cannot be opened must report unknown coverage" + ); + assert_eq!( + coverage("payload_idx"), + Some(vec![0]), + "an index that opens must still have its coverage recalculated" + ); + } + #[tokio::test] async fn test_load_and_sort_new_transactions() { // Create a dataset