fix(index): replace same-name index segments across types - #8166
fix(index): replace same-name index segments across types#8166lance-gatekeeper[bot] wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gate recommendation: request changes. The current coverage-based removal still allows a partial type switch to commit a heterogeneous logical index, so normal scalar loading fails after a successful metadata commit. Treat a type change as a whole-name replacement—requiring full current-fragment coverage and removing every same-name segment—or reject the partial cross-type commit before writing metadata.
| .zip(incoming_type_url.as_deref()) | ||
| .is_none_or(|(details, expected)| details.type_url == expected) | ||
| }) | ||
| .map(|idx| -> Result<Option<IndexMetadata>> { |
There was a problem hiding this comment.
Disjoint same-name segments now bypass removal at the is_disjoint branch below even when their index-details type differs from the incoming type, so a successful partial replacement can persist a logical index that readers reject. Require a type switch to cover the whole current dataset and remove every same-name segment, or reject the cross-type commit before the transaction.
Reproducer
I ran this added regression against 2c70daaefbfe25abf255329a9d8259d2ab2afeea:
#[tokio::test]
async fn test_partial_cross_type_replacement_keeps_logical_index_loadable() {
use lance_datagen::{BatchCount, RowCount, array};
let test_dir = tempfile::tempdir().unwrap();
let reader = lance_datagen::gen_batch()
.col("id", array::step::<arrow_array::types::Int32Type>())
.into_reader_rows(RowCount::from(20), BatchCount::from(2));
let mut dataset = Dataset::write(
reader,
test_dir.path().to_str().unwrap(),
Some(WriteParams {
max_rows_per_file: 20,
max_rows_per_group: 20,
..Default::default()
}),
)
.await
.unwrap();
let fragments = dataset.get_fragments();
assert_eq!(fragments.len(), 2);
let btree = ScalarIndexParams::for_builtin(BuiltinIndexType::BTree);
let mut originals = Vec::new();
for fragment in &fragments {
originals.push(
dataset
.create_index_builder(&["id"], IndexType::BTree, &btree)
.fragments(vec![fragment.id() as u32])
.execute_uncommitted()
.await
.unwrap(),
);
}
dataset
.commit_existing_index_segments("shared_name", "id", originals)
.await
.unwrap();
let bitmap = ScalarIndexParams::for_builtin(BuiltinIndexType::Bitmap);
let replacement = dataset
.create_index_builder(&["id"], IndexType::Bitmap, &bitmap)
.fragments(vec![fragments[0].id() as u32])
.execute_uncommitted()
.await
.unwrap();
dataset
.commit_existing_index_segments("shared_name", "id", vec![replacement])
.await
.unwrap();
crate::index::scalar_logical::load_named_scalar_segments(
&dataset,
"id",
"shared_name",
)
.await
.expect("a successful commit must leave a loadable logical index");
}Command: cargo test -p lance --lib test_partial_cross_type_replacement_keeps_logical_index_loadable -- --nocapture
Observed: the commit returned Ok, then the expectation failed with Scalar index shared_name on column id mixes incompatible segment types.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The new head fixes modern metadata but still lets a partial type switch commit a heterogeneous logical index when an existing scalar segment has legacy missing index_details. Infer the legacy type before comparison, or conservatively require full replacement whenever any existing same-name type is unknown, so a successful commit cannot leave readers to discover mixed segment types.
| ))); | ||
| } | ||
| let existing_different_type_url = existing_named_indices.iter().find_map(|idx| { | ||
| let existing_type_url = idx.index_details.as_ref()?.type_url.as_str(); |
There was a problem hiding this comment.
find_map silently skips an existing segment when index_details is None, because this ? returns None from the closure. Stable legacy scalar metadata can have that shape. If it covers fragment 0, a new Bitmap segment for an appended fragment 1 is considered disjoint and commits alongside it; subsequent scalar loading rejects the logical index as mixing incompatible segment types. Infer the missing scalar details before comparing, or treat any unknown same-name segment as a possible type change and require full current-fragment coverage.
Reproducer
I added this regression test to the existing test module on 2e424cf9327ba9010919a7d5efac1c84690264a9:
#[tokio::test]
async fn test_partial_type_change_with_legacy_missing_details_is_rejected() {
use lance_datagen::{BatchCount, RowCount, array};
let test_dir = tempfile::tempdir().unwrap();
let test_uri = test_dir.path().to_str().unwrap();
let reader = lance_datagen::gen_batch()
.col("id", array::step::<arrow_array::types::Int32Type>())
.into_reader_rows(RowCount::from(10), BatchCount::from(1));
let mut dataset = Dataset::write(reader, test_uri, None).await.unwrap();
let btree_params = ScalarIndexParams::for_builtin(BuiltinIndexType::BTree);
let original = dataset
.create_index_builder(&["id"], IndexType::BTree, &btree_params)
.execute_uncommitted()
.await
.unwrap();
dataset
.commit_existing_index_segments("shared_name", "id", vec![original])
.await
.unwrap();
let current = dataset.load_indices_by_name("shared_name").await.unwrap();
let mut legacy = current.clone();
legacy[0].index_details = None;
legacy[0].index_version = 0;
let transaction = Transaction::new(
dataset.manifest.version,
Operation::CreateIndex {
new_indices: legacy,
removed_indices: current,
},
None,
);
dataset
.apply_commit(transaction, &Default::default(), &Default::default())
.await
.unwrap();
let append_reader = lance_datagen::gen_batch()
.col("id", array::step::<arrow_array::types::Int32Type>())
.into_reader_rows(RowCount::from(10), BatchCount::from(1));
let mut dataset = Dataset::write(
append_reader,
test_uri,
Some(WriteParams {
mode: WriteMode::Append,
..Default::default()
}),
)
.await
.unwrap();
let fragments = dataset.get_fragments();
assert_eq!(fragments.len(), 2);
let legacy = dataset.load_indices_by_name("shared_name").await.unwrap();
assert_eq!(legacy.len(), 1);
assert!(legacy[0].index_details.is_none());
let bitmap_params = ScalarIndexParams::for_builtin(BuiltinIndexType::Bitmap);
let replacement = dataset
.create_index_builder(&["id"], IndexType::Bitmap, &bitmap_params)
.fragments(vec![fragments[1].id() as u32])
.execute_uncommitted()
.await
.unwrap();
let version_before = dataset.manifest.version;
let result = dataset
.commit_existing_index_segments("shared_name", "id", vec![replacement])
.await;
assert!(result.is_err(), "partial type change unexpectedly committed");
assert_eq!(dataset.manifest.version, version_before);
}Executed:
cargo test -p lance test_partial_type_change_with_legacy_missing_details_is_rejected -- --nocapture
Observed: the test failed at assert!(result.is_err()) because the partial type change committed. A companion test that allowed the commit confirmed two mixed segments were persisted and load_named_scalar_segments then returned mixes incompatible segment types.
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The legacy case is now covered for normal type URLs, but representing the unknown state with an accepted type-URL string leaves a sentinel collision that can still commit an unreadable mixed-type logical index. Keep unknownness structural—branch on missing details before comparing strings—or reject malformed incoming type URLs before writing metadata.
| let existing_type_url = idx | ||
| .index_details | ||
| .as_ref() | ||
| .map_or("<unknown>", |details| details.type_url.as_str()); |
There was a problem hiding this comment.
Using "<unknown>" both as the missing-details sentinel and as a possible caller-provided Any.type_url lets this guard be bypassed. Public IndexSegment::new accepts the value, and validate_segment_index_details checks only presence and equality; with an existing legacy BTree lacking details, a disjoint incoming segment whose type URL is exactly "<unknown>" compares equal, commits, and leaves scalar loading to reject the mixed types. Although that URL is malformed under the protobuf Any contract, the public boundary currently accepts it, so it must return an input error rather than persist unreadable metadata. Branch on index_details.is_none() so it unconditionally requires full replacement, using the sentinel only for display, or validate type URLs before this comparison.
Reproducer
I added this regression to the existing test module on 64d161d3c05a28f4ac003e5a660e2ebff8c135cb:
#[tokio::test]
async fn test_unknown_type_url_cannot_bypass_legacy_partial_replacement_guard() {
use lance_datagen::{BatchCount, RowCount, array};
let test_dir = tempfile::tempdir().unwrap();
let test_uri = test_dir.path().to_str().unwrap();
let reader = lance_datagen::gen_batch()
.col("id", array::step::<arrow_array::types::Int32Type>())
.into_reader_rows(RowCount::from(10), BatchCount::from(1));
let mut dataset = Dataset::write(reader, test_uri, None).await.unwrap();
let index_name = "sentinel_collision";
let btree_params = ScalarIndexParams::for_builtin(BuiltinIndexType::BTree);
let original = dataset
.create_index_builder(&["id"], IndexType::BTree, &btree_params)
.execute_uncommitted()
.await
.unwrap();
dataset
.commit_existing_index_segments(index_name, "id", vec![original])
.await
.unwrap();
let current = dataset.load_indices_by_name(index_name).await.unwrap();
let mut legacy = current.clone();
legacy[0].index_details = None;
legacy[0].index_version = 0;
let transaction = Transaction::new(
dataset.manifest.version,
Operation::CreateIndex {
new_indices: legacy,
removed_indices: current,
},
None,
);
dataset
.apply_commit(transaction, &Default::default(), &Default::default())
.await
.unwrap();
let append_reader = lance_datagen::gen_batch()
.col("id", array::step::<arrow_array::types::Int32Type>())
.into_reader_rows(RowCount::from(10), BatchCount::from(1));
let mut dataset = Dataset::write(
append_reader,
test_uri,
Some(WriteParams {
mode: WriteMode::Append,
..Default::default()
}),
)
.await
.unwrap();
let fragments = dataset.get_fragments();
assert_eq!(fragments.len(), 2);
let field_id = dataset.schema().field("id").unwrap().id;
let sentinel_segment = IndexSegment::new(
Uuid::new_v4(),
[fragments[1].id() as u32],
[field_id],
Arc::new(prost_types::Any {
type_url: "<unknown>".to_string(),
value: Vec::new(),
}),
0,
dataset.manifest.version,
);
let version_before = dataset.manifest.version;
let result = dataset
.commit_existing_index_segments(index_name, "id", vec![sentinel_segment])
.await;
let loaded =
crate::index::scalar_logical::load_named_scalar_segments(&dataset, "id", index_name)
.await;
assert!(
result.is_err(),
"sentinel type URL bypassed the partial replacement guard; loader result: {loaded:?}"
);
assert!(matches!(result.unwrap_err(), Error::InvalidInput { .. }));
assert_eq!(dataset.manifest.version, version_before);
assert_eq!(loaded.unwrap().len(), 1);
}Executed:
cargo test -p lance test_unknown_type_url_cannot_bypass_legacy_partial_replacement_guard -- --nocapture
Observed: the test failed because the commit returned Ok; the manifest advanced and loaded was Err(InvalidInput("Scalar index 'sentinel_collision' on column 'id' mixes incompatible segment types")).
Summary
Root cause
commit_existing_index_segments originally excluded different-type metadata from removal. Removing that filter fixed full replacements, but the normal coverage logic still retained disjoint old-type segments during a partial type switch. The later type-change guard first skipped legacy segments whose index details were absent, then represented that absence with a string sentinel that could collide with an accepted incoming type URL. Both paths could publish mixed logical index segments.
Validation
Fixes #7842