diff --git a/java/lance-jni/src/utils.rs b/java/lance-jni/src/utils.rs
index c9d2d2005b1..09bc09207da 100644
--- a/java/lance-jni/src/utils.rs
+++ b/java/lance-jni/src/utils.rs
@@ -515,6 +515,7 @@ pub fn get_vector_index_params(
version: IndexFileVersion::V3,
skip_transpose: false,
runtime_hints: Default::default(),
+ covering_columns: Default::default(),
})
},
)?;
diff --git a/java/src/main/java/org/lance/index/IndexDescription.java b/java/src/main/java/org/lance/index/IndexDescription.java
index b1e2689a00c..09a182c192b 100755
--- a/java/src/main/java/org/lance/index/IndexDescription.java
+++ b/java/src/main/java/org/lance/index/IndexDescription.java
@@ -69,7 +69,14 @@ public String getName() {
return name;
}
- /** Field ids that this index is built on. */
+ /**
+ * Field ids that this index is built on -- the columns it can answer queries for.
+ *
+ *
This is the index's keyed prefix only. An index may additionally carry values for
+ * columns it is not keyed on; those are deliberately absent here, because the index cannot be
+ * searched on them. They stay reachable per segment via {@link Index#coveringFields()} on the
+ * entries of {@link #getMetadata()}.
+ */
public List getFieldIds() {
return fieldIds;
}
diff --git a/python/src/indices.rs b/python/src/indices.rs
index acb15491fcb..164dee21ac6 100644
--- a/python/src/indices.rs
+++ b/python/src/indices.rs
@@ -674,9 +674,11 @@ pub struct PyIndexDescription {
pub type_url: String,
/// The short type of the index (may not be unique)
pub index_type: String,
- /// The ids of the fields that the index is built on
+ /// The ids of the fields the index is keyed on -- the columns it can answer queries about.
+ /// Covering ("included") columns are excluded; those are reported per-segment as
+ /// `covering_fields`.
pub fields: Vec,
- /// The full paths of the fields that the index is built on
+ /// The full paths of the fields the index is keyed on, matching `fields`
/// (dotted, with backtick-quoted segments for non-identifier names)
pub field_names: Vec,
/// The number of rows indexed by the index
diff --git a/rust/lance-index/src/traits.rs b/rust/lance-index/src/traits.rs
index f5441bd80f0..9980530d278 100644
--- a/rust/lance-index/src/traits.rs
+++ b/rust/lance-index/src/traits.rs
@@ -344,7 +344,13 @@ pub trait IndexDescription: Send + Sync {
/// deleted.
fn rows_indexed(&self) -> u64;
- /// Returns the ids of the fields that the index is built on.
+ /// Returns the ids of the fields that the index is built on -- the columns it can
+ /// answer queries for.
+ ///
+ /// This is the index's *keyed* prefix only. An index may additionally carry values
+ /// for columns it is not keyed on (see [`IndexMetadata::covering_fields`]); those are
+ /// deliberately absent here, because the index cannot be searched on them. They stay
+ /// reachable per segment via [`Self::metadata`].
fn field_ids(&self) -> &[u32];
/// Returns a JSON string representation of the index details
diff --git a/rust/lance-index/src/vector/bq/storage.rs b/rust/lance-index/src/vector/bq/storage.rs
index 0f7f034b2c1..cf719789e94 100644
--- a/rust/lance-index/src/vector/bq/storage.rs
+++ b/rust/lance-index/src/vector/bq/storage.rs
@@ -44,6 +44,7 @@ use crate::frag_reuse::{FragReuseIndex, FragReuseIndexHandle};
use crate::pb;
use crate::scalar::RowIdRemapper;
use crate::vector::ApproxMode;
+use crate::vector::PART_ID_COLUMN;
use crate::vector::bq::dist_table_quant::{
DistTableDequant, quantize_dist_table_into, quantize_dist_table_u16_into,
};
@@ -66,6 +67,7 @@ use crate::vector::pq::storage::transpose;
use crate::vector::quantizer::{QuantizerMetadata, QuantizerStorage};
use crate::vector::storage::{
DistCalculator, DistanceCalculatorOptions, QueryResidual, RabitRawQueryContext, VectorStore,
+ covering_field_indices_excluding,
};
pub const RABIT_METADATA_KEY: &str = "lance:rabit";
@@ -2048,6 +2050,29 @@ impl VectorStore for RabitQuantizationStorage {
self.batch.schema_ref()
}
+ /// RQ storage carries the row id plus many internal columns (the binary codes, the
+ /// extended-bit codes, and the per-row factor columns, some present only for higher
+ /// `num_bits`); the covering ("included") columns are everything else.
+ fn covering_field_indices(&self) -> Vec {
+ // Every RaBitQ-internal column plus the legacy `__ivf_part_id` column
+ // (left in pre-#3606 single-partition builds; unlike PQ, RaBitQ storage
+ // does not drop it at construction, so it is excluded here). Anything
+ // else is a user-declared covering column.
+ const INTERNAL: &[&str] = &[
+ ROW_ID,
+ RABIT_CODE_COLUMN,
+ RABIT_EX_CODE_COLUMN,
+ RABIT_BLOCKED_EX_CODE_COLUMN,
+ ADD_FACTORS_COLUMN,
+ SCALE_FACTORS_COLUMN,
+ ERROR_FACTORS_COLUMN,
+ EX_ADD_FACTORS_COLUMN,
+ EX_SCALE_FACTORS_COLUMN,
+ PART_ID_COLUMN,
+ ];
+ covering_field_indices_excluding(self.schema().as_ref(), INTERNAL)
+ }
+
fn to_batches(&self) -> Result + Send> {
Ok(std::iter::once(self.batch.clone()))
}
@@ -3035,6 +3060,39 @@ mod tests {
.unwrap()
}
+ #[test]
+ fn test_covering_excludes_legacy_part_id_column() {
+ // Same legacy `__ivf_part_id` guard as the other quantizer storages:
+ // pre-#3606 `num_partitions=1` builds left it in the storage file, and
+ // it must never be classified as a covering column -- otherwise search
+ // emits a column the exec's declared schema (from
+ // `IndexMetadata.covering_fields`, empty for those indexes) lacks.
+ use crate::vector::PART_ID_COLUMN;
+ use arrow_array::UInt32Array;
+ use arrow_schema::{DataType, Field, Schema};
+
+ let code_dim = 64;
+ let codes = make_test_codes(10, code_dim);
+ let metadata = make_test_metadata(codes.value_length() as usize * 8);
+ let base = make_test_batch(codes);
+
+ let mut fields: Vec<_> = base.schema().fields().iter().cloned().collect();
+ fields.push(Arc::new(Field::new(PART_ID_COLUMN, DataType::UInt32, true)));
+ let mut columns = base.columns().to_vec();
+ columns.push(Arc::new(UInt32Array::from_iter_values(
+ (0..base.num_rows()).map(|_| 0),
+ )) as ArrayRef);
+ let batch = RecordBatch::try_new(Arc::new(Schema::new(fields)), columns).unwrap();
+
+ let storage =
+ RabitQuantizationStorage::try_from_batch(batch, &metadata, DistanceType::L2, None)
+ .unwrap();
+ assert!(
+ storage.covering_field_indices().is_empty(),
+ "legacy __ivf_part_id must not be classified as a covering column"
+ );
+ }
+
fn make_test_ex_codes(num_vectors: usize, code_dim: usize, num_bits: u8) -> FixedSizeListArray {
let ex_bits = rabit_ex_bits(num_bits).unwrap();
let ex_code_bytes = rabit_ex_code_bytes(code_dim, ex_bits).unwrap();
diff --git a/rust/lance-index/src/vector/flat/storage.rs b/rust/lance-index/src/vector/flat/storage.rs
index 1de4c4387fb..3c90f0c9e01 100644
--- a/rust/lance-index/src/vector/flat/storage.rs
+++ b/rust/lance-index/src/vector/flat/storage.rs
@@ -6,8 +6,11 @@ use std::{borrow::Cow, sync::Arc};
use super::index::FlatMetadata;
use crate::frag_reuse::{FragReuseIndex, FragReuseIndexHandle};
use crate::scalar::RowIdRemapper;
+use crate::vector::PART_ID_COLUMN;
use crate::vector::quantizer::QuantizerStorage;
-use crate::vector::storage::{DistCalculator, VectorStore};
+use crate::vector::storage::{
+ DistCalculator, VectorStore, covering_field_indices_excluding, remap_row_ids_by_name,
+};
use crate::vector::utils::do_prefetch;
use arrow::array::AsArray;
use arrow::compute::concat_batches;
@@ -85,7 +88,7 @@ impl QuantizerStorage for FlatFloatStorage {
frag_reuse_index: Option>,
) -> Result {
let batch = if let Some(frag_reuse_index_ref) = frag_reuse_index.as_ref() {
- frag_reuse_index_ref.remap_row_ids_record_batch(batch, 0)?
+ remap_row_ids_by_name(batch, frag_reuse_index_ref.as_ref())?
} else {
batch
};
@@ -194,6 +197,17 @@ impl VectorStore for FlatFloatStorage {
self.batch.schema_ref()
}
+ /// Flat storage is `[_rowid, flat, ]`, so the covering columns
+ /// are every field except the row id, the flat vector column, and the legacy
+ /// `__ivf_part_id` column (left in pre-#3606 single-partition builds; unlike
+ /// PQ, flat storage does not drop it at construction, so it is excluded here).
+ fn covering_field_indices(&self) -> Vec {
+ covering_field_indices_excluding(
+ self.schema().as_ref(),
+ &[ROW_ID, FLAT_COLUMN, PART_ID_COLUMN],
+ )
+ }
+
fn as_any(&self) -> &dyn std::any::Any {
self
}
@@ -265,7 +279,7 @@ impl QuantizerStorage for FlatBinStorage {
frag_reuse_index: Option>,
) -> Result {
let batch = if let Some(frag_reuse_index_ref) = frag_reuse_index.as_ref() {
- frag_reuse_index_ref.remap_row_ids_record_batch(batch, 0)?
+ remap_row_ids_by_name(batch, frag_reuse_index_ref.as_ref())?
} else {
batch
};
@@ -369,6 +383,17 @@ impl VectorStore for FlatBinStorage {
self.batch.schema_ref()
}
+ /// Flat storage is `[_rowid, flat, ]`, so the covering columns
+ /// are every field except the row id, the flat vector column, and the legacy
+ /// `__ivf_part_id` column (left in pre-#3606 single-partition builds; unlike
+ /// PQ, flat storage does not drop it at construction, so it is excluded here).
+ fn covering_field_indices(&self) -> Vec {
+ covering_field_indices_excluding(
+ self.schema().as_ref(),
+ &[ROW_ID, FLAT_COLUMN, PART_ID_COLUMN],
+ )
+ }
+
fn as_any(&self) -> &dyn std::any::Any {
self
}
@@ -685,6 +710,69 @@ mod tests {
FlatFloatStorage::new(vectors, DistanceType::L2)
}
+ // Pre-#3606 `num_partitions=1` builds left `__ivf_part_id` in the flat
+ // storage file. It must never be classified as a covering column --
+ // otherwise search emits a column the exec's declared schema (from
+ // `IndexMetadata.covering_fields`, empty for those indexes) lacks,
+ // desyncing the two and failing every query on that legacy index.
+ #[test]
+ fn test_flat_float_covering_excludes_legacy_part_id_column() {
+ use crate::vector::PART_ID_COLUMN;
+ const DIM: i32 = 4;
+ const N: usize = 8;
+ let row_ids = UInt64Array::from_iter_values(0..N as u64);
+ let values =
+ arrow_array::Float32Array::from_iter_values((0..N * DIM as usize).map(|v| v as f32));
+ let vectors = FixedSizeListArray::try_new_from_values(values, DIM).unwrap();
+ let part_ids = arrow_array::UInt32Array::from_iter_values((0..N).map(|_| 0));
+ let batch = RecordBatch::try_from_iter_with_nullable(vec![
+ (ROW_ID, Arc::new(row_ids) as ArrayRef, true),
+ (FLAT_COLUMN, Arc::new(vectors) as ArrayRef, true),
+ (PART_ID_COLUMN, Arc::new(part_ids) as ArrayRef, true),
+ ])
+ .unwrap();
+ let storage = FlatFloatStorage::try_from_batch(
+ batch,
+ &FlatMetadata { dim: DIM as usize },
+ DistanceType::L2,
+ None,
+ )
+ .unwrap();
+ assert!(
+ storage.covering_field_indices().is_empty(),
+ "legacy __ivf_part_id must not be classified as a covering column"
+ );
+ }
+
+ #[test]
+ fn test_flat_bin_covering_excludes_legacy_part_id_column() {
+ use crate::vector::PART_ID_COLUMN;
+ const DIM: i32 = 8;
+ const N: usize = 8;
+ let row_ids = UInt64Array::from_iter_values(0..N as u64);
+ let codes =
+ arrow_array::UInt8Array::from_iter_values((0..N * DIM as usize).map(|v| v as u8));
+ let vectors = FixedSizeListArray::try_new_from_values(codes, DIM).unwrap();
+ let part_ids = arrow_array::UInt32Array::from_iter_values((0..N).map(|_| 0));
+ let batch = RecordBatch::try_from_iter_with_nullable(vec![
+ (ROW_ID, Arc::new(row_ids) as ArrayRef, true),
+ (FLAT_COLUMN, Arc::new(vectors) as ArrayRef, true),
+ (PART_ID_COLUMN, Arc::new(part_ids) as ArrayRef, true),
+ ])
+ .unwrap();
+ let storage = FlatBinStorage::try_from_batch(
+ batch,
+ &FlatMetadata { dim: DIM as usize },
+ DistanceType::L2,
+ None,
+ )
+ .unwrap();
+ assert!(
+ storage.covering_field_indices().is_empty(),
+ "legacy __ivf_part_id must not be classified as a covering column"
+ );
+ }
+
#[test]
fn test_flat_float_storage_distance_f16() {
let storage = make_f16_storage();
diff --git a/rust/lance-index/src/vector/pq/storage.rs b/rust/lance-index/src/vector/pq/storage.rs
index a5ee3496571..0bcd9a3a5ef 100644
--- a/rust/lance-index/src/vector/pq/storage.rs
+++ b/rust/lance-index/src/vector/pq/storage.rs
@@ -14,7 +14,7 @@ use std::{
use arrow::datatypes::{self, UInt8Type};
use arrow_array::{ArrayRef, ArrowPrimitiveType, PrimitiveArray};
use arrow_array::{
- FixedSizeListArray, RecordBatch, UInt8Array, UInt64Array,
+ FixedSizeListArray, RecordBatch, UInt8Array, UInt32Array, UInt64Array,
cast::AsArray,
types::{Float32Type, UInt64Type},
};
@@ -43,10 +43,10 @@ use crate::vector::graph::{OrderedFloat, OrderedNode};
use crate::{
INDEX_METADATA_SCHEMA_KEY, IndexMetadata, pb,
vector::{
- PQ_CODE_COLUMN,
+ PART_ID_COLUMN, PQ_CODE_COLUMN,
pq::transform::PQTransformer,
quantizer::{QuantizerMetadata, QuantizerStorage},
- storage::{DistCalculator, VectorStore},
+ storage::{DistCalculator, VectorStore, covering_field_indices_excluding},
transform::Transformer,
},
};
@@ -230,16 +230,23 @@ impl ProductQuantizationStorage {
transposed: bool,
frag_reuse_index: Option>,
) -> Result {
- if batch.num_columns() != 2 {
- log::warn!(
- "PQ storage should have 2 columns, but got {} columns: {}",
- batch.num_columns(),
- batch.schema(),
- );
- batch = batch.project(&[
- batch.schema().index_of(ROW_ID)?,
- batch.schema().index_of(PQ_CODE_COLUMN)?,
- ])?;
+ // Require `_rowid` and the PQ code column, but preserve any additional
+ // ("included"/covering) columns row-aligned.
+ // Normalize the order to `[_rowid, __pq_code, ]` so the schema
+ // is stable across builds. `__ivf_part_id` is never a covering column:
+ // legacy `num_partitions=1` builds (pre-#3606) wrote it into the index
+ // file, so keep dropping it on load as before.
+ let row_id_idx = batch.schema().index_of(ROW_ID)?;
+ let pq_idx = batch.schema().index_of(PQ_CODE_COLUMN)?;
+ if row_id_idx != 0 || pq_idx != 1 || batch.num_columns() > 2 {
+ let schema = batch.schema();
+ let mut order = Vec::with_capacity(batch.num_columns());
+ order.push(row_id_idx);
+ order.push(pq_idx);
+ order.extend((0..batch.num_columns()).filter(|&i| {
+ i != row_id_idx && i != pq_idx && schema.field(i).name() != PART_ID_COLUMN
+ }));
+ batch = batch.project(&order)?;
}
let Some(row_ids) = batch.column_by_name(ROW_ID) else {
@@ -285,6 +292,7 @@ impl ProductQuantizationStorage {
let transposed_codes = pq_code.values();
let mut new_row_ids = Vec::with_capacity(row_ids.len());
let mut new_codes = Vec::with_capacity(row_ids.len() * num_sub_vectors);
+ let mut kept_indices: Vec = Vec::with_capacity(row_ids.len());
let row_ids_values = row_ids.values();
for (i, row_id) in row_ids_values.iter().enumerate() {
@@ -296,6 +304,7 @@ impl ProductQuantizationStorage {
num_sub_vectors,
i as u32,
));
+ kept_indices.push(i as u32);
}
}
@@ -311,7 +320,14 @@ impl ProductQuantizationStorage {
new_transposed_codes,
num_bytes_in_code as i32,
)?);
- RecordBatch::try_new(batch.schema(), vec![new_row_ids, codes_fsl])?
+ // Preserve any included/covering columns row-aligned to the kept rows.
+ rebuild_storage_batch(
+ batch.schema(),
+ &batch,
+ new_row_ids,
+ codes_fsl,
+ &UInt32Array::from(kept_indices),
+ )?
};
pq_code = batch[PQ_CODE_COLUMN]
.as_fixed_size_list()
@@ -529,6 +545,40 @@ where
transposed_codes.into()
}
+/// Rebuild a PQ-storage batch after a row selection (fragment-reuse remap in
+/// [`ProductQuantizationStorage::new`] or [`ProductQuantizationStorage::remap`]),
+/// preserving any extra ("included"/covering) columns beyond `_rowid` and the PQ
+/// code. The already-remapped `row_ids` and transposed `pq_codes` are supplied
+/// directly; every other column is gathered from `original` at the surviving
+/// positions `kept` so it stays row-aligned. `schema` carries the column order
+/// `[_rowid, __pq_code, ]`.
+fn rebuild_storage_batch(
+ schema: SchemaRef,
+ original: &RecordBatch,
+ row_ids: ArrayRef,
+ pq_codes: ArrayRef,
+ kept: &UInt32Array,
+) -> Result {
+ let columns = schema
+ .fields()
+ .iter()
+ .map(|field| {
+ let name = field.name().as_str();
+ if name == ROW_ID {
+ Ok(row_ids.clone())
+ } else if name == PQ_CODE_COLUMN {
+ Ok(pq_codes.clone())
+ } else {
+ let col = original.column_by_name(name).ok_or_else(|| {
+ Error::index(format!("column '{name}' missing from PQ storage batch"))
+ })?;
+ Ok(arrow::compute::take(col.as_ref(), kept, None)?)
+ }
+ })
+ .collect::>>()?;
+ Ok(RecordBatch::try_new(schema, columns)?)
+}
+
#[async_trait]
impl QuantizerStorage for ProductQuantizationStorage {
type Metadata = ProductQuantizationMetadata;
@@ -610,6 +660,7 @@ impl QuantizerStorage for ProductQuantizationStorage {
let transposed_codes = self.pq_code.values();
let mut new_row_ids = Vec::with_capacity(self.len());
let mut new_codes = Vec::with_capacity(self.len() * self.metadata.num_sub_vectors);
+ let mut kept_indices: Vec = Vec::with_capacity(self.len());
let row_ids = self.row_ids.values();
for (i, row_id) in row_ids.iter().enumerate() {
@@ -622,6 +673,7 @@ impl QuantizerStorage for ProductQuantizationStorage {
self.metadata.num_sub_vectors,
i as u32,
));
+ kept_indices.push(i as u32);
}
Some(None) => {}
None => {
@@ -632,6 +684,7 @@ impl QuantizerStorage for ProductQuantizationStorage {
self.metadata.num_sub_vectors,
i as u32,
));
+ kept_indices.push(i as u32);
}
}
}
@@ -647,7 +700,14 @@ impl QuantizerStorage for ProductQuantizationStorage {
new_transposed_codes,
num_bytes_in_code as i32,
)?);
- RecordBatch::try_new(self.schema(), vec![new_row_ids.clone(), codes_fsl])?
+ // Preserve any included/covering columns row-aligned to the kept rows.
+ rebuild_storage_batch(
+ self.schema(),
+ &self.batch,
+ new_row_ids.clone(),
+ codes_fsl,
+ &UInt32Array::from(kept_indices),
+ )?
};
let transposed_codes = batch[PQ_CODE_COLUMN]
.as_fixed_size_list()
@@ -722,6 +782,12 @@ impl VectorStore for ProductQuantizationStorage {
self.batch.schema_ref()
}
+ /// PQ storage is `[_rowid, __pq_code, ]`, so the covering
+ /// columns are every field except the row id and the PQ code column.
+ fn covering_field_indices(&self) -> Vec {
+ covering_field_indices_excluding(self.schema().as_ref(), &[ROW_ID, PQ_CODE_COLUMN])
+ }
+
fn as_any(&self) -> &dyn std::any::Any {
self
}
@@ -1290,6 +1356,132 @@ mod tests {
assert_eq!(storage.row_ids.len(), TOTAL);
}
+ #[tokio::test]
+ async fn test_build_preserves_extra_column() {
+ // A covering ("included") column beyond _rowid + __pq_code must be kept
+ // in the storage batch, row-aligned to the rows, not dropped.
+ let storage = create_pq_storage_with_extra_column().await;
+ let batch = storage.batch();
+ assert_eq!(batch.num_columns(), 3);
+ assert!(batch.column_by_name(ROW_ID).is_some());
+ assert!(batch.column_by_name(PQ_CODE_COLUMN).is_some());
+ let extra = batch
+ .column_by_name("extra")
+ .expect("extra column should be preserved")
+ .as_primitive::();
+ // In the fixture extra[i] == row_ids[i] == i, so it must stay aligned.
+ for (i, row_id) in storage.row_ids().enumerate() {
+ assert_eq!(extra.value(i) as u64, *row_id);
+ }
+ }
+
+ #[tokio::test]
+ async fn test_build_rejects_mismatched_batch_columns() {
+ // Batches merged into one storage must carry the same column set. A
+ // batch with a column the FIRST batch lacks must be rejected: aligning
+ // to the first batch's schema would silently drop it, losing covering
+ // data while the index metadata still advertises it.
+ let codebook = Float32Array::from_iter_values((0..256 * DIM).map(|_| rand::random()));
+ let codebook = FixedSizeListArray::try_new_from_values(codebook, DIM as i32).unwrap();
+ let pq = ProductQuantizer::new(NUM_SUB_VECTORS, 8, DIM, codebook, DistanceType::Dot);
+
+ let vec_field = Field::new(
+ "vec",
+ DataType::FixedSizeList(
+ Field::new_list_field(DataType::Float32, true).into(),
+ DIM as i32,
+ ),
+ true,
+ );
+ let make_fsl = || {
+ let vectors = Float32Array::from_iter_values((0..TOTAL * DIM).map(|_| rand::random()));
+ FixedSizeListArray::try_new_from_values(vectors, DIM as i32).unwrap()
+ };
+ let row_ids = || UInt64Array::from_iter_values((0..TOTAL).map(|v| v as u64));
+
+ let plain_schema = ArrowSchema::new(vec![vec_field.clone(), ROW_ID_FIELD.clone()]);
+ let plain = RecordBatch::try_new(
+ plain_schema.into(),
+ vec![Arc::new(make_fsl()), Arc::new(row_ids())],
+ )
+ .unwrap();
+
+ let covered_schema = ArrowSchema::new(vec![
+ vec_field,
+ ROW_ID_FIELD.clone(),
+ Field::new("extra", DataType::UInt32, true),
+ ]);
+ let covered = RecordBatch::try_new(
+ covered_schema.into(),
+ vec![
+ Arc::new(make_fsl()),
+ Arc::new(row_ids()),
+ Arc::new(UInt32Array::from_iter_values((0..TOTAL).map(|v| v as u32))),
+ ],
+ )
+ .unwrap();
+
+ let err = StorageBuilder::new("vec".to_owned(), pq.distance_type, pq, None)
+ .unwrap()
+ .build(vec![plain, covered])
+ .expect_err("a batch with extra columns must be rejected, not silently projected");
+ assert!(err.to_string().contains("extra"), "got: {err}");
+ }
+
+ #[tokio::test]
+ async fn test_new_drops_legacy_part_id_column() {
+ // Pre-#3606 `num_partitions=1` builds wrote `__ivf_part_id` into the
+ // index file. It must still be dropped on load (the legacy read shim),
+ // NOT preserved and misclassified as a covering column -- that would
+ // make search emit an extra column the exec's declared schema (from
+ // `IndexMetadata.covering_fields`, empty for those indexes) lacks.
+ use crate::vector::PART_ID_COLUMN;
+ let codebook = Float32Array::from_iter_values((0..256 * DIM).map(|_| rand::random()));
+ let codebook = FixedSizeListArray::try_new_from_values(codebook, DIM as i32).unwrap();
+
+ let schema = ArrowSchema::new(vec![
+ ROW_ID_FIELD.clone(),
+ Field::new(
+ PQ_CODE_COLUMN,
+ DataType::FixedSizeList(
+ Field::new_list_field(DataType::UInt8, true).into(),
+ NUM_SUB_VECTORS as i32,
+ ),
+ true,
+ ),
+ Field::new(PART_ID_COLUMN, DataType::UInt32, true),
+ ]);
+ let row_ids = UInt64Array::from_iter_values((0..TOTAL).map(|v| v as u64));
+ let codes = UInt8Array::from_iter_values((0..TOTAL * NUM_SUB_VECTORS).map(|v| v as u8));
+ let codes = FixedSizeListArray::try_new_from_values(codes, NUM_SUB_VECTORS as i32).unwrap();
+ let part_ids = UInt32Array::from_iter_values((0..TOTAL).map(|_| 0));
+ let batch = RecordBatch::try_new(
+ schema.into(),
+ vec![Arc::new(row_ids), Arc::new(codes), Arc::new(part_ids)],
+ )
+ .unwrap();
+
+ let storage = ProductQuantizationStorage::new(
+ codebook,
+ batch,
+ 8,
+ NUM_SUB_VECTORS,
+ DIM,
+ DistanceType::L2,
+ true, // codes already transposed
+ None,
+ )
+ .unwrap();
+ assert!(
+ storage.batch().column_by_name(PART_ID_COLUMN).is_none(),
+ "legacy __ivf_part_id must be dropped on load, not kept"
+ );
+ assert!(
+ storage.covering_field_indices().is_empty(),
+ "legacy __ivf_part_id must not be classified as a covering column"
+ );
+ }
+
#[tokio::test]
async fn test_distance_all() {
let storage = create_pq_storage().await;
@@ -1392,8 +1584,19 @@ mod tests {
// Rewritten row i lands at offset i of frag 1.
assert_eq!(*row_id, (1u64 << 32) | i as u64);
}
- assert_eq!(new_storage.batch.num_columns(), 2);
+ // The covering ("extra") column must survive remap, row-aligned to the
+ // surviving rows — otherwise PK/covering data is lost on compaction.
+ assert_eq!(new_storage.batch.num_columns(), 3);
assert!(new_storage.batch.column_by_name(ROW_ID).is_some());
assert!(new_storage.batch.column_by_name(PQ_CODE_COLUMN).is_some());
+ let extra = new_storage
+ .batch
+ .column_by_name("extra")
+ .expect("extra column should survive remap")
+ .as_primitive::();
+ // Surviving rows are original indices 0..TOTAL/2, whose extra values are 0..TOTAL/2.
+ for i in 0..TOTAL / 2 {
+ assert_eq!(extra.value(i), i as u32);
+ }
}
}
diff --git a/rust/lance-index/src/vector/quantizer.rs b/rust/lance-index/src/vector/quantizer.rs
index 433fa5031d4..c8b43636e24 100644
--- a/rust/lance-index/src/vector/quantizer.rs
+++ b/rust/lance-index/src/vector/quantizer.rs
@@ -290,7 +290,15 @@ pub trait QuantizerStorage: Clone + Sized + DeepSizeOf + VectorStore {
let mut indices = Vec::with_capacity(b.num_rows());
let mut new_row_ids = Vec::with_capacity(b.num_rows());
- let row_ids = b.column(0).as_primitive::().values();
+ // Look up by name, not position: a covering storage batch is
+ // `[_rowid, code, ]` for some quantizers but the
+ // covering columns can precede `_rowid` if the writer schema was
+ // inferred from a batch rather than declared, so position 0 is
+ // not reliably the row id column.
+ let row_id_col = b
+ .column_by_name(ROW_ID)
+ .ok_or_else(|| Error::index(format!("column {ROW_ID} not found in batch")))?;
+ let row_ids = row_id_col.as_primitive::().values();
for (i, row_id) in row_ids.iter().enumerate() {
match mapping.get(*row_id) {
Some(Some(new_id)) => {
diff --git a/rust/lance-index/src/vector/sq/storage.rs b/rust/lance-index/src/vector/sq/storage.rs
index 6062c22242e..c7b42c4c33c 100644
--- a/rust/lance-index/src/vector/sq/storage.rs
+++ b/rust/lance-index/src/vector/sq/storage.rs
@@ -30,9 +30,12 @@ use crate::scalar::RowIdRemapper;
use crate::{
INDEX_METADATA_SCHEMA_KEY, IndexMetadata,
vector::{
- SQ_CODE_COLUMN,
+ PART_ID_COLUMN, SQ_CODE_COLUMN,
quantizer::{QuantizerMetadata, QuantizerStorage},
- storage::{DistCalculator, DistanceCalculatorOptions, QueryResidual, VectorStore},
+ storage::{
+ DistCalculator, DistanceCalculatorOptions, QueryResidual, VectorStore,
+ covering_field_indices_excluding, remap_row_ids_by_name,
+ },
transform::Transformer,
},
};
@@ -193,7 +196,7 @@ impl ScalarQuantizationStorage {
offsets.push(0);
for mut batch in batches.into_iter() {
if let Some(frag_reuse_index_ref) = frag_reuse_index.as_ref() {
- batch = frag_reuse_index_ref.remap_row_ids_record_batch(batch, 0)?
+ batch = remap_row_ids_by_name(batch, frag_reuse_index_ref.as_ref())?
}
offsets.push(offsets.last().unwrap() + batch.num_rows() as u32);
let chunk = SQStorageChunk::new(batch)?;
@@ -370,6 +373,17 @@ impl VectorStore for ScalarQuantizationStorage {
self.chunks[0].schema()
}
+ /// SQ storage is `[_rowid, __sq_code, ]`, so the covering
+ /// columns are every field except the row id, the SQ code column, and the
+ /// legacy `__ivf_part_id` column (left in pre-#3606 single-partition builds;
+ /// unlike PQ, SQ does not drop it at construction, so it is excluded here).
+ fn covering_field_indices(&self) -> Vec {
+ covering_field_indices_excluding(
+ self.schema().as_ref(),
+ &[ROW_ID, SQ_CODE_COLUMN, PART_ID_COLUMN],
+ )
+ }
+
fn as_any(&self) -> &dyn std::any::Any {
self
}
@@ -785,6 +799,50 @@ mod tests {
RecordBatch::try_new(schema, vec![Arc::new(row_ids), Arc::new(code_arr)]).unwrap()
}
+ #[test]
+ fn test_covering_excludes_legacy_part_id_column() {
+ // Pre-#3606 `num_partitions=1` builds left `__ivf_part_id` in the SQ
+ // storage file. It must never be classified as a covering column --
+ // otherwise search emits a column the exec's declared schema (from
+ // `IndexMetadata.covering_fields`, empty for those indexes) lacks,
+ // desyncing the two and failing every query on that legacy index.
+ use crate::vector::PART_ID_COLUMN;
+ use arrow_array::UInt32Array;
+ const DIM: usize = 64;
+ const N: usize = 10;
+
+ let row_ids = UInt64Array::from_iter_values(0..N as u64);
+ let sq_code = UInt8Array::from_iter_values((0..N * DIM).map(|v| v as u8));
+ let code_arr = FixedSizeListArray::try_new_from_values(sq_code, DIM as i32).unwrap();
+ let part_ids = UInt32Array::from_iter_values((0..N).map(|_| 0));
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new(ROW_ID, DataType::UInt64, false),
+ Field::new(
+ SQ_CODE_COLUMN,
+ DataType::FixedSizeList(
+ Arc::new(Field::new("item", DataType::UInt8, true)),
+ DIM as i32,
+ ),
+ false,
+ ),
+ Field::new(PART_ID_COLUMN, DataType::UInt32, true),
+ ]));
+ let batch = RecordBatch::try_new(
+ schema,
+ vec![Arc::new(row_ids), Arc::new(code_arr), Arc::new(part_ids)],
+ )
+ .unwrap();
+
+ let storage =
+ ScalarQuantizationStorage::try_new(8, DistanceType::L2, -0.7..0.7, [batch], None)
+ .unwrap();
+ assert!(
+ storage.covering_field_indices().is_empty(),
+ "legacy __ivf_part_id must not be classified as a covering column"
+ );
+ }
+
#[test]
fn test_get_chunks() {
const DIM: usize = 64;
diff --git a/rust/lance-index/src/vector/storage.rs b/rust/lance-index/src/vector/storage.rs
index 151be945741..9dc843b9d6f 100644
--- a/rust/lance-index/src/vector/storage.rs
+++ b/rust/lance-index/src/vector/storage.rs
@@ -373,6 +373,41 @@ impl DeepSizeOf for QueryScratchPool {
///
/// It abstracts away the logic to compute the distance between vectors.
///
+/// Indices of the covering ("included") columns in a vector-storage schema: every
+/// field whose name is not one of `internal`. Each storage lists its own non-covering
+/// column names (the row id, its quantization code columns, and any legacy bookkeeping
+/// column it carries) and shares this filter, so covering detection stays a per-storage
+/// data decision rather than duplicated iteration logic. See
+/// [`VectorStore::covering_field_indices`] for why the set cannot be inferred generically.
+pub(crate) fn covering_field_indices_excluding(
+ schema: &arrow_schema::Schema,
+ internal: &[&str],
+) -> Vec {
+ schema
+ .fields()
+ .iter()
+ .enumerate()
+ .filter(|(_, f)| !internal.contains(&f.name().as_str()))
+ .map(|(i, _)| i)
+ .collect()
+}
+
+/// Remap `batch`'s row ids through `frag_reuse_index`, locating the row id column
+/// by name rather than assuming it is at a fixed position. A covering storage batch
+/// is built from a scan projection of `[, vector]` with `_rowid`
+/// appended by the scan, so `_rowid` lands wherever the covering columns end --
+/// never reliably at a fixed index once any are configured.
+pub(crate) fn remap_row_ids_by_name(
+ batch: RecordBatch,
+ frag_reuse_index: &dyn RowIdRemapper,
+) -> Result {
+ let row_id_idx = batch
+ .schema()
+ .index_of(ROW_ID)
+ .map_err(|_| Error::schema(format!("column {} not found", ROW_ID)))?;
+ frag_reuse_index.remap_row_ids_record_batch(batch, row_id_idx)
+}
+
/// TODO: should we rename this to "VectorDistance"?;
///
///
@@ -410,6 +445,45 @@ pub trait VectorStore: Send + Sync + Sized + Clone {
/// The storage implement will perform quantization if necessary.
fn append_batch(&self, batch: RecordBatch, vector_column: &str) -> Result;
+ /// Field indices of the "included"/covering columns: the extra columns
+ /// stored alongside the row id and quantization code so a covered query can
+ /// skip the take from the base table.
+ ///
+ /// The default is empty (no covering). A storage that supports covering must
+ /// override this, since only it knows which of its columns are quantization
+ /// code columns versus included columns — inferring that generically by name
+ /// is unsafe (e.g. it would mistake RaBitQ's code columns for covering ones).
+ fn covering_field_indices(&self) -> Vec {
+ Vec::new()
+ }
+
+ /// `[_rowid, ]` for the whole storage. Captured while a
+ /// partition is loaded during search so covering columns can be emitted with
+ /// the result without a separate take. Returns `None` if there are no
+ /// included columns (ordinary index — nothing to cover).
+ fn covering_batch(&self) -> Result> {
+ let included = self.covering_field_indices();
+ if included.is_empty() {
+ return Ok(None);
+ }
+ let schema = self.schema().clone();
+ let row_id_idx = schema.index_of(ROW_ID)?;
+ let mut indices = Vec::with_capacity(included.len() + 1);
+ indices.push(row_id_idx);
+ indices.extend(included);
+ // Project each batch BEFORE concatenating. Concatenating the full partition first
+ // would copy every quantization-code column only to discard it on the next line --
+ // and those columns dominate the partition (HNSW partitions target 1 << 20 rows), so
+ // on a multi-probe query that copy is hundreds of MB of pure waste on the ANN hot
+ // path. Only `[_rowid, ]` is ever read from the result.
+ let projected: Vec = self
+ .to_batches()?
+ .map(|batch| batch.project(&indices))
+ .collect::>()?;
+ let projected_schema = Arc::new(schema.project(&indices)?);
+ Ok(Some(concat_batches(&projected_schema, projected.iter())?))
+ }
+
/// Create a [DistCalculator] to compute the distance between the query.
///
/// Using dist calculator can be more efficient as it can pre-compute some
@@ -480,7 +554,43 @@ impl StorageBuilder {
}
pub fn build(&self, batches: Vec) -> Result {
- let mut batch = concat_batches(batches[0].schema_ref(), batches.iter())?;
+ // Batches can come from different sources (existing partitions loaded
+ // from disk vs freshly shuffled/split/reassigned data) that carry the
+ // same columns in a different order -- notably when the index has
+ // included/covering columns. `concat_batches` matches columns by
+ // position, so align every batch to the first batch's column order (by
+ // name). No-op when all batches already share a schema.
+ let schema = batches[0].schema();
+ let aligned: Vec = batches
+ .iter()
+ .map(|b| {
+ if b.schema() == schema {
+ return Ok(b.clone());
+ }
+ // `project_by_schema` reorders by name and errors on a missing
+ // column, but it cannot see extras -- and a batch with MORE
+ // columns must error rather than have them silently dropped
+ // (e.g. covering columns the index metadata still advertises).
+ // Equal count + every expected column present = same set.
+ if b.num_columns() != schema.fields().len() {
+ let names = |s: &arrow_schema::Schema| {
+ s.fields()
+ .iter()
+ .map(|f| f.name().as_str())
+ .collect::>()
+ .join(", ")
+ };
+ return Err(Error::index(format!(
+ "mismatched columns while merging vector storage batches: \
+ expected [{}], got [{}]",
+ names(&schema),
+ names(&b.schema()),
+ )));
+ }
+ Ok(b.project_by_schema(schema.as_ref())?)
+ })
+ .collect::>>()?;
+ let mut batch = concat_batches(&schema, aligned.iter())?;
if batch.column_by_name(self.quantizer.column()).is_none() {
let vectors = batch
@@ -518,6 +628,13 @@ pub struct IvfQuantizationStorage {
ivf: IvfModel,
frag_reuse_index: Option>,
+ /// Lazily-computed covering schema (see [`Self::covering_schema`]). The schema is
+ /// fixed for the storage's lifetime and is consulted on every search, so it is
+ /// resolved once rather than per query. Constructing it builds an empty `Q::Storage`:
+ /// cheap for a current-format index (zero rows, and the codebook is cloned, not
+ /// decoded), but the legacy `codebook_tensor` path does decode, so this is not free
+ /// for every index.
+ covering_schema: std::sync::OnceLock>,
}
impl DeepSizeOf for IvfQuantizationStorage {
@@ -589,6 +706,7 @@ impl IvfQuantizationStorage {
metadata,
ivf,
frag_reuse_index,
+ covering_schema: std::sync::OnceLock::new(),
})
}
@@ -620,6 +738,7 @@ impl IvfQuantizationStorage {
metadata,
ivf,
frag_reuse_index,
+ covering_schema: std::sync::OnceLock::new(),
}
}
@@ -656,6 +775,31 @@ impl IvfQuantizationStorage {
Arc::new(self.reader.schema().as_ref().into())
}
+ /// The `[_rowid, ]` schema for this index's covering
+ /// columns, or `None` if the index has no covering columns. Derived from an
+ /// empty storage instance (no I/O) so callers can emit the correct covered
+ /// output schema even when zero partitions are searched. Cached because it is queried
+ /// per search. The cache is best-effort under concurrency: racing cold callers may each
+ /// construct one and all but the first are discarded. That is deliberate -- the empty
+ /// construction is cheap on the current format, and `OnceLock::get_or_init` cannot
+ /// carry the `Result` this returns.
+ pub fn covering_schema(&self) -> Result> {
+ if let Some(cached) = self.covering_schema.get() {
+ return Ok(cached.clone());
+ }
+ let arrow_schema = arrow_schema::Schema::from(self.reader.schema().as_ref());
+ let empty = RecordBatch::new_empty(Arc::new(arrow_schema));
+ // No remapper: the batch is empty, so there are no row ids to remap and the
+ // remapper cannot influence the schema this derives. Passing one would also be
+ // wrong now that the field holds a `dyn RowIdRemapper` -- the default
+ // `try_from_batch_with_remapper` rejects a remapper outright for every storage
+ // that does not override it, which would fail this call on an SQ/RQ/FLAT index
+ // whose dataset happens to carry a fragment-reuse index.
+ let storage = Q::Storage::try_from_batch(empty, self.metadata(), self.distance_type, None)?;
+ let computed = storage.covering_batch()?.map(|b| b.schema());
+ Ok(self.covering_schema.get_or_init(|| computed).clone())
+ }
+
/// Get the number of partitions in the storage.
pub fn num_partitions(&self) -> usize {
self.ivf.num_partitions()
diff --git a/rust/lance-index/src/vector/transform.rs b/rust/lance-index/src/vector/transform.rs
index 2faae7b9ce6..1ad89afd237 100644
--- a/rust/lance-index/src/vector/transform.rs
+++ b/rust/lance-index/src/vector/transform.rs
@@ -7,18 +7,18 @@
use std::fmt::Debug;
use std::sync::Arc;
-use arrow::datatypes::UInt64Type;
-use arrow_array::UInt64Array;
use arrow_array::types::{Float16Type, Float32Type, Float64Type};
use arrow_array::{Array, ArrowPrimitiveType, RecordBatch, UInt32Array, cast::AsArray};
use arrow_schema::{DataType, Field, Schema};
use lance_arrow::RecordBatchExt;
use num_traits::Float;
-use lance_core::{Error, ROW_ID, ROW_ID_FIELD, Result};
+use lance_core::{Error, Result};
use lance_linalg::kernels::normalize_fsl;
use tracing::instrument;
+use super::{CENTROID_DIST_COLUMN, PART_ID_COLUMN};
+
/// Transform of a Vector Matrix.
///
///
@@ -194,25 +194,48 @@ impl Transformer for Flatten {
match arr.data_type() {
DataType::FixedSizeList(_, _) => Ok(batch.clone()),
DataType::List(_) => {
- let row_ids = batch[ROW_ID].as_primitive::();
let vectors = arr.as_list::();
-
- let row_ids = row_ids.values().iter().zip(vectors.iter()).flat_map(
- |(row_id, multivector)| {
- std::iter::repeat_n(
- *row_id,
- multivector.map(|multivec| multivec.len()).unwrap_or(0),
- )
- },
- );
- let row_ids = UInt64Array::from_iter_values(row_ids);
- let vectors = vectors.values().as_fixed_size_list().clone();
- let schema = Arc::new(Schema::new(vec![
- ROW_ID_FIELD.clone(),
- Field::new(self.column.as_str(), vectors.data_type().clone(), true),
- ]));
- let batch =
- RecordBatch::try_new(schema, vec![Arc::new(row_ids), Arc::new(vectors)])?;
+ // Each source row expands into one output row per vector in its list.
+ // Replicate the row id AND every other column (e.g. covering / "included"
+ // columns) across the expansion via a gather; dropping them here would
+ // silently lose covering data on the multivector build/split path.
+ //
+ // The IVF partition-assignment columns are the exception and must be
+ // DROPPED, not replicated: they describe the source *row*, and after the
+ // expansion each sub-vector needs its own nearest centroid. Carrying them
+ // through makes `PartitionTransformer` see its output column already
+ // present and early-return, silently collapsing every sub-vector of a row
+ // into that row's single partition -- no error, just degraded recall.
+ let take_indices =
+ UInt32Array::from_iter_values((0..vectors.len()).flat_map(|i| {
+ let n = if vectors.is_valid(i) {
+ vectors.value(i).len()
+ } else {
+ 0
+ };
+ std::iter::repeat_n(i as u32, n)
+ }));
+ let flat_vectors = vectors.values().as_fixed_size_list().clone();
+ let mut fields: Vec> = Vec::with_capacity(batch.num_columns());
+ let mut columns: Vec =
+ Vec::with_capacity(batch.num_columns());
+ for (i, field) in batch.schema().fields().iter().enumerate() {
+ if field.name() == &self.column {
+ fields.push(Arc::new(Field::new(
+ self.column.as_str(),
+ flat_vectors.data_type().clone(),
+ true,
+ )));
+ columns.push(Arc::new(flat_vectors.clone()));
+ } else if field.name() == PART_ID_COLUMN || field.name() == CENTROID_DIST_COLUMN
+ {
+ continue;
+ } else {
+ fields.push(field.clone());
+ columns.push(arrow::compute::take(batch.column(i), &take_indices, None)?);
+ }
+ }
+ let batch = RecordBatch::try_new(Arc::new(Schema::new(fields)), columns)?;
Ok(batch)
}
_ => Err(Error::index(format!(
@@ -236,6 +259,114 @@ mod tests {
use lance_arrow::*;
use lance_linalg::distance::L2;
+ #[test]
+ fn test_flatten_preserves_extra_columns_for_multivector() {
+ use arrow::buffer::OffsetBuffer;
+ use arrow::datatypes::UInt64Type;
+ use arrow_array::{ListArray, UInt64Array};
+ use lance_core::{ROW_ID, ROW_ID_FIELD};
+
+ // row 10 has 2 vectors, row 20 has 1 vector; a covering column "meta" must be
+ // replicated across each row's expanded vectors, not dropped.
+ let values = Float32Array::from(vec![1.0, 1.0, 2.0, 2.0, 3.0, 3.0]);
+ let fsl = FixedSizeListArray::try_new_from_values(values, 2).unwrap();
+ let offsets = OffsetBuffer::new(vec![0i32, 2, 3].into());
+ let item = Arc::new(Field::new("item", fsl.data_type().clone(), true));
+ let list = ListArray::new(item, offsets, Arc::new(fsl), None);
+
+ let batch = RecordBatch::try_new(
+ Arc::new(Schema::new(vec![
+ ROW_ID_FIELD.clone(),
+ Field::new("vector", list.data_type().clone(), true),
+ Field::new("meta", DataType::Int32, true),
+ ])),
+ vec![
+ Arc::new(UInt64Array::from(vec![10u64, 20])),
+ Arc::new(list),
+ Arc::new(Int32Array::from(vec![100, 200])),
+ ],
+ )
+ .unwrap();
+
+ let out = Flatten::new("vector").transform(&batch).unwrap();
+
+ assert_eq!(out.num_rows(), 3, "2 + 1 vectors expand to 3 rows");
+ assert_eq!(
+ out[ROW_ID].as_primitive::().values(),
+ &[10, 10, 20]
+ );
+ let meta = out
+ .column_by_name("meta")
+ .expect("covering column must survive Flatten")
+ .as_primitive::();
+ assert_eq!(
+ meta.values(),
+ &[100, 100, 200],
+ "covering column replicated per expanded vector"
+ );
+ assert!(matches!(
+ out.column_by_name("vector").unwrap().data_type(),
+ DataType::FixedSizeList(_, 2)
+ ));
+ }
+
+ /// The IVF partition-assignment columns describe the source *row*. Replicating them
+ /// across the multivector expansion makes `PartitionTransformer` see its output column
+ /// already present and early-return, so every sub-vector inherits its row's single
+ /// partition instead of being assigned to its own nearest centroid -- silent recall
+ /// loss with no error. They must be dropped even though covering columns are kept.
+ #[test]
+ fn test_flatten_drops_partition_columns_for_multivector() {
+ use arrow::buffer::OffsetBuffer;
+ use arrow_array::{ListArray, UInt32Array as U32, UInt64Array};
+ use lance_core::ROW_ID_FIELD;
+
+ let values = Float32Array::from(vec![1.0, 1.0, 2.0, 2.0, 3.0, 3.0]);
+ let fsl = FixedSizeListArray::try_new_from_values(values, 2).unwrap();
+ let offsets = OffsetBuffer::new(vec![0i32, 2, 3].into());
+ let item = Arc::new(Field::new("item", fsl.data_type().clone(), true));
+ let list = ListArray::new(item, offsets, Arc::new(fsl), None);
+
+ let batch = RecordBatch::try_new(
+ Arc::new(Schema::new(vec![
+ ROW_ID_FIELD.clone(),
+ Field::new("vector", list.data_type().clone(), true),
+ Field::new("meta", DataType::Int32, true),
+ Field::new(PART_ID_COLUMN, DataType::UInt32, true),
+ Field::new(CENTROID_DIST_COLUMN, DataType::Float32, true),
+ ])),
+ vec![
+ Arc::new(UInt64Array::from(vec![10u64, 20])),
+ Arc::new(list),
+ Arc::new(Int32Array::from(vec![100, 200])),
+ Arc::new(U32::from(vec![7u32, 9])),
+ Arc::new(Float32Array::from(vec![0.5f32, 0.25])),
+ ],
+ )
+ .unwrap();
+
+ let out = Flatten::new("vector").transform(&batch).unwrap();
+
+ assert_eq!(out.num_rows(), 3);
+ assert!(
+ out.column_by_name(PART_ID_COLUMN).is_none(),
+ "a row's partition assignment must not survive the multivector expansion, or \
+ PartitionTransformer early-returns and every sub-vector shares one partition"
+ );
+ assert!(
+ out.column_by_name(CENTROID_DIST_COLUMN).is_none(),
+ "the row's centroid distance is likewise meaningless per sub-vector"
+ );
+ // ...while genuine covering columns are still replicated.
+ assert_eq!(
+ out.column_by_name("meta")
+ .expect("covering column must survive")
+ .as_primitive::()
+ .values(),
+ &[100, 100, 200]
+ );
+ }
+
#[tokio::test]
async fn test_normalize_transformer_f32() {
let data = Float32Array::from_iter_values([1.0, 1.0, 2.0, 2.0].into_iter());
diff --git a/rust/lance-table/src/format/overlay/staleness.rs b/rust/lance-table/src/format/overlay/staleness.rs
index a6eebe61254..4e096b25ce4 100644
--- a/rust/lance-table/src/format/overlay/staleness.rs
+++ b/rust/lance-table/src/format/overlay/staleness.rs
@@ -108,6 +108,12 @@ pub fn overlaid_fragments(fragments: &[Fragment]) -> HashMap {
/// stale because an overlay committed after the segment was built touches a field the segment
/// indexes. Field-aware and version-gated via [`overlay_exclusion_offsets`].
///
+/// Walking `segment.fields` alone covers both the key fields and any covering
+/// (`covering_fields`) columns materialized in the index storage: `fields` always lists
+/// `covering_fields` as its trailing entries (see [`IndexMetadata::covering_fields`]). An
+/// overlay touching either makes the segment's stored copy stale, so both must gate overlay
+/// exclusion.
+///
/// `overlaid_frags` holds only the fragments that actually carry overlays (rare), so the loop is
/// `O(overlaid_frags)` rather than `O(fragments the segment covers)`.
pub fn collect_overlay_stale_frags(
@@ -412,4 +418,48 @@ mod tests {
"fragment absent from bitmap contributes no rows"
);
}
+
+ #[test]
+ fn test_collect_rows_flags_overlay_on_covered_column() {
+ let schema = flat_test_schema();
+ // Segment indexes field 3 (key) and COVERS field 2 (included). An overlay touching
+ // ONLY the covered field must still flag its rows stale -- otherwise a covered query
+ // serves the stale index-stored copy of that column instead of the fresh overlay value.
+ let fragment = fragment_with_overlay(3, dense_overlay(vec![2], [1, 2], 9));
+ let overlaid: HashMap = HashMap::from([(3u32, &fragment)]);
+ // `covering_fields` is always the trailing entries of `fields`.
+ let seg = IndexMetadata {
+ covering_fields: vec![2],
+ ..segment(vec![3, 2], 1, Some(bitmap([3])))
+ };
+
+ let mut stale = HashMap::new();
+ collect_overlay_stale_rows_for_segment(&seg, &overlaid, &mut stale, &schema).unwrap();
+ assert_eq!(
+ stale.get(&3),
+ Some(&bitmap([1, 2])),
+ "overlay on a covered (included) column must flag its rows stale"
+ );
+ }
+
+ #[test]
+ fn test_collect_frags_flags_overlay_on_covered_column() {
+ let schema = flat_test_schema();
+ // Same at fragment granularity: an overlay on a covered column marks the fragment stale.
+ let fragment = fragment_with_overlay(3, dense_overlay(vec![2], [1, 2], 9));
+ let overlaid: HashMap = HashMap::from([(3u32, &fragment)]);
+ // `covering_fields` is always the trailing entries of `fields`.
+ let seg = IndexMetadata {
+ covering_fields: vec![2],
+ ..segment(vec![3, 2], 1, Some(bitmap([3])))
+ };
+
+ let mut stale = RoaringBitmap::new();
+ collect_overlay_stale_frags(&seg, &overlaid, &mut stale, &schema).unwrap();
+ assert_eq!(
+ stale,
+ bitmap([3]),
+ "overlay on a covered (included) column must flag the fragment stale"
+ );
+ }
}
diff --git a/rust/lance-table/src/system_index/frag_reuse.rs b/rust/lance-table/src/system_index/frag_reuse.rs
index 401cbde7a87..20b0f99fbed 100644
--- a/rust/lance-table/src/system_index/frag_reuse.rs
+++ b/rust/lance-table/src/system_index/frag_reuse.rs
@@ -397,11 +397,11 @@ impl CompactFragReuseIndex {
RoaringTreemap::from_iter(row_ids.iter().filter_map(|addr| self.remap_row_id(addr)))
}
- /// Remap a record batch that contains a row_id column at index `row_id_idx`
- /// Currently this assumes there are only 2 columns in the schema,
- /// which is the case for all indexes.
- /// For example, for btree, the schema is (value, row_id).
- /// For vector index storage, the schema is (row_id, vector).
+ /// Remap a record batch that contains a row_id column at index `row_id_idx`.
+ /// Every other column (there may be one, as for scalar indexes -- `(value,
+ /// row_id)` -- or several, as for a covered vector index's storage --
+ /// `(row_id, code, )`) is row-aligned to the surviving,
+ /// remapped row ids by the same take.
pub fn remap_row_ids_record_batch(
&self,
batch: RecordBatch,
@@ -426,8 +426,9 @@ fn remap_row_ids_record_batch(
row_id_idx: usize,
remap: impl FnOnce(&mut [Option]),
) -> Result {
- assert_eq!(batch.schema().fields().len(), 2);
- let other_column_idx = 1 - row_id_idx;
+ // Every column but `_rowid` is carried through row-aligned, however many there are.
+ // A covered index's storage batch holds its covering ("included") columns next to the
+ // codes, so this cannot assume the two-column `[_rowid, ]` shape.
let row_ids = batch.column(row_id_idx).as_primitive::();
let mut remapped_row_ids = row_ids
.values()
@@ -436,26 +437,27 @@ fn remap_row_ids_record_batch(
.map(Some)
.collect::>();
remap(&mut remapped_row_ids);
- let (val_indices, new_row_ids): (Vec, Vec) = remapped_row_ids
+ let (keep_indices, new_row_ids): (Vec, Vec) = remapped_row_ids
.iter()
.enumerate()
.filter_map(|(idx, new_id)| new_id.map(|new_id| (idx as u64, new_id)))
.unzip();
- let new_val_indices = UInt64Array::from_iter_values(val_indices);
- let new_vals = arrow::compute::take(batch.column(other_column_idx), &new_val_indices, None)?;
-
- let mut batch_data: Vec<(usize, ArrayRef)> = vec![
- (
- row_id_idx,
- Arc::new(UInt64Array::from_iter_values(new_row_ids)) as ArrayRef,
- ),
- (other_column_idx, Arc::new(new_vals)),
- ];
- batch_data.sort_by_key(|(i, _)| *i);
- Ok(RecordBatch::try_new(
- batch.schema(),
- batch_data.into_iter().map(|(_, item)| item).collect(),
- )?)
+ let keep_indices = UInt64Array::from_iter_values(keep_indices);
+ let new_row_ids: ArrayRef = Arc::new(UInt64Array::from_iter_values(new_row_ids));
+
+ let columns = batch
+ .columns()
+ .iter()
+ .enumerate()
+ .map(|(idx, column)| {
+ if idx == row_id_idx {
+ Ok(new_row_ids.clone())
+ } else {
+ Ok(arrow::compute::take(column, &keep_indices, None)?)
+ }
+ })
+ .collect::>>()?;
+ Ok(RecordBatch::try_new(batch.schema(), columns)?)
}
fn remap_row_ids_array(
diff --git a/rust/lance-table/src/transaction/index_maintenance.rs b/rust/lance-table/src/transaction/index_maintenance.rs
index 06cf9648a88..f96833fe8a3 100644
--- a/rust/lance-table/src/transaction/index_maintenance.rs
+++ b/rust/lance-table/src/transaction/index_maintenance.rs
@@ -19,6 +19,18 @@ use lance_core::{Error, Result};
use roaring::RoaringBitmap;
use std::collections::{HashMap, HashSet};
+/// Insert `field`'s id and every descendant's id into `field_ids`.
+///
+/// A private copy of `lance::index::collect_subtree_field_ids`: the walk is four
+/// lines and the alternative is exporting a lance-table helper solely so the lance
+/// crate's copy could delegate to it.
+fn collect_subtree_field_ids(field: &lance_core::datatypes::Field, field_ids: &mut HashSet) {
+ field_ids.insert(field.id);
+ for child in &field.children {
+ collect_subtree_field_ids(child, field_ids);
+ }
+}
+
impl Transaction {
pub(super) fn register_pure_rewrite_rows_update_frags_in_indices(
indices: &mut [IndexMetadata],
@@ -32,6 +44,13 @@ impl Transaction {
return Ok(());
}
+ // Read the parameter name as the inverse of what it does. Despite
+ // "for_preserving_frag_bitmap", this list is the set of fields whose VALUES were
+ // updated by the rewrite, and an index depending on any of them is *excluded* from
+ // the bitmap extension below -- i.e. passing a field here withholds preservation
+ // rather than granting it. A caller that cannot prove any field survived the rewrite
+ // unchanged therefore passes *every* field id, which correctly extends no bitmap at
+ // all. The name is kept only because renaming it would churn shared callers.
let value_updated_field_set = fields_for_preserving_frag_bitmap
.iter()
.collect::>();
@@ -42,9 +61,21 @@ impl Transaction {
if index.results_are_row_addrs() {
continue;
}
- let index_covers_modified_field = index.fields.iter().any(|field_id| {
- value_updated_field_set.contains(&u32::try_from(*field_id).unwrap())
- });
+ // Covering (`covering_fields`) values are materialized in the index
+ // storage just like the indexed `fields`, so a rewrite of either
+ // makes this fragment's entries stale. Reusing them would serve the
+ // pre-update value, so treat both as "modified field" here.
+ //
+ // Expand to the leaf subtree, exactly as every sibling prune does:
+ // `covering_fields` can only ever hold TOP-LEVEL ids (index creation
+ // resolves whole columns and rejects dotted names), while the caller
+ // may report the individual leaves it rewrote. Comparing raw ids would
+ // miss a covered struct whose child was rewritten and wrongly extend
+ // this index's bitmap onto the moved fragment, serving the pre-update
+ // subfield value from index storage.
+ let index_covers_modified_field = Self::index_dependent_field_ids(index, schema)
+ .iter()
+ .any(|field_id| value_updated_field_set.contains(field_id));
if index_covers_modified_field {
continue;
}
@@ -86,27 +117,92 @@ impl Transaction {
Ok(())
}
- /// If an operation modifies one or more fields in a fragment then we need to remove
- /// that fragment from any indices that cover one of the modified fields.
+ /// The full set of leaf field ids an index depends on: its indexed key fields plus
+ /// every covered (included) field, each expanded to its whole subtree. `fields`
+ /// already lists `covering_fields` as its trailing entries (see
+ /// [`IndexMetadata::covering_fields`]), so walking `fields` alone covers both; a
+ /// modified/overlaid data file lists leaf ids, so a covered struct's parent id must
+ /// still be expanded to recognize a change to one of its subfields. Returned as raw
+ /// `i32` (the field id space of `DataFile.fields` / overlay fields). Shared by the
+ /// freshness prunes and the conflict-rebase checks.
+ pub fn index_dependent_leaf_ids(index: &IndexMetadata, schema: &Schema) -> HashSet {
+ let mut ids: HashSet = HashSet::new();
+ for &id in index.fields.iter() {
+ match schema.field_by_id(id) {
+ Some(field) => collect_subtree_field_ids(field, &mut ids),
+ None => {
+ ids.insert(id);
+ }
+ }
+ }
+ ids
+ }
+
+ fn index_dependent_field_ids(index: &IndexMetadata, schema: &Schema) -> HashSet {
+ Self::index_dependent_leaf_ids(index, schema)
+ .into_iter()
+ .filter_map(|id| u32::try_from(id).ok())
+ .collect()
+ }
+
+ /// Drop `updated_fragments` from the coverage of any index that depends on a field
+ /// listed in `fields_modified`. Covered struct fields are expanded to their leaf
+ /// subtree: `covering_fields` records the parent struct id while a modified data
+ /// file lists leaf ids, so exact-id matching would miss an update to a covered
+ /// struct's subfield and serve its stale value from the index. Every caller must
+ /// therefore supply the schema (the conflict-rebase path captures it at the read
+ /// version).
pub fn prune_updated_fields_from_indices(
indices: &mut [IndexMetadata],
updated_fragments: &[Fragment],
fields_modified: &[u32],
+ schema: &Schema,
+ ) {
+ if fields_modified.is_empty() {
+ return;
+ }
+
+ let deps = Self::index_dependent_field_ids_for_each(indices, schema);
+ Self::prune_updated_fields_with_deps(indices, updated_fragments, fields_modified, &deps);
+ }
+
+ /// The dependent-leaf-id set for each index, positionally parallel to `indices`.
+ ///
+ /// Resolving these walks the schema and allocates per index, so callers that prune
+ /// fragment-by-fragment must compute this once and reuse it rather than paying it per
+ /// fragment -- on a compaction rewriting N fragments across M indices the per-fragment
+ /// form is N*M schema walks on the commit path.
+ fn index_dependent_field_ids_for_each(
+ indices: &[IndexMetadata],
+ schema: &Schema,
+ ) -> Vec> {
+ indices
+ .iter()
+ .map(|index| Self::index_dependent_field_ids(index, schema))
+ .collect()
+ }
+
+ /// [`Self::prune_updated_fields_from_indices`] with the per-index dependent-id sets
+ /// already resolved. `deps` must be positionally parallel to `indices`.
+ fn prune_updated_fields_with_deps(
+ indices: &mut [IndexMetadata],
+ updated_fragments: &[Fragment],
+ fields_modified: &[u32],
+ deps: &[HashSet],
) {
if fields_modified.is_empty() {
return;
}
+ debug_assert_eq!(indices.len(), deps.len());
// If we modified any fields in the fragments then we need to remove those fragments
// from the index if the index covers one of those modified fields.
- let fields_modified_set = fields_modified.iter().collect::>();
- for index in indices.iter_mut() {
- if index
- .fields
+ let fields_modified_set = fields_modified.iter().copied().collect::>();
+ for (index, dependent_ids) in indices.iter_mut().zip(deps) {
+ let touches_index = dependent_ids
.iter()
- .any(|field_id| fields_modified_set.contains(&u32::try_from(*field_id).unwrap()))
- && let Some(fragment_bitmap) = &mut index.fragment_bitmap
- {
+ .any(|field_id| fields_modified_set.contains(field_id));
+ if touches_index && let Some(fragment_bitmap) = &mut index.fragment_bitmap {
for fragment_id in updated_fragments.iter().map(|f| f.id as u32) {
fragment_bitmap.remove(fragment_id);
}
@@ -116,7 +212,9 @@ impl Transaction {
/// Map each (non-tombstoned) field id in a fragment to the path of the data
/// file that backs it.
- fn fragment_field_paths(frag: &Fragment) -> HashMap {
+ // `pub`: the conflict-rebase path in the `lance` crate resolves per-fragment
+ // field ownership with this too.
+ pub fn fragment_field_paths(frag: &Fragment) -> HashMap {
let mut map = HashMap::new();
for file in &frag.files {
for &field_id in file.fields.iter() {
@@ -138,9 +236,13 @@ impl Transaction {
indices: &mut [IndexMetadata],
prev_fragments: &[Fragment],
new_fragments: &[Fragment],
+ schema: &Schema,
) {
let prev_by_id: HashMap =
prev_fragments.iter().map(|f| (f.id, f)).collect();
+ // Resolved once for all fragments: this is the commit path, and the per-fragment
+ // form costs a schema walk and two HashSet allocations per index per fragment.
+ let deps = Self::index_dependent_field_ids_for_each(indices, schema);
for new_frag in new_fragments {
let Some(prev) = prev_by_id.get(&new_frag.id) else {
continue; // brand-new fragment: nothing stale to prune
@@ -160,10 +262,11 @@ impl Transaction {
if changed.is_empty() {
continue;
}
- Self::prune_updated_fields_from_indices(
+ Self::prune_updated_fields_with_deps(
indices,
std::slice::from_ref(new_frag),
&changed,
+ &deps,
);
}
}
@@ -178,17 +281,22 @@ impl Transaction {
pub(super) fn prune_overlay_stale_fields_from_indices(
indices: &mut [IndexMetadata],
groups: &[RewriteGroup],
+ schema: &Schema,
) {
+ // Resolved once for all groups, as in `prune_merge_rewritten_fields_from_indices`:
+ // this is the commit path, and the per-index form costs a schema walk and two
+ // HashSet allocations for every (group, index) pair.
+ let deps = Self::index_dependent_field_ids_for_each(indices, schema);
for group in groups {
// field id -> newest overlay committed_version supplying that field
- let mut overlaid_field_versions: HashMap = HashMap::new();
+ let mut overlaid_field_versions: HashMap = HashMap::new();
for old_frag in &group.old_fragments {
for overlay in &old_frag.overlays {
for &field_id in overlay.data_file.fields.iter() {
- if field_id < 0 {
- // Tombstoned (obsolete) overlay field: supplies nothing.
+ // Tombstoned (obsolete) overlay fields (< 0) supply nothing.
+ let Ok(field_id) = u32::try_from(field_id) else {
continue;
- }
+ };
let entry = overlaid_field_versions.entry(field_id).or_insert(0);
*entry = (*entry).max(overlay.committed_version);
}
@@ -203,8 +311,12 @@ impl Transaction {
.iter()
.map(|f| f.id as u32)
.collect::>();
- for index in indices.iter_mut() {
- let is_stale = index.fields.iter().any(|field_id| {
+ for (index, dependent_field_ids) in indices.iter_mut().zip(deps.iter()) {
+ // A covered (included) field an overlay supplied makes the index just as
+ // stale as an indexed field would -- the index storage still holds the
+ // pre-overlay copy of that column. `deps` expands covered structs to their
+ // leaf subtree, since the overlay lists leaf ids.
+ let is_stale = dependent_field_ids.iter().any(|field_id| {
overlaid_field_versions
.get(field_id)
.is_some_and(|&overlay_version| overlay_version > index.dataset_version)
@@ -218,6 +330,87 @@ impl Transaction {
}
}
+ /// Reject a commit that would drop, rename, retype, or otherwise change the subtree of
+ /// a field an index *covers* (an "included" column). A covered field's physical payload
+ /// schema is fixed at index build time, so an index whose key survives but whose covered
+ /// column's subtree changed would keep emitting the old payload schema while covered ANN
+ /// queries declare the new one. This fires for two commit operations:
+ /// - `Project`, which can drop/rename/retype a covered field (public drop/alter APIs
+ /// preflight this, but a raw `Operation::Project` reaches `build_manifest` directly);
+ /// - `Merge` (how `add_columns` commits), which can grow a covered *struct* by adding a
+ /// child -- even an AllNulls, metadata-only child that writes no data file, so the
+ /// file-path-keyed coverage prune never sees it.
+ ///
+ /// `old_schema` is the pre-commit schema (needed to detect renames/retypes/child-adds,
+ /// which keep the covered field's id). `op` names the operation for the error message.
+ pub(super) fn reject_covered_field_subtree_change(
+ indices: &[IndexMetadata],
+ old_schema: Option<&Schema>,
+ new_schema: &Schema,
+ op: &str,
+ ) -> Result<()> {
+ let new_ids: HashSet = new_schema.fields_pre_order().map(|f| f.id).collect();
+ for index in indices {
+ // Skip only if a KEYED field left the schema: `retain_relevant_indices` drops
+ // the whole index once any of `fields` is gone, so a dropped key field means
+ // the index is on its way out anyway and there is no dangling covering to
+ // protect. Checking the *keyed* prefix specifically (not all of `fields`)
+ // matters because `fields` also lists the covering suffix (see
+ // `IndexMetadata::covering_fields`): a commit that drops *only* a covering
+ // field, leaving the key intact, must NOT be skipped here. If it were, this
+ // guard would let the drop through uncontested, and `retain_relevant_indices`
+ // -- which keys its own retention on the very same full `fields` list -- would
+ // then silently delete the entire index (not just desync its covering) with no
+ // error. Below, that same dropped covering id is caught by
+ // `covered_field_subtree_changed`'s "removed" case and rejected instead.
+ // (`FLAG_COVERED_INDEX_METADATA` fences pre-covering builds off the whole
+ // dataset, so a declaration erased by an old writer -- once this guard's
+ // other concern -- cannot arise.)
+ if !index.keyed_fields().iter().all(|id| new_ids.contains(id)) {
+ continue;
+ }
+ for &covered_id in index.covering_fields.iter() {
+ let changed = match old_schema {
+ Some(old) => Self::covered_field_subtree_changed(old, new_schema, covered_id),
+ // Without the pre-commit schema we can only detect a drop.
+ None => new_schema.field_by_id(covered_id).is_none(),
+ };
+ if changed {
+ return Err(Error::invalid_input(format!(
+ "{op} would drop or alter covered (included) field id {covered_id} \
+ (its subtree changed), still used by index '{}'. Drop the index with \
+ drop_index() before changing the column.",
+ index.name
+ )));
+ }
+ }
+ }
+ Ok(())
+ }
+
+ /// Whether a covered ("included") field's subtree differs between `old_schema` and
+ /// `new_schema` -- dropped, renamed, retyped, a nullability flip, or any child change.
+ /// Such a change means the index's stored physical payload no longer matches the
+ /// schema a query declares. `data_type()` is recursive for structs, so it also covers
+ /// a child's name/type/nullability change.
+ pub fn covered_field_subtree_changed(
+ old_schema: &Schema,
+ new_schema: &Schema,
+ covered_id: i32,
+ ) -> bool {
+ let Some(old) = old_schema.field_by_id(covered_id) else {
+ return false;
+ };
+ match new_schema.field_by_id(covered_id) {
+ None => true,
+ Some(new) => {
+ old.name != new.name
+ || old.nullable != new.nullable
+ || old.data_type() != new.data_type()
+ }
+ }
+ }
+
pub(crate) fn retain_relevant_indices(
indices: &mut Vec,
schema: &Schema,
@@ -451,6 +644,8 @@ impl Transaction {
mod tests {
use super::*;
use crate::transaction::test_support::overlay_with_field;
+ use arrow_schema::{DataType, Field as ArrowField, Schema as ArrowSchema};
+ use lance_core::datatypes::Schema as LanceSchema;
use uuid::Uuid;
#[test]
@@ -942,6 +1137,13 @@ mod tests {
// Post-remap state: every index already covers the new fragment (7).
let covering = || Some(RoaringBitmap::from_iter([7u32]));
+ // Indexes an un-overlaid field (3) but *covers* the overlaid field 1 as an
+ // included column; built (v2) before the overlay, so its stored copy of
+ // field 1 is stale and the rewritten fragment must be dropped.
+ let mut covered_stale = create_test_index("covered_stale", 3, 2, covering(), false);
+ // `covering_fields` is always the trailing entries of `fields`.
+ covered_stale.fields = vec![3, 1];
+ covered_stale.covering_fields = vec![1];
let mut indices = vec![
// Stale: covers the overlaid field 1, built (v2) before the overlay.
create_test_index("stale", 1, 2, covering(), false),
@@ -950,9 +1152,16 @@ mod tests {
create_test_index("fresh", 1, 5, covering(), false),
// Unrelated: covers field 2, which the overlay never touched.
create_test_index("unrelated", 2, 2, covering(), false),
+ covered_stale,
];
- Transaction::prune_overlay_stale_fields_from_indices(&mut indices, &groups);
+ // Flat field ids with no struct nesting: an empty schema makes subtree
+ // expansion a no-op (each id maps to itself), exercising the id-level logic.
+ Transaction::prune_overlay_stale_fields_from_indices(
+ &mut indices,
+ &groups,
+ &LanceSchema::default(),
+ );
assert!(
!indices[0].fragment_bitmap.as_ref().unwrap().contains(7),
@@ -966,6 +1175,207 @@ mod tests {
indices[2].fragment_bitmap.as_ref().unwrap().contains(7),
"an index on an un-overlaid field is unaffected"
);
+ assert!(
+ !indices[3].fragment_bitmap.as_ref().unwrap().contains(7),
+ "an index whose *covered* field was overlaid is stale too"
+ );
+ }
+
+ /// `covering_fields` records a covered struct's parent id, but a modified data file
+ /// lists leaf ids. Schema-aware pruning must expand the covered struct to its subtree
+ /// so an update to a subfield still invalidates the fragment's coverage.
+ #[test]
+ fn test_prune_updated_fields_expands_covered_struct_subtree() {
+ use arrow_schema::Fields as ArrowFields;
+
+ let arrow = ArrowSchema::new(vec![
+ ArrowField::new(
+ "s",
+ DataType::Struct(ArrowFields::from(vec![
+ ArrowField::new("a", DataType::Int32, true),
+ ArrowField::new("b", DataType::Int32, true),
+ ])),
+ true,
+ ),
+ ArrowField::new("x", DataType::Int32, true),
+ ]);
+ let schema = LanceSchema::try_from(&arrow).unwrap();
+ let s_id = schema.field("s").unwrap().id;
+ let a_id = u32::try_from(schema.field("s.a").unwrap().id).unwrap();
+ let x_id = schema.field("x").unwrap().id;
+
+ // Index keyed on scalar `x`, covering the whole struct `s`, over fragment 0.
+ let mut idx = create_test_index(
+ "cov",
+ x_id,
+ 1,
+ Some(RoaringBitmap::from_iter([0u32])),
+ false,
+ );
+ // `covering_fields` is always the trailing entries of `fields`.
+ idx.fields = vec![x_id, s_id];
+ idx.covering_fields = vec![s_id];
+
+ // Updating leaf `s.a` prunes the covered struct's fragment: the subtree
+ // expansion bridges the parent-id-vs-leaf-id mismatch that exact-id matching
+ // would miss.
+ let mut indices = vec![idx];
+ Transaction::prune_updated_fields_from_indices(
+ &mut indices,
+ &[Fragment::new(0)],
+ &[a_id],
+ &schema,
+ );
+ assert!(
+ !indices[0].fragment_bitmap.as_ref().unwrap().contains(0),
+ "updating a covered struct's subfield must drop the fragment from coverage"
+ );
+ }
+
+ /// A Project that drops or renames a covered (included) column must be rejected at
+ /// the commit boundary, even when the index's key column survives.
+ #[test]
+ fn test_reject_projected_covered_fields() {
+ let arrow = ArrowSchema::new(vec![
+ ArrowField::new("vec", DataType::Int32, false),
+ ArrowField::new("meta", DataType::Utf8, true),
+ ArrowField::new("other", DataType::Int32, true),
+ ]);
+ let old_schema = LanceSchema::try_from(&arrow).unwrap();
+ let vec_id = old_schema.field("vec").unwrap().id;
+ let meta_id = old_schema.field("meta").unwrap().id;
+
+ // Index keyed on `vec`, covering `meta`.
+ let mut idx = create_test_index(
+ "cov",
+ vec_id,
+ 1,
+ Some(RoaringBitmap::from_iter([0u32])),
+ true,
+ );
+ // `covering_fields` is always the trailing entries of `fields`.
+ idx.fields = vec![vec_id, meta_id];
+ idx.covering_fields = vec![meta_id];
+ let indices = vec![idx];
+
+ // Keeping every field -> Ok.
+ assert!(
+ Transaction::reject_covered_field_subtree_change(
+ &indices,
+ Some(&old_schema),
+ &old_schema,
+ "Project"
+ )
+ .is_ok()
+ );
+
+ // Dropping the covered `meta` while keeping the key -> rejected.
+ let dropped = old_schema.project(&["vec", "other"]).unwrap();
+ let err = Transaction::reject_covered_field_subtree_change(
+ &indices,
+ Some(&old_schema),
+ &dropped,
+ "Project",
+ )
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("drop or alter covered"),
+ "expected a covered-subtree rejection, got: {err}"
+ );
+
+ // Dropping the key too (the index would be dropped) -> not our concern.
+ let drop_all = old_schema.project(&["other"]).unwrap();
+ assert!(
+ Transaction::reject_covered_field_subtree_change(
+ &indices,
+ Some(&old_schema),
+ &drop_all,
+ "Project"
+ )
+ .is_ok()
+ );
+
+ // Renaming the covered field (id stays, name changes) -> rejected.
+ let mut renamed = old_schema.clone();
+ renamed.field_by_id_mut(meta_id).unwrap().name = "meta_renamed".to_string();
+ assert!(
+ Transaction::reject_covered_field_subtree_change(
+ &indices,
+ Some(&old_schema),
+ &renamed,
+ "Project"
+ )
+ .is_err(),
+ "renaming a covered field must be rejected"
+ );
+
+ // Flipping the covered field's nullability (id + name stay) -> rejected.
+ let mut retyped_null = old_schema.clone();
+ retyped_null.field_by_id_mut(meta_id).unwrap().nullable = false;
+ assert!(
+ Transaction::reject_covered_field_subtree_change(
+ &indices,
+ Some(&old_schema),
+ &retyped_null,
+ "Project"
+ )
+ .is_err(),
+ "a covered field nullability change must be rejected"
+ );
+ }
+
+ /// Growing a covered *struct*'s subtree (adding a child) is what `add_columns`/Merge
+ /// does; the covered field id is unchanged but its `data_type()` differs, so the commit
+ /// boundary must reject it just like the Project drop/alter cases above.
+ #[test]
+ fn test_reject_covered_struct_child_add() {
+ let child = ArrowField::new("a", DataType::Int32, false);
+ let arrow = ArrowSchema::new(vec![
+ ArrowField::new("vec", DataType::Int32, false),
+ ArrowField::new("meta", DataType::Struct(vec![child.clone()].into()), true),
+ ]);
+ let old_schema = LanceSchema::try_from(&arrow).unwrap();
+ let vec_id = old_schema.field("vec").unwrap().id;
+ let meta_id = old_schema.field("meta").unwrap().id;
+
+ let mut idx = create_test_index(
+ "cov",
+ vec_id,
+ 1,
+ Some(RoaringBitmap::from_iter([0u32])),
+ true,
+ );
+ // `covering_fields` is always the trailing entries of `fields`.
+ idx.fields = vec![vec_id, meta_id];
+ idx.covering_fields = vec![meta_id]; // covers the struct's parent id
+ let indices = vec![idx];
+
+ // New schema grows `meta` with a second child -> the struct's data_type changes.
+ let grown_arrow = ArrowSchema::new(vec![
+ ArrowField::new("vec", DataType::Int32, false),
+ ArrowField::new(
+ "meta",
+ DataType::Struct(vec![child, ArrowField::new("b", DataType::Int32, true)].into()),
+ true,
+ ),
+ ]);
+ // Preorder id assignment gives `meta` the same id (1) in both schemas, so the
+ // covered id resolves to the struct on each side and only its data_type differs.
+ let grown = LanceSchema::try_from(&grown_arrow).unwrap();
+ assert_eq!(grown.field("meta").unwrap().id, meta_id);
+
+ let err = Transaction::reject_covered_field_subtree_change(
+ &indices,
+ Some(&old_schema),
+ &grown,
+ "add_columns (Merge)",
+ )
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("drop or alter covered")
+ && err.to_string().contains("add_columns"),
+ "expected a covered-struct-growth rejection, got: {err}"
+ );
}
// Helper functions for retain_relevant_indices tests
diff --git a/rust/lance-table/src/transaction/manifest_build.rs b/rust/lance-table/src/transaction/manifest_build.rs
index 6f216f78c06..bbc16c9cd8e 100644
--- a/rust/lance-table/src/transaction/manifest_build.rs
+++ b/rust/lance-table/src/transaction/manifest_build.rs
@@ -697,6 +697,7 @@ impl Transaction {
&mut final_indices,
updated_fragments,
fields_modified,
+ &schema,
);
let mut new_fragments =
@@ -841,7 +842,7 @@ impl Transaction {
// base data. Any index older than one of those overlays was built on
// the pre-overlay values, so drop the rewritten fragment from its
// coverage to keep it from serving stale values.
- Self::prune_overlay_stale_fields_from_indices(&mut final_indices, groups);
+ Self::prune_overlay_stale_fields_from_indices(&mut final_indices, groups, &schema);
if let Some(frag_reuse_index) = frag_reuse_index {
final_indices.retain(|idx| idx.name != frag_reuse_index.name);
@@ -870,6 +871,45 @@ impl Transaction {
new_index.validate_covering_fields()?;
}
final_indices.extend(new_indices.clone());
+ // All delta segments of one logical index (same name) must declare
+ // the same covering columns: the covered read path derives its
+ // output schema from one delta but executes all of them, so
+ // committing a mixed set would fail every query on the index.
+ // Scoped to names this operation adds, so a commit untouched by
+ // covering cannot be blocked by pre-existing state.
+ for new_index in new_indices {
+ // A declared covering field must resolve to a TOP-LEVEL field of
+ // the schema being committed: the read path resolves the id to its
+ // bare name, so a nested id (e.g. a struct child) would silently
+ // bind to a same-named top-level column, and a nonexistent id
+ // fails every later query at planning time ("index metadata and
+ // schema are inconsistent"). Reject at the commit boundary.
+ for id in &new_index.covering_fields {
+ if !schema.fields.iter().any(|field| field.id == *id) {
+ return Err(Error::invalid_input(format!(
+ "CreateIndex: index '{}' (segment {}) declares covering \
+ field id {}, which is not a top-level field of the \
+ schema (covering columns must be top-level)",
+ new_index.name, new_index.uuid, id
+ )));
+ }
+ }
+ if let Some(conflict) = final_indices.iter().find(|idx| {
+ idx.name == new_index.name
+ && idx.covering_fields != new_index.covering_fields
+ }) {
+ return Err(Error::invalid_input(format!(
+ "CreateIndex: delta segment {} of index '{}' declares covering \
+ fields {:?}, but delta segment {} declares {:?}; all segments of \
+ one index must declare the same covering columns",
+ new_index.uuid,
+ new_index.name,
+ new_index.covering_fields,
+ conflict.uuid,
+ conflict.covering_fields
+ )));
+ }
+ }
}
Operation::ReserveFragments { .. } | Operation::UpdateConfig { .. } => {
final_fragments.extend(maybe_existing_fragments?.clone());
@@ -906,6 +946,18 @@ impl Transaction {
}
final_fragments.extend(merged_fragments);
+ // `add_columns` commits as a Merge; growing a *covered* field's subtree
+ // (e.g. an AllNulls child under a covered struct, which writes no data file
+ // and so is invisible to the file-path-keyed prune below) would leave the
+ // index emitting the old payload schema while covered queries declare the
+ // new one. Reject at the commit boundary, mirroring the Project guard.
+ Self::reject_covered_field_subtree_change(
+ &final_indices,
+ current_manifest.map(|m| &m.schema),
+ &schema,
+ "add_columns (Merge)",
+ )?;
+
// A Merge can rewrite a column's data file in place; the field stays
// in the schema, so the index is retained -- prune its now-stale
// entries for the rewritten fragments.
@@ -913,6 +965,7 @@ impl Transaction {
&mut final_indices,
existing_fragments,
fragments,
+ &schema,
);
// Some fields that have indices may have been removed, so we should
@@ -936,6 +989,16 @@ impl Transaction {
});
}
+ // A Project that drops or renames a *covered* field would leave dangling
+ // index metadata whose storage still emits the column -- reject it before
+ // dropping/retaining any indices.
+ Self::reject_covered_field_subtree_change(
+ &final_indices,
+ current_manifest.map(|m| &m.schema),
+ &schema,
+ "Project",
+ )?;
+
// Some fields that have indices may have been removed, so we should
// remove those indices as well.
Self::retain_relevant_indices(&mut final_indices, &schema, &final_fragments)
@@ -1162,6 +1225,7 @@ impl Transaction {
&mut final_indices,
&modified_fragments,
&replaced_fields,
+ &schema,
);
}
Operation::DataOverlay { groups } => {
@@ -1573,6 +1637,95 @@ mod tests {
)
}
+ /// A stable-row-id `RewriteRows` update that modifies ONLY a covering
+ /// (included) column must not re-extend a covered index's fragment
+ /// coverage onto the moved fragment: the index still holds the pre-update
+ /// materialized value, so reusing it would serve a stale covered result.
+ /// The modified field is in `covering_fields`, not `fields`.
+ ///
+ /// Both spellings of "the covering column changed" must be caught. `covering_fields`
+ /// only ever holds TOP-LEVEL ids (index creation resolves whole columns and rejects
+ /// dotted names), but the caller may report either the covered struct's own id or the
+ /// leaf ids it actually rewrote -- `Operation::Update` is public, so
+ /// `fields_for_preserving_frag_bitmap` is caller-supplied and unvalidated. Comparing raw
+ /// ids catches only the first spelling and silently serves stale subfield values for the
+ /// second.
+ #[test]
+ fn test_pure_rewrite_preserves_covering_index_coverage() {
+ use arrow_schema::Fields as ArrowFields;
+
+ // meta{a} => ids 0 (struct) and 1 (leaf); vector => id 2.
+ let arrow = ArrowSchema::new(vec![
+ ArrowField::new(
+ "meta",
+ DataType::Struct(ArrowFields::from(vec![ArrowField::new(
+ "a",
+ DataType::Int32,
+ true,
+ )])),
+ true,
+ ),
+ ArrowField::new("vector", DataType::Int32, true),
+ ]);
+ let schema = LanceSchema::try_from(&arrow).unwrap();
+ let meta_id = schema.field("meta").unwrap().id;
+ let leaf_id = schema.field("meta.a").unwrap().id;
+ let vector_id = schema.field("vector").unwrap().id;
+
+ // Returns (covered index re-extended?, plain index re-extended?).
+ let run = |modified: &[u32]| {
+ let mut covering = sample_index_metadata("covering");
+ // `covering_fields` is always the trailing entries of `fields`.
+ covering.fields = vec![vector_id, meta_id]; // top-level, as creation guarantees
+ covering.covering_fields = vec![meta_id];
+ covering.fragment_bitmap = Some([0].into_iter().collect());
+
+ let mut plain = sample_index_metadata("plain");
+ plain.fields = vec![vector_id];
+ plain.covering_fields = vec![];
+ plain.fragment_bitmap = Some([0].into_iter().collect());
+
+ let mut indices = vec![covering, plain];
+ let overlaid: HashMap = HashMap::new();
+ Transaction::register_pure_rewrite_rows_update_frags_in_indices(
+ &mut indices,
+ &[1], // new fragment holding the moved rows
+ &[0], // original fragment (covered by both indices)
+ modified,
+ &overlaid,
+ &schema,
+ )
+ .unwrap();
+ (
+ indices[0].fragment_bitmap.as_ref().unwrap().contains(1),
+ indices[1].fragment_bitmap.as_ref().unwrap().contains(1),
+ )
+ };
+
+ for (label, modified) in [
+ (
+ "covered struct reported by its own id",
+ vec![meta_id as u32],
+ ),
+ (
+ "covered struct reported by its leaf id",
+ vec![leaf_id as u32],
+ ),
+ ] {
+ let (covered_extended, plain_extended) = run(&modified);
+ assert!(
+ !covered_extended,
+ "covered index must not re-extend coverage onto a fragment whose covering \
+ column was rewritten ({label})"
+ );
+ assert!(
+ plain_extended,
+ "index that neither indexes nor covers the modified field still reuses its \
+ entries ({label})"
+ );
+ }
+ }
+
#[test]
fn test_create_index_build_manifest_keeps_unremoved_same_name_indices() {
let manifest = sample_manifest();
@@ -1649,6 +1802,251 @@ mod tests {
);
}
+ /// A raw CreateIndex commit whose new index declares a covering field id missing
+ /// from the schema must be rejected at the commit boundary -- otherwise every later
+ /// query on the index fails at planning time with an inconsistent-metadata error.
+ ///
+ /// `covering_fields` must also be a suffix of `fields` (`fields = [0, 9999]` here),
+ /// or `validate_covering_fields`'s cardinality check rejects the index earlier, for
+ /// an unrelated reason, before this test's intended "unresolvable in schema" check
+ /// ever runs.
+ #[test]
+ fn test_create_index_build_manifest_rejects_unresolvable_covered_field() {
+ let manifest = sample_manifest();
+ let mut covered = sample_index_metadata("vector_idx");
+ covered.fields = vec![0, 9999];
+ covered.covering_fields = vec![9999];
+ let transaction = Transaction::new(
+ manifest.version,
+ Operation::CreateIndex {
+ new_indices: vec![covered],
+ removed_indices: vec![],
+ },
+ None,
+ );
+ let err = transaction
+ .build_manifest(Some(&manifest), vec![], "txn", &default_build_config())
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("9999") && err.to_string().contains("top-level"),
+ "an unresolvable covered field id must be rejected at commit: {err}"
+ );
+ }
+
+ /// A covering declaration must reference a TOP-LEVEL field: `covering_fields`
+ /// resolution on the read path goes id -> name -> arrow field by bare name, so a
+ /// nested field id (e.g. a struct child) would silently bind to a same-named
+ /// top-level column instead of erroring. The create path rejects dotted names;
+ /// raw commits must reject nested ids for the same reason.
+ #[test]
+ fn test_create_index_build_manifest_rejects_nested_covered_field() {
+ use arrow_schema::Fields as ArrowFields;
+
+ let arrow = ArrowSchema::new(vec![
+ ArrowField::new("id", DataType::Int32, false),
+ ArrowField::new(
+ "s",
+ DataType::Struct(ArrowFields::from(vec![ArrowField::new(
+ "id",
+ DataType::Int64,
+ true,
+ )])),
+ true,
+ ),
+ ]);
+ let schema = LanceSchema::try_from(&arrow).unwrap();
+ let nested_id = schema.field("s.id").unwrap().id;
+ let manifest = Manifest::new(
+ schema,
+ Arc::new(vec![Fragment::new(0)]),
+ DataStorageFormat::new(ConcreteFileVersion::V2_0),
+ HashMap::new(),
+ );
+
+ let mut covered = sample_index_metadata("vector_idx");
+ covered.fields = vec![0, nested_id];
+ covered.covering_fields = vec![nested_id];
+ let transaction = Transaction::new(
+ manifest.version,
+ Operation::CreateIndex {
+ new_indices: vec![covered],
+ removed_indices: vec![],
+ },
+ None,
+ );
+ let err = transaction
+ .build_manifest(Some(&manifest), vec![], "txn", &default_build_config())
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("top-level"),
+ "a nested covered field id must be rejected at commit: {err}"
+ );
+ }
+
+ /// A raw `Operation::Project` that drops ONLY a covering (included) field, keeping the
+ /// index's key field intact, must be rejected at the commit boundary. `fields` lists the
+ /// covering suffix alongside the key (see `IndexMetadata::covering_fields`), so
+ /// `retain_relevant_indices` -- which requires every entry of `fields` to survive --
+ /// would otherwise auto-prune the WHOLE index the moment the commit went through, with
+ /// no error: a working vector index silently disappearing rather than merely desyncing
+ /// its covering payload.
+ #[test]
+ fn test_project_build_manifest_rejects_dropped_covering_field() {
+ let arrow = ArrowSchema::new(vec![
+ ArrowField::new("vec", DataType::Int32, false),
+ ArrowField::new("meta", DataType::Utf8, true),
+ ]);
+ let schema = LanceSchema::try_from(&arrow).unwrap();
+ let vec_id = schema.field("vec").unwrap().id;
+ let meta_id = schema.field("meta").unwrap().id;
+ let manifest = Manifest::new(
+ schema,
+ Arc::new(vec![Fragment::new(0)]),
+ DataStorageFormat::new(ConcreteFileVersion::V2_0),
+ HashMap::new(),
+ );
+
+ let mut covered = sample_index_metadata("vector_idx");
+ covered.fields = vec![vec_id, meta_id];
+ covered.covering_fields = vec![meta_id];
+
+ let projected_schema = manifest.schema.project(&["vec"]).unwrap();
+ let transaction = Transaction::new(
+ manifest.version,
+ Operation::Project {
+ schema: projected_schema,
+ preserves_nullability: true,
+ },
+ None,
+ );
+ let err = transaction
+ .build_manifest(
+ Some(&manifest),
+ vec![covered],
+ "txn",
+ &default_build_config(),
+ )
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("drop or alter covered"),
+ "dropping a covering field while its index's key survives must be rejected \
+ instead of silently letting the whole index be auto-pruned: {err}"
+ );
+ }
+
+ /// All delta segments of one logical index must declare the same covering columns:
+ /// the covered read path derives its output schema from one delta but executes all
+ /// of them, so a mixed set fails every query on the index at execution time.
+ #[test]
+ fn test_create_index_build_manifest_rejects_mixed_covering_deltas() {
+ // A second top-level field is needed so the covered index can declare it as a
+ // trailing, schema-resolvable `covering_fields` entry (`sample_manifest`'s
+ // single-field schema has no field left over once field 0 is keyed on).
+ let arrow = ArrowSchema::new(vec![
+ ArrowField::new("id", DataType::Int32, false),
+ ArrowField::new("vector", DataType::Int32, false),
+ ]);
+ let schema = LanceSchema::try_from(&arrow).unwrap();
+ let covered_field_id = schema.field("vector").unwrap().id;
+ let manifest = Manifest::new(
+ schema,
+ Arc::new(vec![Fragment::new(0)]),
+ DataStorageFormat::new(ConcreteFileVersion::V2_0),
+ HashMap::new(),
+ );
+ let mut covered = sample_index_metadata("vector_idx");
+ covered.fields = vec![0, covered_field_id];
+ covered.covering_fields = vec![covered_field_id];
+ let plain = sample_index_metadata("vector_idx");
+
+ // Adding a non-covered delta to an existing covered index is rejected.
+ let transaction = Transaction::new(
+ manifest.version,
+ Operation::CreateIndex {
+ new_indices: vec![plain.clone()],
+ removed_indices: vec![],
+ },
+ None,
+ );
+ let err = transaction
+ .build_manifest(
+ Some(&manifest),
+ vec![covered.clone()],
+ "txn",
+ &default_build_config(),
+ )
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("covering"),
+ "mixed covering across deltas must be rejected: {err}"
+ );
+
+ // A mixed covering set within one commit's own new segments is rejected too.
+ let transaction = Transaction::new(
+ manifest.version,
+ Operation::CreateIndex {
+ new_indices: vec![covered.clone(), plain.clone()],
+ removed_indices: vec![],
+ },
+ None,
+ );
+ let err = transaction
+ .build_manifest(Some(&manifest), vec![], "txn", &default_build_config())
+ .unwrap_err();
+ assert!(
+ err.to_string().contains("covering"),
+ "mixed covering within one commit must be rejected: {err}"
+ );
+
+ // Removing the disagreeing delta in the same commit leaves a homogeneous
+ // final state, which is fine.
+ let mut covered2 = sample_index_metadata("vector_idx");
+ covered2.fields = vec![0, covered_field_id];
+ covered2.covering_fields = vec![covered_field_id];
+ let transaction = Transaction::new(
+ manifest.version,
+ Operation::CreateIndex {
+ new_indices: vec![covered2],
+ removed_indices: vec![plain.clone()],
+ },
+ None,
+ );
+ let (_, final_indices) = transaction
+ .build_manifest(
+ Some(&manifest),
+ vec![covered.clone(), plain],
+ "txn",
+ &default_build_config(),
+ )
+ .unwrap();
+ assert_eq!(final_indices.len(), 2);
+ assert!(
+ final_indices
+ .iter()
+ .all(|idx| idx.covering_fields == vec![covered_field_id]),
+ "the homogeneous replacement set should commit"
+ );
+
+ // An index with a different name is unaffected.
+ let other = sample_index_metadata("other_idx");
+ let transaction = Transaction::new(
+ manifest.version,
+ Operation::CreateIndex {
+ new_indices: vec![other],
+ removed_indices: vec![],
+ },
+ None,
+ );
+ transaction
+ .build_manifest(
+ Some(&manifest),
+ vec![covered],
+ "txn",
+ &default_build_config(),
+ )
+ .unwrap();
+ }
+
#[test]
fn test_update_build_manifest_replaces_and_removes_fragments() {
let manifest = sample_manifest_with_fragments(0..5);
diff --git a/rust/lance/src/dataset/index.rs b/rust/lance/src/dataset/index.rs
index a3e0917d432..ff46d27adae 100644
--- a/rust/lance/src/dataset/index.rs
+++ b/rust/lance/src/dataset/index.rs
@@ -421,79 +421,6 @@ mod tests {
assert_ne!(remapped[0].new_id, unaffected_segment_id);
}
- /// A covered index must be withdrawn from remapping rather than remapped.
- /// No index type carries the declared payload through a remap, so a
- /// replacement would republish a covering claim its storage does not back.
- /// Withdrawal also means the legacy `index_details: None` migration path is
- /// never reached for such an index, so it cannot panic there either.
- #[tokio::test]
- async fn test_remapper_migration_path_withdraws_covered_index() {
- let reader = lance_datagen::gen_batch()
- .col("a", array::step::())
- .col("b", array::step::())
- .into_reader_rows(RowCount::from(20), BatchCount::from(1));
- let mut dataset = Dataset::write(reader, "memory://", None).await.unwrap();
- dataset
- .create_index(
- &["a"],
- IndexType::BTree,
- None,
- &ScalarIndexParams::for_builtin(BuiltinIndexType::BTree),
- false,
- )
- .await
- .unwrap();
-
- let a_id = dataset.schema().field("a").unwrap().id;
- let b_id = dataset.schema().field("b").unwrap().id;
- let current = dataset.load_indices().await.unwrap();
- let mut legacy_covered = current[0].clone();
- legacy_covered.fields = vec![a_id, b_id];
- legacy_covered.covering_fields = vec![b_id];
- // Force the legacy migration path this fix touches.
- legacy_covered.index_details = None;
-
- let transaction = Transaction::new(
- dataset.manifest.version,
- Operation::CreateIndex {
- new_indices: vec![legacy_covered],
- removed_indices: current.to_vec(),
- },
- None,
- );
- dataset
- .apply_commit(transaction, &Default::default(), &Default::default())
- .await
- .unwrap();
-
- let index_uuid = dataset.load_indices().await.unwrap()[0].uuid;
-
- // Fully delete every row so `remap_index` returns `RemapResult::Keep`,
- // landing in the `index_details: None` migration branch this fix touches.
- let remap_to_empty = (0..dataset.count_all_rows().await.unwrap())
- .map(|i| (i as u64, None))
- .collect::>();
- let remapper = DatasetIndexRemapperOptions::default()
- .create_remapper(&dataset)
- .await
- .unwrap()
- .expect("a real index should require a remapper");
- let remapped = remapper
- .remap_indices(RowAddrRemap::direct(remap_to_empty), &[0])
- .await
- .unwrap();
-
- // Withdrawn, not remapped: no index type carries the declared payload
- // through a remap, so producing a replacement would republish a covering
- // claim its storage does not back. The original entry stays in the
- // manifest and simply stops covering the rewritten fragments.
- assert!(
- remapped.is_empty(),
- "a covered index must be withdrawn from remapping, got {remapped:?}"
- );
- let _ = index_uuid;
- }
-
/// The same migration path must reject -- cleanly, not with a panic -- a
/// malformed `covering_fields` longer than `fields`.
///
diff --git a/rust/lance/src/dataset/optimize.rs b/rust/lance/src/dataset/optimize.rs
index 81f01882e55..5f8b4f06645 100644
--- a/rust/lance/src/dataset/optimize.rs
+++ b/rust/lance/src/dataset/optimize.rs
@@ -6994,6 +6994,7 @@ mod tests {
version: crate::index::vector::IndexFileVersion::V3,
skip_transpose: false,
runtime_hints: Default::default(),
+ covering_columns: Default::default(),
},
false,
)
@@ -7129,6 +7130,7 @@ mod tests {
version: crate::index::vector::IndexFileVersion::V3,
skip_transpose: false,
runtime_hints: Default::default(),
+ covering_columns: Default::default(),
},
false,
)
diff --git a/rust/lance/src/dataset/optimize/remapping.rs b/rust/lance/src/dataset/optimize/remapping.rs
index 8a9c8898cb6..065612ed862 100644
--- a/rust/lance/src/dataset/optimize/remapping.rs
+++ b/rust/lance/src/dataset/optimize/remapping.rs
@@ -307,16 +307,23 @@ async fn remap_index(dataset: &mut Dataset, index_id: &Uuid) -> Result<()> {
// does not incorporate overlays committed after the source index was built.
// Exclude those fragments so queries scan their current values instead.
if let Some(fragment_bitmap) = &mut bitmap_after_remap {
+ // Fields whose value this index materializes: its indexed key fields plus every
+ // covered (included) column expanded to its leaf subtree (an overlay lists leaf
+ // ids, while `covering_fields` records a parent struct id).
+ let relevant_field_ids =
+ Transaction::index_dependent_leaf_ids(&curr_index_meta, dataset.schema());
for fragment in dataset.manifest.fragments.iter() {
- let has_newer_indexed_overlay = fragment.overlays.iter().any(|overlay| {
+ // An overlay refreshing an indexed or covered field makes the index storage
+ // stale for that fragment: it still holds the pre-overlay copy of the value.
+ let has_newer_relevant_overlay = fragment.overlays.iter().any(|overlay| {
overlay.committed_version > curr_index_meta.dataset_version
&& overlay
.data_file
.fields
.iter()
- .any(|field_id| curr_index_meta.fields.contains(field_id))
+ .any(|field_id| relevant_field_ids.contains(field_id))
});
- if has_newer_indexed_overlay {
+ if has_newer_relevant_overlay {
fragment_bitmap.remove(fragment.id as u32);
}
}
diff --git a/rust/lance/src/dataset/scanner.rs b/rust/lance/src/dataset/scanner.rs
index 87f2d83a281..21c93c051cb 100644
--- a/rust/lance/src/dataset/scanner.rs
+++ b/rust/lance/src/dataset/scanner.rs
@@ -5649,11 +5649,32 @@ impl Scanner {
knn_node = self.take(knn_node, vector_projection)?;
}
+ // The index search now emits the index's covering ("included") columns, so every flat
+ // path below must produce the same columns before it is unioned with `knn_node`;
+ // otherwise projecting the flat output to `knn_node.schema()` panics on the missing column.
+ let covering_columns: Vec = indexed_segments
+ .first()
+ .map(|s| s.covering_fields.as_slice())
+ .unwrap_or(&[])
+ .iter()
+ .filter_map(|id| {
+ self.dataset
+ .schema()
+ .field_by_id(*id)
+ .map(|field| field.name.clone())
+ })
+ .collect();
+
let mut columns = vec![q.column.clone()];
if let Some(expr) = filter_plan.full_expr.as_ref() {
let filter_columns = Planner::column_names_in_expr(expr);
columns.extend(filter_columns);
}
+ for name in &covering_columns {
+ if !columns.contains(name) {
+ columns.push(name.clone());
+ }
+ }
// Collect flat-path plans; union order matches original (flat before ANN) so test snapshots
// and downstream plan analyses remain stable.
@@ -5708,6 +5729,9 @@ impl Scanner {
let filter_columns = Planner::column_names_in_expr(expr);
take_proj = take_proj.union_columns(filter_columns, OnMissing::Error)?;
}
+ if !covering_columns.is_empty() {
+ take_proj = take_proj.union_columns(covering_columns.clone(), OnMissing::Error)?;
+ }
let mut stale_node = self.stale_rows_take(stale_rows, take_proj).await?;
if let Some(expr) = filter_plan.full_expr.as_ref() {
stale_node = Arc::new(LanceFilterExec::try_new(expr.clone(), stale_node)?);
@@ -15032,6 +15056,7 @@ full_filter=name LIKE Utf8(\"test%2\"), refine_filter=name LIKE Utf8(\"test%2\")
version: crate::index::vector::IndexFileVersion::Legacy,
skip_transpose: false,
runtime_hints: Default::default(),
+ covering_columns: Default::default(),
},
false,
)
diff --git a/rust/lance/src/dataset/schema_evolution.rs b/rust/lance/src/dataset/schema_evolution.rs
index af0e1ab2622..b93f90f3bd6 100644
--- a/rust/lance/src/dataset/schema_evolution.rs
+++ b/rust/lance/src/dataset/schema_evolution.rs
@@ -793,37 +793,102 @@ pub(super) async fn alter_columns(
new_schema.validate()?;
new_schema.verify_primary_key()?;
- // If any column being cast has an attached index, fail fast. Cast operations
- // rewrite the underlying column data and silently invalidate any index on the
- // affected column(s). The current behavior is to drop such indices without
- // warning, which has caused production incidents where vector search silently
- // regressed to brute-force scan. We require users to explicitly drop the
- // index before altering the column type, so the action is never silent. That
- // includes an index this build has no reader for: the cast reassigns the
- // field id, so carrying it forward is impossible and staying quiet about it
- // is the silent drop this guard exists to abolish.
- if !cast_fields.is_empty() {
+ // Fail fast on alterations that would invalidate an index or desync its
+ // covering data:
+ // * CAST of an indexed *key* column -- cast rewrites the column data and
+ // silently invalidates the index. This has caused production incidents
+ // where vector search regressed to a brute-force scan. (Indices are
+ // keyed by field id, so a plain rename or nullability change of a key
+ // column is safe and stays allowed.)
+ // * ANY alter (rename, cast, or nullability) of a covering ("included")
+ // column. `fields` lists the covering suffix right alongside the key
+ // (see `IndexMetadata::covering_fields`), so the auto-prune
+ // (`retain_relevant_indices`) DOES see a covering column's id.
+ //
+ // For a CAST this guard is load-bearing: a cast reassigns the column a
+ // fresh field id, so the old covering id disappears from the schema and
+ // auto-prune -- which requires every id in `fields` to survive -- would
+ // delete the WHOLE index, not just leave its covering stale.
+ //
+ // For a plain rename or nullability flip the id survives, so auto-prune
+ // never fires and the index is left declaring covering that its storage
+ // no longer matches. That is NOT silent corruption: the read path
+ // intersects the declaration with each segment's proven physical
+ // capability (`common_physical_covering`, which compares name, type,
+ // nullability and metadata), so a mismatch withdraws covering and the
+ // query falls back to a base-table take -- correct results, minus the
+ // elision. The guard is conservative here rather than load-bearing, and
+ // is kept so a lost elision surfaces as an explicit error instead of a
+ // silent slowdown. Relaxing it to degrade instead of reject is a tracked
+ // follow-up.
+ //
+ // Both cases include an index this build has no reader for -- hence
+ // `load_all_indices`. A cast reassigns the field id, so carrying such an index
+ // forward is impossible, and staying quiet about it is the silent drop these
+ // guards exist to abolish.
+ // Reject rather than let the index quietly change behaviour; the cast case
+ // above is the one that would otherwise destroy the index outright.
+ let cast_ids: Vec = alterations
+ .iter()
+ .filter(|a| a.data_type.is_some())
+ .filter_map(|a| dataset.schema().field(&a.path).map(|f| f.id))
+ .collect();
+ let altered: Vec<(&str, i32)> = alterations
+ .iter()
+ .filter(|a| a.rename.is_some() || a.data_type.is_some() || a.nullable.is_some())
+ .filter_map(|a| {
+ dataset
+ .schema()
+ .field(&a.path)
+ .map(|f| (a.path.as_str(), f.id))
+ })
+ .collect();
+ if !altered.is_empty() {
let indices = load_all_indices(dataset).await?;
- let affected: Vec<&lance_table::format::IndexMetadata> = indices
- .iter()
- .filter(|idx| {
- cast_fields
- .iter()
- .any(|(old, _)| idx.fields.contains(&old.id))
+ // A covered column is stored as a whole subtree, so altering a *descendant* of a
+ // covered field (e.g. a subfield of a covered struct) invalidates the covering
+ // too; expand each altered field to its ancestors so the covered ancestor's id is
+ // caught. The expansion depends only on the schema, so do it once per altered
+ // field here rather than per (index x altered field) below.
+ let altered: Vec<(&str, i32, Vec)> = altered
+ .into_iter()
+ .map(|(name, id)| {
+ let ancestors =
+ field_ids_with_ancestors(dataset.schema(), std::slice::from_ref(&id));
+ (name, id, ancestors)
})
.collect();
+ // An index is broken if a covering column of it is altered in any way, or if one
+ // of its indexed key columns is being cast. The cast check stays on the field's
+ // own id (only a cast of the indexed key column rewrites index data).
+ let is_broken = |idx: &lance_table::format::IndexMetadata| {
+ altered.iter().any(|(_, _, ancestors)| {
+ ancestors
+ .iter()
+ .any(|ancestor| idx.covering_fields.contains(ancestor))
+ }) || cast_ids.iter().any(|id| idx.fields.contains(id))
+ };
+ let affected: Vec<&lance_table::format::IndexMetadata> =
+ indices.iter().filter(|idx| is_broken(idx)).collect();
if !affected.is_empty() {
- let affected_cols: Vec = cast_fields
+ let affected_cols: Vec = altered
.iter()
- .filter(|(old, _)| affected.iter().any(|i| i.fields.contains(&old.id)))
- .map(|(old, _)| old.name.clone())
+ .filter(|(_, id, ancestors)| {
+ affected.iter().any(|i| {
+ ancestors
+ .iter()
+ .any(|ancestor| i.covering_fields.contains(ancestor))
+ || (cast_ids.contains(id) && i.fields.contains(id))
+ })
+ })
+ .map(|(name, _, _)| name.to_string())
.collect();
let affected_idx_names: Vec = affected.iter().map(|i| i.name.clone()).collect();
return Err(Error::invalid_input(format!(
- "Cannot cast column(s) [{}] to a new type: they have {} index(es) \
- attached: [{}]. Cast rewrites column data and invalidates any index \
- on the affected column(s). Drop the index(es) with drop_index() \
- before altering, then recreate them after the cast completes.",
+ "Cannot alter column(s) [{}]: they are used by {} index(es) [{}] -- as a \
+ covering (included) column (any change), or as an indexed key column being \
+ cast (which rewrites the data and invalidates the index). Drop the index(es) \
+ with drop_index() before altering, then recreate them.",
affected_cols.join(", "),
affected.len(),
affected_idx_names.join(", "),
@@ -1017,6 +1082,23 @@ pub(super) async fn alter_columns(
Ok(())
}
+/// Expand `field_ids` to also include every ancestor field id (root -> field).
+/// A covering column is stored as a whole subtree, so touching a *descendant* of a
+/// covered field (e.g. a subfield of a covered struct) still invalidates the covering
+/// even though `covering_fields` records only the ancestor's id. Adding each targeted
+/// field's ancestors makes the ancestor's id appear in the set so the exact-membership
+/// checks catch it.
+fn field_ids_with_ancestors(schema: &Schema, field_ids: &[i32]) -> Vec {
+ let mut expanded = Vec::with_capacity(field_ids.len());
+ for &id in field_ids {
+ match schema.field_ancestry_by_id(id) {
+ Some(ancestry) => expanded.extend(ancestry.iter().map(|field| field.id)),
+ None => expanded.push(id),
+ }
+ }
+ expanded
+}
+
/// Remove columns from the dataset.
///
/// This is a metadata-only operation and does not remove the data from the
@@ -1045,6 +1127,63 @@ pub(super) async fn drop_columns(dataset: &mut Dataset, columns: &[&str]) -> Res
));
}
+ // Fail fast if any column being dropped is a covering ("included") column of
+ // an index, while the index's key survives. Dropping the key too is fine --
+ // `retain_relevant_indices` auto-prunes the whole index once any id in `fields`
+ // leaves the schema, and `fields` lists the covering suffix right alongside the
+ // key (see `IndexMetadata::covering_fields`), so a key+covering drop cleanly
+ // removes the whole thing. But that is exactly why dropping *only* the covering
+ // column, without this guard, would not merely leave the index's storage
+ // referencing a missing column -- the same auto-prune check would see the
+ // covering id vanish from `fields` and silently delete the ENTIRE index (the key,
+ // its ANN structure, everything), not just desync its covering. Require the user
+ // to drop the index first so the action is never silent.
+ let drop_field_ids: Vec = columns
+ .iter()
+ .filter_map(|c| dataset.schema().field(c).map(|f| f.id))
+ .collect();
+ let indices = load_all_indices(dataset).await?;
+ // Treat a descendant of a covered (e.g. struct) column as covered too: the covering
+ // declaration names the struct while the drop names a leaf inside it.
+ let drop_field_ids = field_ids_with_ancestors(dataset.schema(), &drop_field_ids);
+ // An index whose *key* is also being dropped is auto-pruned by
+ // `retain_relevant_indices`, so its covering metadata cannot dangle -- the whole index
+ // goes away. Rejecting here would block the perfectly safe "drop the vector column and
+ // its covered payload together" case. The commit-boundary guard
+ // (`reject_covered_field_subtree_change`) skips these indices for the same reason, and
+ // this preflight must agree with it or it forbids what the commit would accept.
+ //
+ // Agreement requires testing what that guard tests: whether a KEYED field survives the
+ // post-drop schema. Neither half of that can be read off `drop_field_ids`. It is
+ // expanded *upwards* to ancestors, so dropping `s.a` lists the struct `s` that in fact
+ // survives; and the keyed prefix is not all of `fields`, which also carries the
+ // covering suffix. Testing `fields` against the expanded set got both wrong in
+ // opposite directions -- it rejected dropping a key alongside a strict subset of its
+ // covering columns (the commit accepts that: the key is gone, the index goes with it),
+ // and it skipped an index keyed on a struct when a leaf under that struct was dropped,
+ // deferring a case this preflight is meant to catch to a far less legible error.
+ let surviving: HashSet = new_schema.fields_pre_order().map(|f| f.id).collect();
+ let affected: Vec<&lance_table::format::IndexMetadata> = indices
+ .iter()
+ .filter(|idx| idx.keyed_fields().iter().all(|id| surviving.contains(id)))
+ .filter(|idx| {
+ drop_field_ids
+ .iter()
+ .any(|id| idx.covering_fields.contains(id))
+ })
+ .collect();
+ if !affected.is_empty() {
+ let affected_idx_names: Vec = affected.iter().map(|i| i.name.clone()).collect();
+ return Err(Error::invalid_input(format!(
+ "Cannot drop column(s) [{}]: they are a covering (included) column of {} \
+ index(es) [{}]. Drop the index(es) with drop_index() before dropping the \
+ column(s), then recreate them.",
+ columns.join(", "),
+ affected.len(),
+ affected_idx_names.join(", "),
+ )));
+ }
+
let transaction = Transaction::new(
dataset.manifest.version,
Operation::Project {
@@ -3589,6 +3728,365 @@ mod test {
Ok(())
}
+ /// The covered-column schema guards must treat a *descendant* of a covered field as
+ /// covered too: a covered column is stored as a whole subtree, so `covering_fields`
+ /// records only the ancestor's id. `field_ids_with_ancestors` expands a targeted
+ /// subfield to include that ancestor so the exact-membership guard catches it.
+ #[test]
+ fn test_field_ids_with_ancestors_expands_struct_subtree() {
+ let arrow_schema = ArrowSchema::new(vec![
+ ArrowField::new(
+ "s",
+ DataType::Struct(ArrowFields::from(vec![
+ ArrowField::new("a", DataType::Int32, true),
+ ArrowField::new("b", DataType::Int32, true),
+ ])),
+ true,
+ ),
+ ArrowField::new("x", DataType::Int32, true),
+ ]);
+ let schema = Schema::try_from(&arrow_schema).unwrap();
+ let s_id = schema.field("s").unwrap().id;
+ let a_id = schema.field("s.a").unwrap().id;
+
+ let expanded = field_ids_with_ancestors(&schema, &[a_id]);
+ assert!(
+ expanded.contains(&s_id),
+ "expanding a subfield must include its covered struct ancestor's id"
+ );
+ assert!(
+ expanded.contains(&a_id),
+ "the field's own id must be included"
+ );
+ }
+
+ /// Dropping a covered column TOGETHER WITH its index's key column is safe and must be
+ /// allowed: `retain_relevant_indices` drops the whole index once its key leaves the
+ /// schema, so no covering metadata can dangle. The commit-boundary guard
+ /// (`reject_covered_field_subtree_change`) already skips such indices; this preflight
+ /// must agree with it, or it forbids an operation the commit would accept.
+ ///
+ /// The `key_and_covering_subset` case is what makes the two agree in general. The
+ /// preflight used to skip an index only when *every* id in `fields` was being dropped,
+ /// while the commit guard skips as soon as a *keyed* id is. With two covering columns
+ /// and only one of them dropped alongside the key, those disagree: the commit accepts
+ /// (the key is gone, so the whole index goes) and the preflight rejected.
+ #[rstest]
+ #[case::key_and_all_covering(&["vec", "id", "extra"], vec!["keep"])]
+ #[case::key_and_covering_subset(&["vec", "id"], vec!["extra", "keep"])]
+ #[tokio::test]
+ async fn test_drop_covered_column_with_its_index_key_is_allowed(
+ #[case] to_drop: &[&str],
+ #[case] expected_remaining: Vec<&str>,
+ ) -> Result<()> {
+ use lance_arrow::FixedSizeListArrayExt;
+ use lance_index::IndexType;
+ use lance_linalg::distance::MetricType;
+ use lance_testing::datagen::generate_random_array;
+
+ use crate::index::vector::VectorIndexParams;
+
+ let schema = Arc::new(ArrowSchema::new(vec![
+ ArrowField::new(
+ "vec",
+ DataType::FixedSizeList(
+ Arc::new(ArrowField::new("item", DataType::Float32, true)),
+ 64,
+ ),
+ false,
+ ),
+ ArrowField::new("id", DataType::Int32, false),
+ ArrowField::new("extra", DataType::Int32, false),
+ ArrowField::new("keep", DataType::Int32, false),
+ ]));
+ let nrows: i32 = 256;
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(
+ ::try_new_from_values(
+ generate_random_array(64 * nrows as usize),
+ 64,
+ )
+ .unwrap(),
+ ),
+ Arc::new(Int32Array::from_iter_values(0..nrows)),
+ Arc::new(Int32Array::from_iter_values(0..nrows)),
+ Arc::new(Int32Array::from_iter_values(0..nrows)),
+ ],
+ )?;
+
+ let test_dir = TempStrDir::default();
+ let mut dataset = Dataset::write(
+ RecordBatchIterator::new(vec![Ok(batch)], schema.clone()),
+ &test_dir,
+ None,
+ )
+ .await?;
+
+ let mut params = VectorIndexParams::ivf_pq(4, 8, 8, MetricType::L2, 50);
+ params.covering_columns(vec!["id".to_string(), "extra".to_string()]);
+ dataset
+ .create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await?;
+
+ // Drop the index key together with its covering column(s) in one call.
+ dataset.drop_columns(to_drop).await?;
+
+ assert!(
+ dataset.load_indices().await?.is_empty(),
+ "the index must be pruned once its key column is gone"
+ );
+ let remaining: Vec<&str> = dataset
+ .schema()
+ .fields
+ .iter()
+ .map(|f| f.name.as_str())
+ .collect();
+ assert_eq!(remaining, expected_remaining);
+ Ok(())
+ }
+
+ /// Dropping a column that an index *covers* (an "included"/covering column,
+ /// not the key column) must fail fast. The covering id lives in `fields`, so
+ /// letting the drop through would make `retain_relevant_indices` delete the
+ /// ENTIRE index -- key, ANN structure and all -- not merely desync its
+ /// covering. The user must drop the index first.
+ #[tokio::test]
+ async fn test_drop_columns_fails_on_covered_column() -> Result<()> {
+ use lance_arrow::FixedSizeListArrayExt;
+ use lance_index::IndexType;
+ use lance_linalg::distance::MetricType;
+ use lance_testing::datagen::generate_random_array;
+
+ use crate::index::vector::VectorIndexParams;
+
+ let schema = Arc::new(ArrowSchema::new(vec![
+ ArrowField::new(
+ "vec",
+ DataType::FixedSizeList(
+ Arc::new(ArrowField::new("item", DataType::Float32, true)),
+ 64,
+ ),
+ false,
+ ),
+ ArrowField::new("id", DataType::Int32, false),
+ ]));
+ let nrows: i32 = 256;
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(
+ ::try_new_from_values(
+ generate_random_array(64 * nrows as usize),
+ 64,
+ )
+ .unwrap(),
+ ),
+ Arc::new(Int32Array::from_iter_values(0..nrows)),
+ ],
+ )?;
+
+ let test_dir = TempStrDir::default();
+ let mut dataset = Dataset::write(
+ RecordBatchIterator::new(vec![Ok(batch)], schema.clone()),
+ &test_dir,
+ None,
+ )
+ .await?;
+
+ // IVF_PQ index on `vec` that COVERS `id`.
+ let mut params = VectorIndexParams::ivf_pq(4, 8, 8, MetricType::L2, 50);
+ params.covering_columns(vec!["id".to_string()]);
+ dataset
+ .create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await?;
+ let index_name = dataset.load_indices().await?[0].name.clone();
+
+ // Dropping the covered column must fail, naming the column, the index,
+ // and the remediation.
+ let err = dataset
+ .drop_columns(&["id"])
+ .await
+ .expect_err("dropping a covered column should fail");
+ assert!(
+ matches!(err, Error::InvalidInput { .. }),
+ "expected Error::InvalidInput, got: {err:?}"
+ );
+ let msg = err.to_string();
+ // `[id]` (the rendered column list), not a bare `id`: the auto-generated index name
+ // is `vec_idx`, which contains `id` as a substring, so a bare `contains("id")` would
+ // pass even if the message never named the column.
+ assert!(
+ msg.contains("[id]") && msg.contains(&index_name) && msg.contains("drop_index"),
+ "error should mention the column, index, and remediation, got: {msg}"
+ );
+
+ // Unchanged: column still present, index still there.
+ assert!(dataset.schema().field("id").is_some());
+ assert_eq!(dataset.load_indices().await?.len(), 1);
+
+ // After dropping the index, the same drop succeeds.
+ dataset.drop_index(&index_name).await?;
+ dataset.drop_columns(&["id"]).await?;
+ assert!(dataset.schema().field("id").is_none());
+
+ Ok(())
+ }
+
+ /// Dropping a column that is only an INDEXED key column (not a covering
+ /// column) must remain allowed: lance auto-drops the dependent index. Only
+ /// a covering ("included") column dropped *while its index's key survives*
+ /// is protected by the drop guard: dropping the key removes the whole
+ /// index either way (via `retain_relevant_indices`), so there is nothing to
+ /// protect, whereas dropping just the covering column would otherwise
+ /// silently take the whole index down with it (see `drop_columns`).
+ /// Regression guard for the Python `test_drop_columns` behavior.
+ #[tokio::test]
+ async fn test_drop_indexed_noncovered_column_drops_index() -> Result<()> {
+ use crate::index::vector::VectorIndexParams;
+ use lance_arrow::FixedSizeListArrayExt;
+ use lance_index::IndexType;
+ use lance_linalg::distance::MetricType;
+ use lance_testing::datagen::generate_random_array;
+
+ let schema = Arc::new(ArrowSchema::new(vec![
+ ArrowField::new(
+ "vec",
+ DataType::FixedSizeList(
+ Arc::new(ArrowField::new("item", DataType::Float32, true)),
+ 64,
+ ),
+ false,
+ ),
+ ArrowField::new("id", DataType::Int32, false),
+ ]));
+ let nrows: i32 = 256;
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(
+ ::try_new_from_values(
+ generate_random_array(64 * nrows as usize),
+ 64,
+ )
+ .unwrap(),
+ ),
+ Arc::new(Int32Array::from_iter_values(0..nrows)),
+ ],
+ )?;
+ let test_dir = TempStrDir::default();
+ let mut dataset = Dataset::write(
+ RecordBatchIterator::new(vec![Ok(batch)], schema.clone()),
+ &test_dir,
+ None,
+ )
+ .await?;
+
+ // Plain IVF_PQ index on `vec` -- NO covering columns.
+ let params = VectorIndexParams::ivf_pq(4, 8, 8, MetricType::L2, 50);
+ dataset
+ .create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await?;
+ assert_eq!(dataset.load_indices().await?.len(), 1);
+
+ // Dropping the indexed key column is allowed and auto-drops the index.
+ dataset.drop_columns(&["vec"]).await?;
+ assert!(dataset.schema().field("vec").is_none());
+ assert_eq!(
+ dataset.load_indices().await?.len(),
+ 0,
+ "dropping the indexed column should have dropped its index"
+ );
+
+ Ok(())
+ }
+
+ /// Fix 1: renaming or changing the nullability of a covered ("included")
+ /// column must fail fast (alongside cast) -- it would leave the index storage
+ /// referencing a stale name/schema and break covered queries.
+ #[tokio::test]
+ async fn test_alter_columns_rename_nullable_fail_on_covered_column() -> Result<()> {
+ use crate::index::vector::VectorIndexParams;
+ use lance_arrow::FixedSizeListArrayExt;
+ use lance_index::IndexType;
+ use lance_linalg::distance::MetricType;
+ use lance_testing::datagen::generate_random_array;
+
+ let schema = Arc::new(ArrowSchema::new(vec![
+ ArrowField::new(
+ "vec",
+ DataType::FixedSizeList(
+ Arc::new(ArrowField::new("item", DataType::Float32, true)),
+ 64,
+ ),
+ false,
+ ),
+ ArrowField::new("id", DataType::Int32, false),
+ ]));
+ let nrows: i32 = 256;
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(
+ ::try_new_from_values(
+ generate_random_array(64 * nrows as usize),
+ 64,
+ )
+ .unwrap(),
+ ),
+ Arc::new(Int32Array::from_iter_values(0..nrows)),
+ ],
+ )?;
+ let test_dir = TempStrDir::default();
+ let mut dataset = Dataset::write(
+ RecordBatchIterator::new(vec![Ok(batch)], schema.clone()),
+ &test_dir,
+ None,
+ )
+ .await?;
+
+ let mut params = VectorIndexParams::ivf_pq(4, 8, 8, MetricType::L2, 50);
+ params.covering_columns(vec!["id".to_string()]);
+ dataset
+ .create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await?;
+
+ // Rename of the covered column must fail with the remediation hint.
+ let err = dataset
+ .alter_columns(&[ColumnAlteration::new("id".into()).rename("uid".into())])
+ .await
+ .expect_err("rename of covered column should fail");
+ assert!(
+ matches!(err, Error::InvalidInput { .. }),
+ "expected Error::InvalidInput, got: {err:?}"
+ );
+ assert!(
+ err.to_string().contains("drop_index"),
+ "rename error should suggest drop_index, got: {err}"
+ );
+ assert!(
+ dataset.schema().field("id").is_some(),
+ "rename should not have applied"
+ );
+
+ // Nullability change of the covered column must also fail.
+ let err = dataset
+ .alter_columns(&[ColumnAlteration::new("id".into()).set_nullable(true)])
+ .await
+ .expect_err("nullability change of covered column should fail");
+ assert!(
+ matches!(err, Error::InvalidInput { .. }),
+ "expected Error::InvalidInput, got: {err:?}"
+ );
+ assert!(
+ err.to_string().contains("drop_index"),
+ "nullable error should suggest drop_index, got: {err}"
+ );
+
+ Ok(())
+ }
+
#[test]
fn test_is_upcast_downcast_dictionary() {
use DataType::*;
diff --git a/rust/lance/src/dataset/take.rs b/rust/lance/src/dataset/take.rs
index ddb76a0f720..971d1d189f4 100644
--- a/rust/lance/src/dataset/take.rs
+++ b/rust/lance/src/dataset/take.rs
@@ -530,6 +530,19 @@ impl TakeBuilder {
self
}
+ /// Add the row id to the output *without* requesting it as a projected column.
+ ///
+ /// Naming `_rowid` in a [`ProjectionRequest`] also sets `must_add_row_offset`
+ /// (see `ProjectionPlan::from_schema`), which arms the "must not target deleted
+ /// rows" check in [`do_take_rows`] and turns a take that legitimately returns
+ /// fewer rows than requested into an error. It also computes a row-offset column
+ /// per batch. A caller that wants the ids only to realign its own output against
+ /// the rows that actually came back needs neither, so it uses this instead.
+ pub(crate) fn include_row_id(mut self) -> Self {
+ Arc::make_mut(&mut self.projection).include_row_id();
+ self
+ }
+
pub(super) fn with_missing_row_policy(mut self, policy: MissingRowPolicy) -> Self {
self.missing_row_policy = policy;
self
diff --git a/rust/lance/src/dataset/tests/dataset_index.rs b/rust/lance/src/dataset/tests/dataset_index.rs
index 19673e83ae1..e4eddb6a321 100644
--- a/rust/lance/src/dataset/tests/dataset_index.rs
+++ b/rust/lance/src/dataset/tests/dataset_index.rs
@@ -13,7 +13,7 @@ use crate::dataset::WriteDestination;
use crate::dataset::builder::DatasetBuilder;
use crate::dataset::tests::dataset_migrations::scan_dataset;
use crate::dataset::tests::dataset_transactions::{assert_results, execute_sql};
-use crate::dataset::transaction::{DataReplacementGroup, Operation, Transaction};
+use crate::dataset::transaction::{DataReplacementGroup, Operation};
use crate::index::vector::VectorIndexParams;
use crate::session::Session;
use crate::utils::test::covering;
@@ -21,7 +21,7 @@ use crate::{Dataset, Error, Result};
use lance_arrow::FixedSizeListArrayExt;
use crate::dataset::write::{WriteMode, WriteParams};
-use crate::index::DatasetIndexExt;
+use crate::index::{CreateIndexBuilder, DatasetIndexExt};
use arrow::array::{AsArray, GenericListBuilder, GenericStringBuilder};
use arrow::datatypes::UInt64Type;
use arrow_array::RecordBatch;
@@ -56,6 +56,8 @@ use lance_index::{IndexType, scalar::ScalarIndexParams, vector::DIST_COL};
use lance_io::scheduler::{ScanScheduler, SchedulerConfig};
use lance_io::utils::CachedFileSize;
use lance_linalg::distance::MetricType;
+use lance_table::format::DataFile;
+use object_store::path::Path;
use datafusion::common::{assert_contains, assert_not_contains};
use futures::{StreamExt, TryStreamExt};
@@ -64,7 +66,7 @@ use lance_arrow::json::ARROW_JSON_EXT_NAME;
use lance_index::scalar::inverted::query::{FtsQuery, MultiMatchQuery};
use lance_table::format::BasePath;
use lance_testing::datagen::generate_random_array;
-use rand::Rng;
+use rand::{Rng, SeedableRng, rngs::StdRng};
use rstest::rstest;
#[rstest]
@@ -225,6 +227,125 @@ async fn test_covered_vector_column_is_not_selected_for_ann() {
);
}
+/// The entire point of a covered IVF_PQ index: a query whose projection is
+/// satisfied by the keyed vector column's own `_distance`/`_rowid` plus the
+/// index's declared `covering_fields` must be answered from the index alone,
+/// with no take against the base table. A query that also needs a column the
+/// index does not carry must still take.
+///
+/// Two traps established empirically on this project make a broken
+/// take-elision path look correct anyway:
+/// - `num_partitions = 1` never reaches the probe path at all.
+/// - Even 4 partitions is not enough with uniform-random vectors: the
+/// early-pruning heuristic (`early_pruning` in `io/exec/knn.rs`) still
+/// searches every partition, so `late_search`'s split is never
+/// exercised. The data must have well-separated clusters, and
+/// `early_pruning` must stay off.
+/// So this test uses 4 well-separated clusters and asserts on the plan the
+/// scanner actually built (`explain_plan`), never on query results -- a take
+/// can run and simply return the same values.
+#[tokio::test]
+async fn test_covered_ann_query_elides_base_table_take() {
+ const DIMS: usize = 16;
+ const NUM_CLUSTERS: usize = 4;
+ const ROWS_PER_CLUSTER: usize = 64;
+ const TOTAL: usize = NUM_CLUSTERS * ROWS_PER_CLUSTER;
+
+ let test_uri = TempStrDir::default();
+
+ // Well-separated clusters (centers 1000 apart along dim 0), not
+ // uniform-random vectors -- see the module doc above.
+ let mut rng = StdRng::seed_from_u64(42);
+ let mut payload = Vec::with_capacity(TOTAL);
+ let mut extra = Vec::with_capacity(TOTAL);
+ let mut values = Vec::with_capacity(TOTAL * DIMS);
+ for cluster in 0..NUM_CLUSTERS {
+ let center = (cluster * 1000) as f32;
+ for row in 0..ROWS_PER_CLUSTER {
+ let row_id = (cluster * ROWS_PER_CLUSTER + row) as i32;
+ payload.push(row_id);
+ extra.push(row_id * 2);
+ for dim in 0..DIMS {
+ let base = if dim == 0 { center } else { 0.0 };
+ values.push(base + (rng.random::() - 0.5) * 0.02);
+ }
+ }
+ }
+
+ let vectors: ArrayRef = Arc::new(
+ ::try_new_from_values(
+ Float32Array::from(values),
+ DIMS as i32,
+ )
+ .unwrap(),
+ );
+ let schema = Arc::new(ArrowSchema::new(vec![
+ ArrowField::new("vector", vectors.data_type().clone(), false),
+ ArrowField::new("payload", DataType::Int32, false),
+ ArrowField::new("extra", DataType::Int32, false),
+ ]));
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ vectors,
+ Arc::new(Int32Array::from(payload)),
+ Arc::new(Int32Array::from(extra)),
+ ],
+ )
+ .unwrap();
+ let reader = RecordBatchIterator::new(vec![batch].into_iter().map(Ok), schema);
+ let mut dataset = Dataset::write(reader, &test_uri, None).await.unwrap();
+
+ // 4 partitions, matching the cluster count: the covering columns'
+ // reachability doesn't hinge on which internal probe path runs, but a
+ // covered-ANN test with too few effectively-reachable partitions has
+ // already once passed against broken code on this project (see doc
+ // above), so this follows the same defensive recipe regardless.
+ let mut params = VectorIndexParams::ivf_pq(NUM_CLUSTERS, 8, 4, MetricType::L2, 2);
+ params.covering_columns(vec!["payload".to_string()]);
+ dataset
+ .create_index(&["vector"], IndexType::Vector, None, ¶ms, true)
+ .await
+ .unwrap();
+
+ // Query near the last cluster's center.
+ let mut q_values = vec![0.0f32; DIMS];
+ q_values[0] = ((NUM_CLUSTERS - 1) * 1000) as f32;
+ let query = Float32Array::from(q_values);
+
+ // Covered case: the projection (`payload`) is entirely satisfied by the
+ // index's own output columns -- the plan must contain no base-table take.
+ // A base-table take on this (stable/v2) storage format is a
+ // `FilteredReadExec`, displayed as `LanceRead: ...`; checking for the
+ // literal substring "Take" would be a no-op here, since that name is
+ // only ever used by the legacy (v1) `TakeExec`.
+ let mut scan = dataset.scan();
+ scan.nearest("vector", &query, 10).unwrap();
+ scan.project(&["payload"]).unwrap();
+ let plan = scan.explain_plan(true).await.unwrap();
+ assert!(
+ !plan.contains("LanceRead"),
+ "a covered projection ['payload'] must skip the base-table take entirely:\n{plan}"
+ );
+ let batch = scan.try_into_batch().await.unwrap();
+ assert!(batch.column_by_name("payload").is_some());
+
+ // Uncovered case: `extra` is not a declared covering column, so the
+ // scanner must take it from the base table. `source=stream` is the
+ // marker for a row-id-driven take (as opposed to a full-table scan),
+ // which is what a base-table take against ANN results looks like.
+ let mut scan = dataset.scan();
+ scan.nearest("vector", &query, 10).unwrap();
+ scan.project(&["extra"]).unwrap();
+ let plan = scan.explain_plan(true).await.unwrap();
+ assert!(
+ plan.contains("LanceRead") && plan.contains("source=stream"),
+ "a projection needing the uncovered column 'extra' must take from the base table:\n{plan}"
+ );
+ let batch = scan.try_into_batch().await.unwrap();
+ assert!(batch.column_by_name("extra").is_some());
+}
+
/// A filtered `describe_indices` must still find a covered index by its keyed
/// column. The matcher compares the caller's resolved field slice against the one
/// column named by `for_column`, so a caller that passes all of `index.fields` --
@@ -273,85 +394,23 @@ async fn test_describe_indices_filters_a_covered_index_by_its_keyed_field() {
);
}
-/// An unfiltered `optimize_indices()` is a table-wide request, so a covered index
-/// must not abort it -- that would block optimization of every other index on the
-/// table over one this build merely cannot rebuild. It is skipped with a warning.
-/// Only a caller that names the covered index gets an error (see
-/// `test_optimize_indices_rejects_a_covered_index`).
+/// The scalar counterpart to the covered-vector optimize case above. Before a producer
+/// existed optimize skipped a covered group outright; the producer makes covered rebuilds
+/// real, so what needs pinning is that rebuilding a covered *scalar* group preserves its
+/// covering declaration rather than silently dropping it. A carried column is inert to
+/// every scalar read path, so a lost declaration would stay invisible until a covered
+/// planner consulted it.
///
-/// Both the current and the stale case are covered: erroring on the stale one
-/// aborts the loop before the replacements accumulated for the other groups are
-/// committed, leaving an unrelated index stale too.
-#[rstest]
-#[case::current(false)]
-#[case::stale(true)]
-#[tokio::test]
-async fn test_optimize_skips_a_covered_index_without_blocking_others(#[case] stale: bool) {
- let test_uri = TempStrDir::default();
- let mut dataset = covering::write_vector_payload_dataset(&test_uri).await;
-
- // A covered vector index, and a plain scalar index that optimize may touch.
- covering::create_ivf_pq_index(&mut dataset, "vec").await;
- covering::create_btree_index(&mut dataset, "payload", Some("payload_idx")).await;
-
- let (_, payload_id) = covering::declare_covering(&mut dataset, "vec", "payload").await;
- let covered_uuid = dataset
- .load_indices()
- .await
- .unwrap()
- .iter()
- .find(|idx| !idx.covering_fields.is_empty())
- .expect("the covered index should exist")
- .uuid;
-
- if stale {
- // Now *both* groups have an unindexed fragment, so the covered group
- // would genuinely be rebuilt -- this is where the refusal used to fire.
- covering::append_vector_payload_rows(&mut dataset, 256).await;
- }
-
- dataset
- .optimize_indices(&OptimizeOptions::default())
- .await
- .expect("a covered index must not abort an unfiltered optimize");
-
- let after = dataset.load_indices().await.unwrap();
- assert!(
- after
- .iter()
- .any(|idx| idx.uuid == covered_uuid && idx.covering_fields == vec![payload_id]),
- "the covered index must be left exactly as it was"
- );
-
- if stale {
- let payload_idx = after
- .iter()
- .filter(|idx| idx.name == "payload_idx")
- .filter_map(|idx| idx.fragment_bitmap.as_ref())
- .fold(roaring::RoaringBitmap::new(), |mut acc, bitmap| {
- acc |= bitmap;
- acc
- });
- assert!(
- payload_idx.contains(1),
- "the unrelated index must still have been optimized onto the new fragment, got {payload_idx:?}"
- );
- } else {
- assert_eq!(after.len(), 2, "both indices must survive");
- }
-}
-
-/// The same skip, for a *scalar* covered index. The rule does not branch on index
-/// type, but scalar groups take their own no-work path a few lines below the
-/// covering gate, so a covered scalar index reaching that gate first is worth
-/// pinning separately from the vector case above.
+/// The append is what gives this test teeth: with no unindexed rows the covered group has
+/// no work, and the assertions would hold with the rebuild never happening.
///
-/// The append is what gives this test teeth. Without it the covered group has no
-/// work either way, so the scalar no-work path below the gate produces the same
-/// observable outcome as the gate itself and the test passes with the gate
-/// removed entirely.
+/// The reload right before `optimize_indices` is load-bearing. Without it optimize
+/// intermittently commits a manifest missing one of the two groups. That race needs no
+/// covering to reproduce -- two plain btree indexes over an append are enough -- so it is
+/// not this branch's to fix, and the reload keeps this test measuring covering rather than
+/// that race.
#[tokio::test]
-async fn test_optimize_skips_a_stale_covered_scalar_index() {
+async fn test_optimize_rebuild_preserves_a_covered_scalar_declaration() {
let test_uri = TempStrDir::default();
let mut dataset = covering::write_three_int_column_dataset(&test_uri).await;
covering::create_btree_index(&mut dataset, "a", None).await;
@@ -367,9 +426,10 @@ async fn test_optimize_skips_a_stale_covered_scalar_index() {
.expect("the covered index should exist")
.uuid;
- // Both scalar groups now have an unindexed fragment, so the covered one
- // would genuinely be rebuilt if the gate did not skip it first.
+ // Both scalar groups now have an unindexed fragment, so the covered one is genuinely
+ // rebuilt rather than reaching a no-work path.
covering::append_three_int_column_rows(&mut dataset, 64).await;
+ dataset.load_indices().await.unwrap();
dataset
.optimize_indices(&OptimizeOptions::default())
@@ -377,14 +437,36 @@ async fn test_optimize_skips_a_stale_covered_scalar_index() {
.expect("a stale covered scalar index must not abort optimize");
let after = dataset.load_indices().await.unwrap();
+ let covered = after
+ .iter()
+ .filter(|idx| !idx.covering_fields.is_empty())
+ .collect::>();
+ assert_eq!(
+ covered.len(),
+ 1,
+ "exactly one index must still declare covering, got {covered:?}"
+ );
+ assert_eq!(
+ covered[0].covering_fields,
+ vec![carried_id],
+ "the rebuild must carry the covering declaration forward, not drop it"
+ );
+ assert_ne!(
+ covered[0].uuid, covered_uuid,
+ "precondition: the covered group must actually have been rebuilt, or this test \
+ would pass with the rebuild path never entered"
+ );
+ let covered_coverage = covered[0]
+ .fragment_bitmap
+ .as_ref()
+ .expect("a rebuilt index records its coverage");
assert!(
- after
- .iter()
- .any(|idx| idx.uuid == covered_uuid && idx.covering_fields == vec![carried_id]),
- "the covered scalar index must be left exactly as it was, not rebuilt"
+ covered_coverage.contains(1),
+ "the rebuilt covered index must span the appended fragment, got {covered_coverage:?}"
);
- // The unrelated scalar index was still maintained, so the skip is scoped to
- // the covered group rather than aborting the loop.
+
+ // The unrelated scalar index was still maintained, so the rebuild is scoped to the
+ // covered group rather than aborting the loop.
let b_id = dataset.schema().field_id("b").unwrap();
let b_coverage = after
.iter()
@@ -400,60 +482,6 @@ async fn test_optimize_skips_a_stale_covered_scalar_index() {
);
}
-/// A caller that names the covered index asked for it specifically, so the
-/// refusal is loud rather than a skip.
-#[tokio::test]
-async fn test_optimize_indices_rejects_a_covered_index() {
- let test_uri = TempStrDir::default();
- let mut dataset = covering::write_vector_payload_dataset(&test_uri).await;
- covering::create_ivf_pq_index(&mut dataset, "vec").await;
-
- // Nothing writes carried values, so the storage does not contain `payload`
- // -- which is exactly why optimize must refuse: it rebuilds from a scan
- // projecting the keyed field and `_rowid` only, and would republish the
- // declaration on a segment that still has no payload.
- let (vec_id, payload_id) = covering::declare_covering(&mut dataset, "vec", "payload").await;
-
- // Append AFTER declaring covering, so the group really would be rebuilt.
- // Without it this would assert the refusal against an index optimize had no
- // work for, and would keep passing if the refusal moved behind a no-work
- // check.
- covering::append_vector_payload_rows(&mut dataset, 256).await;
-
- let before = dataset.load_indices().await.unwrap();
- let before_uuid = before[0].uuid;
- let covered_name = before[0].name.clone();
-
- // Name the covered index: the refusal is reserved for a caller that targeted
- // it. An unfiltered call skips it instead, which
- // `test_optimize_skips_a_covered_index_without_blocking_others` covers.
- let err = dataset
- .optimize_indices(&OptimizeOptions::default().index_names(vec![covered_name]))
- .await
- .expect_err("optimizing a targeted covered index must be refused");
- assert!(
- err.to_string().contains("declares covering fields"),
- "unexpected message: {err}"
- );
-
- // Refused, not partially applied: the index is exactly as it was.
- let after = dataset.load_indices().await.unwrap();
- assert_eq!(after.len(), 1);
- assert_eq!(
- after[0].uuid, before_uuid,
- "a refused optimize must not replace the index"
- );
- assert_eq!(after[0].covering_fields, vec![payload_id]);
- assert_eq!(after[0].fields, vec![vec_id, payload_id]);
-
- // The appended data above is unindexed, so this really is a case optimize
- // would otherwise have merged -- the refusal is not the no-op path.
- assert!(
- after.iter().all(|idx| !idx.covering_fields.is_empty()),
- "precondition: the only index is still the covered one"
- );
-}
-
#[rstest]
#[tokio::test]
async fn test_create_scalar_index(
@@ -7448,17 +7476,91 @@ async fn test_load_segment_params_full_fidelity() {
assert_eq!(&read, opened.params());
}
-/// Compaction of a covered index must succeed. `remap_index` rejected any
-/// index with more than one field, so this failed outright -- a covered
-/// dataset could be created but never compacted.
-#[tokio::test]
-async fn test_compaction_withdraws_a_covered_index_without_failing() {
- use crate::dataset::optimize::{CompactionOptions, compact_files};
+/// Write `values` as a standalone single-field `payload` data file (field `payload_id`)
+/// and return the resulting [`DataFile`], ready to use in an `Operation::DataReplacement`
+/// group. Kept in its own file from `vec` throughout this test so replacing it is a
+/// straightforward single-field swap, never a split of a shared file.
+async fn write_payload_data_file(
+ dataset: &Dataset,
+ name: &str,
+ payload_id: i32,
+ values: Int32Array,
+) -> DataFile {
+ let payload_schema = Arc::new(ArrowSchema::new(vec![ArrowField::new(
+ "payload",
+ DataType::Int32,
+ true,
+ )]));
+ let path = format!("{name}.lance");
+ let object_writer = dataset
+ .object_store
+ .create(&Path::from(format!("data/{path}")))
+ .await
+ .unwrap();
+ let mut writer = lance_file::versions::v2_1::create_writer(
+ object_writer,
+ payload_schema.as_ref().try_into().unwrap(),
+ Default::default(),
+ )
+ .unwrap();
+ let write_batch = RecordBatch::try_new(payload_schema, vec![Arc::new(values)]).unwrap();
+ writer.write_batch(&write_batch).await.unwrap();
+ writer.finish().await.unwrap();
- let test_uri = TempStrDir::default();
+ let (major, minor) = LanceFileVersion::Stable.resolve().to_data_file_numbers();
+ DataFile {
+ path,
+ fields: Arc::from([payload_id]),
+ column_indices: Arc::from([0]),
+ file_major_version: major,
+ file_minor_version: minor,
+ file_size_bytes: CachedFileSize::unknown(),
+ base_id: None,
+ }
+}
+
+/// `prune_stale_segment_coverage` must judge staleness from every field a segment
+/// declares -- keyed and carried alike -- not just the keyed field's subtree.
+///
+/// Before the fix, only the keyed (`vec`) field fed the staleness check: a fragment
+/// whose *carried* (`payload`) column was physically rewritten between when the
+/// segment was built and when it is committed stayed marked "covered" regardless, so
+/// the committed index would go on serving that fragment's now-stale carried values.
+/// `dataset/overlay.rs`'s per-read staleness check already considers every declared
+/// field; this is the build-time counterpart disagreeing with it.
+#[tokio::test]
+async fn test_commit_existing_index_segments_prunes_fragment_with_rewritten_carried_column() {
+ // An in-memory store, not `TempStrDir`: `write_payload_data_file` writes bare
+ // relative paths (matching this crate's other `DataReplacement` tests), which for
+ // a `file://` store would resolve against the filesystem root rather than the
+ // dataset's own directory.
+ let test_uri = "memory://";
let dimension = 16;
- let schema = Arc::new(ArrowSchema::new(vec![
+ let vec_schema = Arc::new(ArrowSchema::new(vec![ArrowField::new(
+ "vec",
+ DataType::FixedSizeList(
+ Arc::new(ArrowField::new("item", DataType::Float32, true)),
+ dimension,
+ ),
+ false,
+ )]));
+ let vectors = Arc::new(
+ ::try_new_from_values(
+ generate_random_array(512 * dimension as usize),
+ dimension,
+ )
+ .unwrap(),
+ );
+ let batch = RecordBatch::try_new(vec_schema.clone(), vec![vectors]).unwrap();
+ let reader = RecordBatchIterator::new(vec![Ok(batch)], vec_schema.clone());
+ let dataset = Dataset::write(reader, test_uri, None).await.unwrap();
+
+ // Add `payload` as an all-null column. It stays fileless until populated below, so
+ // it never shares a data file with `vec` -- a later replacement of just its file is
+ // then a straightforward single-field swap, not a split of a multi-field file (which
+ // `DataReplacement` does not support).
+ let extended_schema = Arc::new(ArrowSchema::new(vec![
ArrowField::new(
"vec",
DataType::FixedSizeList(
@@ -7467,81 +7569,105 @@ async fn test_compaction_withdraws_a_covered_index_without_failing() {
),
false,
),
- ArrowField::new("payload", DataType::Int32, false),
+ ArrowField::new("payload", DataType::Int32, true),
]));
+ let fragment = dataset.get_fragments().pop().unwrap().metadata;
+ let read_version = dataset.manifest.version;
+ let dataset = Dataset::commit(
+ WriteDestination::Dataset(Arc::new(dataset)),
+ Operation::Merge {
+ fragments: vec![fragment],
+ schema: extended_schema.as_ref().try_into().unwrap(),
+ preserves_nullability: true,
+ },
+ Some(read_version),
+ None,
+ None,
+ Arc::new(Default::default()),
+ false,
+ )
+ .await
+ .unwrap();
+ let payload_id = dataset.schema().field_id("payload").unwrap();
- let make_batch = |offset: i32| {
- let vectors = Arc::new(
- ::try_new_from_values(
- generate_random_array(256 * dimension as usize),
- dimension,
- )
- .unwrap(),
- );
- let payload = Arc::new(Int32Array::from_iter_values(offset..offset + 256));
- RecordBatch::try_new(schema.clone(), vec![vectors, payload]).unwrap()
- };
-
- // Two fragments, so compaction has something to compact.
- let reader = RecordBatchIterator::new(vec![Ok(make_batch(0))], schema.clone());
- let mut dataset = Dataset::write(reader, &test_uri, None).await.unwrap();
- let reader = RecordBatchIterator::new(vec![Ok(make_batch(256))], schema.clone());
- dataset.append(reader, None).await.unwrap();
+ // Give `payload` its first real values, in a data file of its own.
+ let payload_v1 = write_payload_data_file(
+ &dataset,
+ "payload_v1",
+ payload_id,
+ Int32Array::from_iter_values(0..512),
+ )
+ .await;
+ let read_version = dataset.manifest.version;
+ let mut dataset = Dataset::commit(
+ WriteDestination::Dataset(Arc::new(dataset)),
+ Operation::DataReplacement {
+ replacements: vec![DataReplacementGroup(0, payload_v1)],
+ },
+ Some(read_version),
+ None,
+ None,
+ Arc::new(Default::default()),
+ false,
+ )
+ .await
+ .unwrap();
- let params = VectorIndexParams::ivf_pq(4, 8, 2, MetricType::L2, 50);
- dataset
- .create_index(&["vec"], IndexType::Vector, None, ¶ms, true)
+ // Build a covered segment (Task 1's `covering_columns`) while `payload` still holds
+ // its first values, but do not commit it yet. This mirrors a distributed build,
+ // whose segment metadata records the dataset version it was built from.
+ let mut params = VectorIndexParams::ivf_pq(4, 8, 2, MetricType::L2, 50);
+ params.covering_columns(vec!["payload".to_string()]);
+ let built = CreateIndexBuilder::new(&mut dataset, &["vec"], IndexType::Vector, ¶ms)
+ .name("vec_idx".to_string())
+ .execute_uncommitted()
.await
.unwrap();
let vec_id = dataset.schema().field_id("vec").unwrap();
- let payload_id = dataset.schema().field_id("payload").unwrap();
- let current = dataset.load_indices().await.unwrap();
- let mut covered = current[0].clone();
- covered.fields = vec![vec_id, payload_id];
- covered.covering_fields = vec![payload_id];
-
- let transaction = Transaction::new(
- dataset.manifest.version,
- Operation::CreateIndex {
- new_indices: vec![covered],
- removed_indices: current.to_vec(),
+ assert_eq!(built.fields, vec![vec_id, payload_id]);
+ assert_eq!(built.covering_fields, vec![payload_id]);
+ assert!(
+ built.fragment_bitmap.as_ref().unwrap().contains(0),
+ "sanity: the segment must cover fragment 0 before any rewrite"
+ );
+
+ // Rewrite the *carried* `payload` column's data file -- `vec`'s file is untouched --
+ // advancing the dataset past the version the segment above was built at.
+ let payload_v2 = write_payload_data_file(
+ &dataset,
+ "payload_v2",
+ payload_id,
+ Int32Array::from_iter_values(1000..1512),
+ )
+ .await;
+ let read_version = dataset.manifest.version;
+ let mut dataset = Dataset::commit(
+ WriteDestination::Dataset(Arc::new(dataset)),
+ Operation::DataReplacement {
+ replacements: vec![DataReplacementGroup(0, payload_v2)],
},
+ Some(read_version),
None,
- );
+ None,
+ Arc::new(Default::default()),
+ false,
+ )
+ .await
+ .unwrap();
+
dataset
- .apply_commit(transaction, &Default::default(), &Default::default())
+ .commit_existing_index_segments("vec_idx", "vec", vec![built])
.await
.unwrap();
- let fragments_before: Vec = dataset.fragments().iter().map(|f| f.id).collect();
- assert!(
- fragments_before.len() > 1,
- "precondition: there must be something to compact"
- );
-
- // Compaction of the table must not be blocked by an index it cannot remap.
- compact_files(&mut dataset, CompactionOptions::default(), None)
- .await
- .expect("compaction of a covered index must succeed");
-
- let fragments_after: Vec = dataset.fragments().iter().map(|f| f.id).collect();
- assert_ne!(
- fragments_after, fragments_before,
- "compaction rewrote nothing, so the remap path never ran"
- );
-
- // The entry survives untouched -- withdrawal skips remapping rather than
- // deleting metadata -- but it now covers none of the rewritten fragments, so
- // no query can be answered from a payload the storage never held.
- let after = dataset.load_indices().await.unwrap();
- assert_eq!(after.len(), 1);
- assert_eq!(after[0].covering_fields, vec![payload_id]);
- let live: roaring::RoaringBitmap = dataset.fragments().iter().map(|f| f.id as u32).collect();
- let effective = after[0].effective_fragment_bitmap(&live);
+ let committed = dataset.load_indices_by_name("vec_idx").await.unwrap();
+ assert_eq!(committed.len(), 1);
+ let coverage = committed[0].fragment_bitmap.as_ref().unwrap();
assert!(
- effective.is_none_or(|bitmap| bitmap.is_empty()),
- "a withdrawn covered index must stop covering fragments, got {:?}",
- after[0].fragment_bitmap
+ !coverage.contains(0),
+ "fragment 0's carried `payload` column was rewritten after the segment was built \
+ at dataset version {}; its coverage must be pruned, got {coverage:?}",
+ committed[0].dataset_version,
);
}
diff --git a/rust/lance/src/dataset/write/merge_insert.rs b/rust/lance/src/dataset/write/merge_insert.rs
index 7234e8baa0b..f02f1f24a3f 100644
--- a/rust/lance/src/dataset/write/merge_insert.rs
+++ b/rust/lance/src/dataset/write/merge_insert.rs
@@ -50,6 +50,7 @@ use super::{
use crate::dataset::rowids::get_row_id_index;
use crate::dataset::transaction::UpdateMode::{RewriteColumns, RewriteRows};
use crate::dataset::utils::CapturedRowIds;
+use crate::dataset::{ProjectionRequest, TakeBuilder};
use crate::index::DatasetIndexExt;
use crate::{
Dataset,
@@ -1520,6 +1521,337 @@ impl MergeInsertJob {
self.create_full_table_joined_stream(source).await
}
+ /// Names of `source_schema` columns this merge would update that are covered
+ /// ("included") by some index. Patching such a column in place makes the index's
+ /// materialized copy of it stale, which forces the commit to drop the whole updated
+ /// fragment from every index covering it; the partial-schema path therefore rewrites
+ /// those updates as row-moves, where it can, so that invalidation is confined to the
+ /// rows actually updated. See the branch in `execute_uncommitted_impl` for what that
+ /// does and does not buy, and `covered_move_blocker` for the cases that fall back to
+ /// the in-place patch. Join keys are matched, not updated, so they are excluded.
+ async fn covered_columns_updated(&self, source_schema: &Schema) -> Result> {
+ // Only the update-matched modes actually patch matched rows into fragments;
+ // DoNothing / Fail leave matched rows untouched (Delete is handled separately
+ // and never reaches this path). Without this gate, a partial source carrying a
+ // covered payload under the default DoNothing + InsertAll would be wrongly
+ // flagged and rejected even though no covered column is ever patched in place.
+ if !matches!(
+ self.params.when_matched,
+ WhenMatched::UpdateAll | WhenMatched::UpdateIf(_) | WhenMatched::UpdateIfExpr(_)
+ ) {
+ return Ok(Vec::new());
+ }
+ let indices = self.dataset.load_indices().await?;
+ let covered: HashSet = indices
+ .iter()
+ .flat_map(|idx| idx.covering_fields.iter().copied())
+ .collect();
+ if covered.is_empty() {
+ return Ok(Vec::new());
+ }
+ let target = self.dataset.schema();
+ let keys: HashSet<&str> = self.params.on.iter().map(|s| s.as_str()).collect();
+ let mut updated = Vec::new();
+ for field in source_schema.fields() {
+ let name = field.name();
+ if keys.contains(name.as_str()) {
+ continue;
+ }
+ if let Some(tf) = target.field(name)
+ && covered.contains(&tf.id)
+ {
+ updated.push(name.clone());
+ }
+ }
+ Ok(updated)
+ }
+
+ /// Why `rewrite_covered_update_as_move` cannot handle this merge, if it cannot.
+ ///
+ /// Every case below is a limitation of the *move*, never of the operation: the in-place
+ /// patch handles all of them, and it is correct for covered columns (see the caller --
+ /// the suffix-subset shape already drops the fragment from the index, so no stale copy
+ /// is reachable). The move is an optimisation, and an optimisation must not be able to
+ /// fail an operation that would otherwise succeed, so the caller logs this reason and
+ /// patches in place, forfeiting only confinement. Returns the reason so the log can name
+ /// it; the phrasing is a "because ..." clause.
+ fn covered_move_blocker(&self, source_schema: &Schema) -> Option {
+ // The move would corrupt row identity. Moving a row to a new fragment must carry its
+ // stable row id along, and the full-schema `RewriteRows` path does exactly that -- it
+ // rechunks the merger's captured `row_id_sequence` onto the new fragments and stamps
+ // `row_id_meta` (see the `updating_row_ids` block in `execute_uncommitted_impl`).
+ // `rewrite_covered_update_as_move` has no such step, so the commit would reach
+ // `Transaction::assign_row_ids` and mint *fresh* ids for every moved row, silently
+ // breaking the one guarantee stable row ids exist to provide. The in-place patch moves
+ // no rows at all, so it preserves those ids by construction -- strictly safer here than
+ // the move, not less safe.
+ if self.dataset.manifest.uses_stable_row_ids() {
+ return Some(
+ "the move cannot carry stable row ids onto the new fragments (it has no \
+ `row_id_sequence` rechunk step), while the in-place patch preserves them by \
+ moving no rows"
+ .into(),
+ );
+ }
+ // The move would delete rows the user never touched. With `insert_not_matched`,
+ // `Merger::execute_batch` pushes a *second* batch holding the unmatched (`left_only`)
+ // rows, projected with the row-address column appended. Those rows come from the left
+ // side of an outer join, so their `_rowaddr` is NULL. The move extracts addresses with
+ // `batch[ROW_ADDR].as_primitive::().values()`, which reads the raw value
+ // buffer and *ignores the null mask* -- so every inserted row would contribute a bogus
+ // address (in practice 0), used both to take the wrong row and, via `moved_addrs`, to
+ // tombstone it. Never entering the move prevents that as completely as refusing the
+ // merge did, and `update_fragments` already routes the NULL-`_rowaddr` group to
+ // `handle_new_fragments`.
+ if self.params.insert_not_matched {
+ return Some(
+ "the move reads row addresses through the null mask and would tombstone \
+ unrelated rows for the unmatched (inserted) group, which arrives with a NULL \
+ `_rowaddr`"
+ .into(),
+ );
+ }
+ // The move re-reads full rows via a take, which materializes legacy (v1) blob columns
+ // as description structs, not binary -- writing those back would corrupt the blob. The
+ // full-schema path reads them as binary via the scan provider (`SomeBlobsBinary`); the
+ // take path has no equivalent. The in-place patch never reads the blob column at all.
+ //
+ // Scanning the *whole dataset schema* is deliberate, not sloppy. Do not narrow this to
+ // the covered or updated columns: the take reads every column, so an unrelated v1 blob
+ // elsewhere in the schema is corrupted just the same. In fact a covered column can
+ // never itself be a blob -- index creation rejects blob covering columns outright
+ // (`crate::index::vector`, the `covering_columns` validation) -- so this guard is
+ // entirely about the *collateral* columns the full-row take drags in. Narrowing it
+ // would therefore disable it completely and reintroduce the corruption.
+ if self
+ .dataset
+ .schema()
+ .fields_pre_order()
+ .any(|f| f.is_blob() && !f.is_blob_v2())
+ {
+ return Some(
+ "the move's full-row take reads legacy (v1) blob columns as description \
+ structs rather than binary, which would corrupt them on write back"
+ .into(),
+ );
+ }
+ // The move overlays each source column onto a full-row take by REPLACING the whole
+ // column, which needs the source column's Arrow type to equal the target's exactly:
+ // `replace_column_by_name` keeps the target schema and revalidates through
+ // `RecordBatch::try_new`. The partial-schema contract does not promise that. A source
+ // is admitted as a *subschema*, and `compare_fields` recurses with the same options,
+ // so a column may legally differ in its struct children, in nested nullability, or in
+ // nested field metadata -- all three are part of `DataType` and none is compared by
+ // the check that admitted the source. Note a field's OWN nullability is not part of
+ // its `DataType`, so the ordinary "nullable source, non-null target" partial update
+ // does not land here.
+ let target_arrow = Schema::from(self.dataset.schema());
+ let type_mismatched = source_schema
+ .fields()
+ .iter()
+ .filter(|f| {
+ target_arrow
+ .field_with_name(f.name())
+ .is_ok_and(|tf| tf.data_type() != f.data_type())
+ })
+ .map(|f| f.name().clone())
+ .collect::>();
+ if !type_mismatched.is_empty() {
+ return Some(format!(
+ "the move replaces whole columns and source column(s) {type_mismatched:?} do \
+ not have exactly the target's type (struct children, nested nullability or \
+ nested metadata may differ)"
+ ));
+ }
+ None
+ }
+
+ /// Rewrite a partial-schema covered-column update as a row-move: read the full
+ /// existing rows for the touched addresses, overlay the updated columns, write
+ /// them as new fragments, and tombstone the originals. Only the moved rows leave
+ /// the index's reach; the rest of the fragment keeps its coverage, which is the
+ /// point of the move (a recall/cost property, not a correctness one -- see the
+ /// caller for why).
+ ///
+ /// `source` is the merger's partial output stream (`_rowaddr` + updated cols); it is consumed
+ /// in bounded batches so peak memory is one batch of full rows rather than the whole update.
+ /// Mirrors `dataset.update()`'s streamed `RewriteRows` move (`update.rs::execute_impl`).
+ /// Returns the resulting `RewriteRows` operation together with the moved rows' addresses, so
+ /// the caller can publish them as `affected_rows` and let a concurrent delete/update rebase
+ /// instead of hard-conflicting.
+ ///
+ /// Non-stable-row-id datasets only. The caller **falls back to the in-place path** for
+ /// stable-row-id datasets, insert-carrying combinations, legacy v1 blob columns, and
+ /// partial struct-subschema sources — it does not reject them. That fallback branch is
+ /// live and load-bearing, not dead code: the in-place path is correct for covered columns
+ /// (it drops the updated fragment from the index via `prune_updated_fields_from_indices`),
+ /// so this move is an optimisation that confines the bitmap damage, and an optimisation
+ /// must not be able to fail the operation. Restoring a hard rejection here would
+ /// re-break merge shapes that work today.
+ async fn rewrite_covered_update_as_move(
+ &self,
+ source: SendableRecordBatchStream,
+ target_bases_info: Option>,
+ ) -> Result<(Operation, RoaringTreemap)> {
+ let full_schema = self.dataset.schema().clone();
+ // EVERY field id, leaves included (`fields_pre_order`), never just the
+ // top-level ids: the consumer
+ // (`register_pure_rewrite_rows_update_frags_in_indices`) treats this list
+ // as "fields whose values were updated" and compares it against each
+ // index's leaf-expanded dependency set -- its contract for a caller that
+ // cannot prove any field survived unchanged is to pass *every* field id.
+ // An index keyed on a nested field stores a leaf id that a top-level-only
+ // set does not contain, so it would escape the exclusion and have its
+ // bitmap wrongly extended onto the moved fragment. Both sibling row-move
+ // producers (`update.rs`, the insert exec) already expand to leaves for
+ // this exact reason.
+ let preserving: Vec = full_schema
+ .fields_pre_order()
+ .map(|f| f.id as u32)
+ .collect();
+ let empty_op = |fields_bitmap: Vec| Operation::Update {
+ removed_fragment_ids: Vec::new(),
+ updated_fragments: Vec::new(),
+ new_fragments: Vec::new(),
+ fields_modified: Vec::new(),
+ compacted_sstables: self.params.compacted_sstables.clone(),
+ fields_for_preserving_frag_bitmap: fields_bitmap,
+ update_mode: Some(RewriteRows),
+ inserted_rows_filter: None,
+ updated_fragment_offsets: None,
+ };
+
+ // Per merger batch: take the full existing rows for its addresses and overlay the updated
+ // columns, streaming the result so peak memory is one batch of full rows rather than the
+ // whole update. On the partial-schema path the merger appends `_rowaddr` to its output
+ // (`Merger::output_schema`), so every batch names the originals to tombstone; the take's
+ // result is built from the dataset schema and never carries that column onward.
+ let dataset = self.dataset.clone();
+ let plan = Arc::new(
+ ProjectionRequest::from_schema(full_schema.clone())
+ .into_projection_plan(dataset.clone())?,
+ );
+ let out_arrow = Arc::new(Schema::from(&full_schema));
+
+ // Record which originals to tombstone as the overlay runs. `make_rowid_capture_stream` is
+ // deliberately not reused here: its `AddressStyle` capture bulk-loads a `RoaringTreemap`
+ // with `append`, which rejects any value <= the running maximum, so it is only safe on a
+ // stream whose addresses are strictly ascending. That holds today, but only as an accident
+ // of routing: this function is reachable only via the indexed-scan join (every other
+ // partial source goes to v2 -- see the caller), and that scan reads in fragment order, so
+ // addresses arrive ascending however the user ordered the source. Swapping this `extend`
+ // for `append` accordingly passes the covered suite, reversed source included. `extend` is
+ // kept anyway: it costs nothing, and it means a future routing or planner change cannot
+ // turn an ordering assumption into an abort that fires *after* fragments are written.
+ let moved_addrs = Arc::new(Mutex::new(RoaringTreemap::new()));
+ let moved_addrs_for_stream = moved_addrs.clone();
+ let out_arrow_for_stream = out_arrow.clone();
+ // `.then` processes one batch at a time (fully bounded); a small `.buffered(k)` could later
+ // pipeline the take of the next batch with the write of the current one.
+ let overlaid = source.then(move |batch| {
+ let dataset = dataset.clone();
+ let plan = plan.clone();
+ let moved_addrs = moved_addrs_for_stream.clone();
+ async move {
+ let batch = batch?;
+ let addresses: Vec = batch[ROW_ADDR]
+ .as_primitive::()
+ .values()
+ .to_vec();
+ let full_rows =
+ TakeBuilder::try_new_from_addresses(dataset, addresses.clone(), plan)?
+ .execute()
+ .await?;
+ let mut overlaid = full_rows;
+ for field in batch.schema().fields() {
+ let name = field.name();
+ if name == ROW_ADDR || name == ROW_ID {
+ continue;
+ }
+ let new_col = batch
+ .column_by_name(name)
+ .expect("updated column present in merger output")
+ .clone();
+ overlaid = overlaid.replace_column_by_name(name, new_col)?;
+ }
+ // Only after the take succeeded: a batch that never reaches the writer must not
+ // tombstone its originals.
+ moved_addrs
+ .lock()
+ .expect("covered move address set poisoned")
+ .extend(addresses);
+ Ok::<_, DataFusionError>(overlaid)
+ }
+ });
+ let write_stream = Box::pin(RecordBatchStreamAdapter::new(
+ out_arrow_for_stream,
+ overlaid,
+ ));
+
+ // Write the moved rows as new fragments.
+ // Retain the target-base info: `write_fragments_internal` consumes it, but a later cleanup
+ // must still resolve fragments routed to external bases (otherwise `cleanup_data_fragments`
+ // skips every file with a `base_id`, orphaning it).
+ let bases_for_cleanup = target_bases_info.clone();
+ let (new_fragments, _) = write_fragments_internal(
+ self.dataset
+ .manifest()
+ .data_storage_format
+ .lance_file_format(),
+ Some(&self.dataset),
+ self.dataset.object_store.clone(),
+ &self.dataset.base,
+ full_schema.clone(),
+ write_stream,
+ WriteParams::default(),
+ target_bases_info,
+ )
+ .await?;
+
+ // The overlay recorded every address it handed to the writer; the write above has now
+ // drained the stream, so the set is complete.
+ let removed_row_addrs = std::mem::take(
+ &mut *moved_addrs
+ .lock()
+ .expect("covered move address set poisoned"),
+ );
+ if removed_row_addrs.is_empty() {
+ // Nothing matched -> nothing written; return a no-op update.
+ return Ok((empty_op(preserving), RoaringTreemap::new()));
+ }
+
+ // Tombstone the originals.
+ let deletions = Self::apply_deletions(&self.dataset, &removed_row_addrs).await;
+ let (updated_fragments, removed_fragment_ids) = match deletions {
+ Ok(v) => v,
+ Err(e) => {
+ cleanup_data_fragments(
+ &self.dataset.object_store,
+ &self.dataset.base,
+ bases_for_cleanup.as_deref(),
+ &new_fragments,
+ )
+ .await;
+ return Err(e);
+ }
+ };
+
+ Ok((
+ Operation::Update {
+ removed_fragment_ids,
+ updated_fragments,
+ new_fragments,
+ fields_modified: Vec::new(),
+ compacted_sstables: self.params.compacted_sstables.clone(),
+ fields_for_preserving_frag_bitmap: preserving,
+ update_mode: Some(RewriteRows),
+ inserted_rows_filter: None,
+ updated_fragment_offsets: None,
+ },
+ removed_row_addrs,
+ ))
+ }
+
/// Patches the columns carried by `source` into the fragments that hold the
/// rows it names, and writes the rows with no target address as new
/// fragments.
@@ -2736,7 +3068,7 @@ impl MergeInsertJob {
let joined = self.create_joined_stream(source).await?;
let merger = Merger::try_new(
self.params.clone(),
- source_schema,
+ source_schema.clone(),
!is_full_schema,
self.dataset.manifest.uses_stable_row_ids(),
)?;
@@ -2818,38 +3150,108 @@ impl MergeInsertJob {
return Err(Error::not_supported_source("Deleting rows from the target table when there is no match in the source table is not supported when the source data has a different schema than the target data".into()));
}
- // We will have a different commit path here too, as we are modifying
- // fragments rather than writing new ones
- let PatchedFragments {
- updated_fragments,
- new_fragments,
- fields_modified,
- matched_offsets,
- } = Self::update_fragments(
- self.dataset.clone(),
- Box::pin(stream),
- self.dataset.manifest.version + 1,
- target_bases_info,
- )
- .await?;
-
- let operation = Operation::Update {
- removed_fragment_ids: Vec::new(),
- updated_fragments,
- new_fragments,
- fields_modified,
- compacted_sstables: self.params.compacted_sstables.clone(),
- fields_for_preserving_frag_bitmap: vec![], // in-place update do not affect preserving frag bitmap
- update_mode: Some(RewriteColumns),
- inserted_rows_filter: None, // not implemented for v1
- // The version stamped above is a guess; carry the patched offsets
- // so `build_manifest` can re-stamp them at the real commit
- // version after a rebase.
- updated_fragment_offsets: Some(matched_offsets),
+ // A covered column's values are materialized in index storage, so patching one in
+ // place leaves that copy behind. Correctness is NOT what the row-move buys, and the
+ // comment here used to claim otherwise: `covering_fields` is stored as a suffix of
+ // `IndexMetadata::fields`, so `Transaction::index_dependent_leaf_ids` already counts
+ // a covered column as index-dependent and `prune_updated_fields_from_indices` drops
+ // the whole updated fragment from every index covering it. Disabling the move
+ // entirely leaves every covered correctness test in this module passing, both
+ // optimize variants included -- the stale copy is already unreachable, and no
+ // append-optimize failure mode could be reproduced.
+ //
+ // What the move buys is CONFINEMENT. That prune is all-or-nothing per fragment: one
+ // partial update evicts the ENTIRE fragment from the vector index, including rows the
+ // merge never matched, and the vectors are the expensive part to rebuild. The move
+ // tombstones only the matched rows and re-appends them, so the fragment keeps its
+ // coverage for everything else. Measured on the fixture in
+ // `test_covered_update_is_fresh_and_confines_index_invalidation`: with the move the
+ // index bitmap stays `{0, 1}` over fragments `[0, 1, 2]`; patched in place it
+ // collapses to `{0}`. A recall/cost property rather than a correctness one, then --
+ // and that test is the only thing pinning it.
+ //
+ // It also makes the two merge_insert paths AGREE, which is the stronger argument. A
+ // partial source only reaches this branch when every join key has a scalar index
+ // (`can_use_create_plan` routes the rest to v2), and v2's `create_plan` fills every
+ // missing dataset column from the target side and writes FULL rows -- already a
+ // row-move, already confining. Without this branch the same merge would invalidate a
+ // whole fragment or only the matched rows depending on whether an unrelated scalar
+ // index happened to exist on the join key.
+ //
+ // Known cost, deliberately not addressed here: there is no match-ratio heuristic, so
+ // when the matched rows are (nearly) a whole fragment the move rewrites every column
+ // including the vectors for zero confinement gain, where the in-place patch writes one
+ // column file for the same coverage outcome. Adding one is not cheap -- the strategy
+ // must be chosen before the stream is consumed, and the per-fragment match
+ // distribution is not known until the merger has run, so it would take either a
+ // pre-pass scan or buffering the whole merge, and the latter breaks the bounded-memory
+ // property this path is built around.
+ let covered = self.covered_columns_updated(source_schema.as_ref()).await?;
+ // The move cannot handle every merge (see `covered_move_blocker`). Because it is an
+ // optimisation over an already-correct in-place patch, a case it cannot handle
+ // degrades to that patch instead of failing the merge -- otherwise adding
+ // `covering_columns` to a vector index would silently delete merge_insert shapes that
+ // worked before, and only on the half of the routing that reaches this code at all.
+ let move_blocked = if covered.is_empty() {
+ None
+ } else {
+ self.covered_move_blocker(source_schema.as_ref())
};
- // We have rewritten the fragments, not just the deletion files, so
- // we can't use affected rows here.
- (operation, None)
+ if !covered.is_empty() && move_blocked.is_none() {
+ let (operation, removed_row_addrs) = self
+ .rewrite_covered_update_as_move(
+ Box::pin(stream) as SendableRecordBatchStream,
+ target_bases_info,
+ )
+ .await?;
+ // Same shape as the full-schema `RewriteRows` paths below -- deletions on the
+ // originals plus new fragments -- so publish the moved addresses. Without them
+ // `check_update_txn` turns any concurrent delete/update touching these fragments
+ // into a hard conflict, costing covered datasets concurrency that non-covered
+ // ones keep.
+ (operation, Some(RowAddrTreeMap::from(removed_row_addrs)))
+ } else {
+ if let Some(reason) = &move_blocked {
+ info!(
+ "merge_insert is updating covered column(s) {covered:?} in place rather \
+ than moving the matched rows, because {reason}. The updated fragment(s) \
+ lose their coverage in every index covering those columns until the next \
+ optimize; the merge itself is unaffected."
+ );
+ }
+ // We will have a different commit path here too, as we are modifying
+ // fragments rather than writing new ones
+ let PatchedFragments {
+ updated_fragments,
+ new_fragments,
+ fields_modified,
+ matched_offsets,
+ } = Self::update_fragments(
+ self.dataset.clone(),
+ Box::pin(stream),
+ self.dataset.manifest.version + 1,
+ target_bases_info,
+ )
+ .await?;
+
+ let operation = Operation::Update {
+ removed_fragment_ids: Vec::new(),
+ updated_fragments,
+ new_fragments,
+ fields_modified,
+ compacted_sstables: self.params.compacted_sstables.clone(),
+ fields_for_preserving_frag_bitmap: vec![], // in-place update do not affect preserving frag bitmap
+ update_mode: Some(RewriteColumns),
+ inserted_rows_filter: None, // not implemented for v1
+ // The version stamped above is a guess; carry the patched offsets
+ // so `build_manifest` can re-stamp them at the real commit
+ // version after a rebase.
+ updated_fragment_offsets: Some(matched_offsets),
+ };
+ // We have rewritten the fragments, not just the deletion files, so
+ // we can't use affected rows here.
+ (operation, None)
+ }
} else {
let cleanup_bases = target_bases_info.clone();
let (mut new_fragments, _) = write_fragments_internal(
@@ -13248,7 +13650,7 @@ MergeInsert: on=[id], when_matched=DoNothing, when_not_matched=InsertAll, when_n
mod external_error {
use super::*;
- use arrow_schema::{ArrowError, Field as ArrowField, Schema as ArrowSchema};
+ use arrow_schema::{ArrowError, Field as ArrowField, Schema};
use std::fmt;
#[derive(Debug)]
@@ -13267,7 +13669,7 @@ MergeInsert: on=[id], when_matched=DoNothing, when_not_matched=InsertAll, when_n
#[tokio::test]
async fn test_merge_insert_execute_reader_preserves_error_message() {
- let schema = Arc::new(ArrowSchema::new(vec![
+ let schema = Arc::new(Schema::new(vec![
ArrowField::new("key", DataType::Int32, false),
ArrowField::new("value", DataType::Int32, false),
]));
@@ -13699,16 +14101,14 @@ MergeInsert: on=[id], when_matched=DoNothing, when_not_matched=InsertAll, when_n
assert_eq!(combined.num_rows(), 300);
}
- // Regression test: after a partial-schema merge_insert drops a fragment from the vector
- // index bitmap, a vector search should not return duplicate rows. The stale vector index
- // data still references the dropped fragment, and the scanner also flat-scans unindexed
- // fragments, causing the same rows to appear from both paths.
- #[tokio::test]
- async fn test_partial_merge_insert_stale_vector_index_duplicates() {
+ /// Build a dataset with a plain (non-covered) IVF_FLAT vector index whose fragment 1
+ /// has been pruned from the index bitmap by a partial-schema merge_insert that patched
+ /// the indexed vector column in place -- the pruned fragment's stale rows still live in
+ /// the old index segment's storage. Returns `(dataset, total_rows, rows_per_frag, dim)`.
+ async fn build_pruned_plain_vector_dataset(uri: &str) -> (Arc, usize, usize, i32) {
let dim = 4i32;
let rows_per_frag = 10usize;
let num_frags = 3usize;
- let total_rows = rows_per_frag * num_frags;
let schema = Arc::new(Schema::new(vec![
Field::new("id", DataType::Utf8, false),
@@ -13745,9 +14145,7 @@ MergeInsert: on=[id], when_matched=DoNothing, when_not_matched=InsertAll, when_n
// Write 3 fragments
let batch0 = make_batch(0, 0.0);
let reader = Box::new(RecordBatchIterator::new([Ok(batch0)], schema.clone()));
- let mut ds = Dataset::write(reader, "memory://vector_stale_test", None)
- .await
- .unwrap();
+ let mut ds = Dataset::write(reader, uri, None).await.unwrap();
for frag_idx in 1..num_frags {
let batch = make_batch(frag_idx, 0.0);
let reader = Box::new(RecordBatchIterator::new([Ok(batch)], schema.clone()));
@@ -13797,7 +14195,18 @@ MergeInsert: on=[id], when_matched=DoNothing, when_not_matched=InsertAll, when_n
.await
.unwrap();
- // KNN search with k = total_rows to retrieve all rows
+ (ds, rows_per_frag * num_frags, rows_per_frag, dim)
+ }
+
+ /// A KNN query over every row must return each id exactly once -- no stale duplicate
+ /// served from old index storage alongside the fresh copy.
+ async fn assert_knn_no_duplicate_ids(
+ ds: &Dataset,
+ total_rows: usize,
+ rows_per_frag: usize,
+ dim: i32,
+ ) {
+ let frag1_start = rows_per_frag;
let query: Float32Array = (0..dim)
.map(|i| (frag1_start * dim as usize + i as usize) as f32 + 0.5)
.collect();
@@ -13809,7 +14218,6 @@ MergeInsert: on=[id], when_matched=DoNothing, when_not_matched=InsertAll, when_n
.await
.unwrap();
- // Check no duplicate ids
let ids = results
.column_by_name("id")
.unwrap()
@@ -13827,85 +14235,191 @@ MergeInsert: on=[id], when_matched=DoNothing, when_not_matched=InsertAll, when_n
);
}
- // Regression test: after a partial-schema merge_insert drops a fragment from the FTS
- // index bitmap, a full text search should not return duplicate rows. The stale inverted
- // index data still references the dropped fragment, and the scanner also flat-scans
- // unindexed fragments, causing the same rows to appear from both paths.
+ // Regression test: after a partial-schema merge_insert drops a fragment from the vector
+ // index bitmap, a vector search should not return duplicate rows. The stale vector index
+ // data still references the dropped fragment, and the scanner also flat-scans unindexed
+ // fragments, causing the same rows to appear from both paths.
#[tokio::test]
- async fn test_partial_merge_insert_stale_fts_index_duplicates() {
- let rows_per_frag = 10usize;
- let num_frags = 3usize;
+ async fn test_partial_merge_insert_stale_vector_index_duplicates() {
+ let (ds, total_rows, rows_per_frag, dim) =
+ build_pruned_plain_vector_dataset("memory://vector_stale_test").await;
+ assert_knn_no_duplicate_ids(&ds, total_rows, rows_per_frag, dim).await;
+ }
+
+ /// Same pruned-fragment state, but merged back into the index:
+ /// `optimize_indices(merge)` re-scans the pruned fragment as unindexed data AND
+ /// carries the fragment's stale rows from the old segment's storage -- the merge
+ /// must drop the stale copies for plain (non-covered) indexes just as it does for
+ /// covered ones, or ANN queries return duplicate row ids from the merged index.
+ #[tokio::test]
+ async fn test_optimize_merge_after_partial_update_no_duplicates_non_covered() {
+ use lance_index::optimize::OptimizeOptions;
+ let (ds, total_rows, rows_per_frag, dim) =
+ build_pruned_plain_vector_dataset("memory://vector_stale_opt_merge").await;
+ let mut ds = Arc::try_unwrap(ds).unwrap_or_else(|arc| (*arc).clone());
+ ds.optimize_indices(&OptimizeOptions::merge(1))
+ .await
+ .unwrap();
+ assert_knn_no_duplicate_ids(&ds, total_rows, rows_per_frag, dim).await;
+ }
+
+ /// A partial merge_insert that rewrites only a covered column is a row-MOVE: the
+ /// touched rows are tombstoned at their old addresses and reinserted into a new
+ /// fragment, so the index's stale copy is hidden by the deletion mask rather than
+ /// patched in place. Fragment 1 is updated in full, so it disappears entirely, and a
+ /// covered query must observe the fresh values for every row.
+ #[tokio::test]
+ async fn test_partial_merge_insert_moves_covered_column_rows() {
+ let (ds, total_rows, rows_per_frag, dim) =
+ build_moved_covering_dataset("memory://covering_move_test").await;
+ assert!(
+ !ds.get_fragments().iter().any(|f| f.id() == 1),
+ "fragment 1's rows should have been moved out (tombstoned + reinserted), \
+ but fragment 1 is still present"
+ );
+
+ assert_covered_query_no_duplicates_fresh(&ds, total_rows, rows_per_frag, dim).await;
+ }
+
+ /// Pins the two things a partial merge_insert on a covered column must deliver: covered reads
+ /// are FRESH with no intervening optimize, and the index invalidation is confined to the rows
+ /// actually updated.
+ ///
+ /// The second half is what the row-move uniquely buys, and it is the half that discriminates.
+ /// Freshness alone does not: `covering_fields` is stored as a suffix of `IndexMetadata::fields`,
+ /// so `prune_updated_fields_from_indices` already counts a covered column as index-dependent
+ /// and drops the whole updated fragment from the index whenever a `RewriteColumns` update
+ /// touches one. That backstop hides the stale copy on its own -- verified by disabling the
+ /// row-move entirely, which leaves every covered correctness assertion in this module passing.
+ /// What it costs is coverage: an in-place update of one payload column evicts the ENTIRE
+ /// fragment from the vector index, including rows the merge never touched, and the vectors are
+ /// the expensive part to rebuild. The row-move instead tombstones only the matched rows and
+ /// re-appends them, so the fragment keeps its coverage for everything else. Measured here:
+ /// with the move the bitmap stays `{0, 1}` over fragments `[0, 1, 2]`; patched in place it
+ /// collapses to `{0}`.
+ ///
+ /// Freshness is still checked, against an INDEPENDENT `take_rows` from the base table rather
+ /// than against a formula this test also wrote -- asserting that a column came back, or that
+ /// the query returned something, passes just as happily against a stale copy. `take_rows`
+ /// re-orders its output to match the requested row ids (see the remapping branch in
+ /// `dataset::take`), so the two reads line up row for row.
+ ///
+ /// Shape follows the recipe in `dataset::tests::dataset_index`: four well-separated clusters,
+ /// since a covered-ANN test with one partition never reaches the probe path at all and passes
+ /// against a broken covered read.
+ #[tokio::test]
+ async fn test_covered_update_is_fresh_and_confines_index_invalidation() {
+ use arrow_array::cast::AsArray;
+ use arrow_array::types::UInt64Type;
+ use lance_core::ROW_ID;
+ use std::collections::HashSet;
+
+ const DIMS: i32 = 16;
+ const NUM_CLUSTERS: usize = 4;
+ const ROWS_PER_CLUSTER: usize = 64;
+ const TOTAL: usize = NUM_CLUSTERS * ROWS_PER_CLUSTER;
+ // Only the last cluster is rewritten, so one query spans both moved rows and untouched
+ // rows still served from the original index storage.
+ const UPDATED_FROM: i32 = ((NUM_CLUSTERS - 1) * ROWS_PER_CLUSTER) as i32;
let schema = Arc::new(Schema::new(vec![
- Field::new("id", DataType::Utf8, false),
- Field::new("category", DataType::Utf8, false),
- Field::new("text", DataType::Utf8, false),
+ Field::new("id", DataType::Int32, false),
+ Field::new("payload", DataType::Int32, false),
+ Field::new(
+ "vector",
+ DataType::FixedSizeList(
+ Arc::new(Field::new("item", DataType::Float32, true)),
+ DIMS,
+ ),
+ false,
+ ),
]));
- let make_batch = |frag_idx: usize| {
- let start = frag_idx * rows_per_frag;
- let ids: Vec = (start..start + rows_per_frag)
- .map(|j| format!("id-{j:04}"))
- .collect();
- let cats: Vec<&str> = vec!["A"; rows_per_frag];
- // Every row contains "common" so we can search for it and expect all rows
- let texts: Vec = (start..start + rows_per_frag)
- .map(|j| format!("common unique{j:04}"))
- .collect();
+ // Cluster centers 1000 apart on dim 0 with deterministic jitter: uniform-random vectors
+ // leave the early-pruning heuristic searching every partition.
+ let make_batch = |clusters: std::ops::Range| {
+ let mut ids = Vec::new();
+ let mut payloads = Vec::new();
+ let mut values = Vec::new();
+ for cluster in clusters {
+ let center = (cluster * 1000) as f32;
+ for row in 0..ROWS_PER_CLUSTER {
+ let n = cluster * ROWS_PER_CLUSTER + row;
+ ids.push(n as i32);
+ payloads.push(n as i32 * 10);
+ for dim in 0..DIMS as usize {
+ let base = if dim == 0 { center } else { 0.0 };
+ values.push(base + ((n * 7919 + dim) % 97) as f32 * 0.0001);
+ }
+ }
+ }
+ let vectors =
+ FixedSizeListArray::try_new_from_values(Float32Array::from(values), DIMS).unwrap();
RecordBatch::try_new(
schema.clone(),
vec![
- Arc::new(StringArray::from(ids)),
- Arc::new(StringArray::from(cats)),
- Arc::new(StringArray::from(texts)),
+ Arc::new(Int32Array::from(ids)),
+ Arc::new(Int32Array::from(payloads)),
+ Arc::new(vectors),
],
)
.unwrap()
};
- // Write 3 fragments
- let batch0 = make_batch(0);
- let reader = Box::new(RecordBatchIterator::new([Ok(batch0)], schema.clone()));
- let mut ds = Dataset::write(reader, "memory://fts_stale_test", None)
+ // Two fragments, so the move has to reach across more than one.
+ let reader = Box::new(RecordBatchIterator::new(
+ [Ok(make_batch(0..2))],
+ schema.clone(),
+ ));
+ let mut ds = Dataset::write(reader, "memory://covered_fresh_no_optimize", None)
.await
.unwrap();
- for frag_idx in 1..num_frags {
- let batch = make_batch(frag_idx);
- let reader = Box::new(RecordBatchIterator::new([Ok(batch)], schema.clone()));
- ds.append(reader, None).await.unwrap();
- }
+ let reader = Box::new(RecordBatchIterator::new(
+ [Ok(make_batch(2..NUM_CLUSTERS))],
+ schema.clone(),
+ ));
+ ds.append(reader, None).await.unwrap();
- // Create inverted index on text
- let params = InvertedIndexParams::default();
- ds.create_index(&["text"], IndexType::Inverted, None, ¶ms, true)
+ ds.create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+ let mut params = VectorIndexParams::ivf_pq(NUM_CLUSTERS, 8, 4, MetricType::L2, 2);
+ params.covering_columns(vec!["payload".to_string()]);
+ ds.create_index(&["vector"], IndexType::Vector, None, ¶ms, false)
.await
.unwrap();
-
let ds = Arc::new(ds);
- // Partial merge_insert with (id, text) on fragment 1 rows.
- // Text still contains "common" so FTS will find them via both paths.
- // This drops fragment 1 from the inverted index bitmap.
- let frag1_start = rows_per_frag;
- let ids: Vec = (frag1_start..frag1_start + rows_per_frag)
- .map(|j| format!("id-{j:04}"))
- .collect();
- let texts: Vec = (frag1_start..frag1_start + rows_per_frag)
- .map(|j| format!("common updated{j:04}"))
- .collect();
+ // Precondition the assertion depends on: the index really does carry `payload`, so a
+ // projection of just `payload` is answered from index storage.
+ let payload_field_id = ds.schema().field("payload").unwrap().id;
+ let indices = ds.load_indices().await.unwrap();
+ let vec_idx = indices.iter().find(|i| i.name == "vector_idx").unwrap();
+ assert_eq!(vec_idx.covering_fields, vec![payload_field_id]);
+
+ // Partial-schema merge_insert rewriting ONLY the covered column, for the last cluster.
let sub_schema = Arc::new(Schema::new(vec![
- Field::new("id", DataType::Utf8, false),
- Field::new("text", DataType::Utf8, false),
+ Field::new("id", DataType::Int32, false),
+ Field::new("payload", DataType::Int32, false),
]));
+ let updated_ids: Vec = (UPDATED_FROM..TOTAL as i32).collect();
+ let updated_payloads: Vec = updated_ids.iter().map(|id| id * 10 + 7).collect();
let update_batch = RecordBatch::try_new(
sub_schema.clone(),
vec![
- Arc::new(StringArray::from(ids)),
- Arc::new(StringArray::from(texts)),
+ Arc::new(Int32Array::from(updated_ids)),
+ Arc::new(Int32Array::from(updated_payloads)),
],
)
.unwrap();
let reader = Box::new(RecordBatchIterator::new([Ok(update_batch)], sub_schema));
+ let frags_before: HashSet = ds.get_fragments().iter().map(|f| f.id()).collect();
let (ds, _) = MergeInsertBuilder::try_new(ds, vec!["id".to_string()])
.unwrap()
.when_matched(WhenMatched::UpdateAll)
@@ -13915,17 +14429,1653 @@ MergeInsert: on=[id], when_matched=DoNothing, when_not_matched=InsertAll, when_n
.execute_reader(reader)
.await
.unwrap();
+ // Deliberately no `optimize_indices` here: the point is that the stale copy is hidden
+ // without one.
- // FTS search for "common" — every row should match exactly once
- let query = FullTextSearchQuery::new("common".to_string());
- let results = ds
+ // The move half: the matched rows left fragment 1 for a brand-new fragment, and
+ // fragment 1 -- which also holds 64 rows the merge never matched -- keeps its place in
+ // the vector index. An in-place patch evicts it instead.
+ let frags_after: HashSet = ds.get_fragments().iter().map(|f| f.id()).collect();
+ assert!(
+ frags_after.difference(&frags_before).count() == 1,
+ "the matched rows should have been re-appended to one new fragment; \
+ fragments went from {frags_before:?} to {frags_after:?}"
+ );
+ let updated_frag = *frags_before.iter().max().unwrap();
+ let indices = ds.load_indices().await.unwrap();
+ let vec_idx = indices.iter().find(|i| i.name == "vector_idx").unwrap();
+ let bitmap = vec_idx.fragment_bitmap.as_ref().unwrap();
+ assert!(
+ bitmap.contains(updated_frag as u32),
+ "a covered update must not evict the whole fragment from the vector index: \
+ fragment {updated_frag} still holds {ROWS_PER_CLUSTER} untouched rows, but the \
+ index now covers only {bitmap:?}"
+ );
+
+ let mut q_values = vec![0.0f32; DIMS as usize];
+ q_values[0] = ((NUM_CLUSTERS - 1) * 1000) as f32;
+ let query = Float32Array::from(q_values);
+
+ // Project ONLY the covered column. Adding an uncovered column here would send the whole
+ // projection to the base table and the covered storage would never be read, so the test
+ // would pass against a stale copy.
+ let mut scan = ds.scan();
+ scan.nearest("vector", &query, TOTAL).unwrap();
+ scan.with_row_id();
+ scan.project(&["payload"]).unwrap();
+ let plan = scan.explain_plan(true).await.unwrap();
+ assert!(
+ plan.contains("ANNIvfPartition"),
+ "the covered values must come from the vector index, not a flat rescan:\n{plan}"
+ );
+ let results = scan.try_into_batch().await.unwrap();
+
+ assert_eq!(
+ results.num_rows(),
+ TOTAL,
+ "covered query returned {} rows, expected {TOTAL}",
+ results.num_rows()
+ );
+ let row_ids = results[ROW_ID].as_primitive::();
+ let unique: HashSet = row_ids.values().iter().copied().collect();
+ assert_eq!(
+ unique.len(),
+ TOTAL,
+ "a stale copy survived at its old address: duplicate row ids in covered results"
+ );
+
+ // The independent read: same rows, fetched from the base table by row id.
+ let queried_row_ids: Vec = row_ids.values().to_vec();
+ let base = ds
+ .take_rows(
+ &queried_row_ids,
+ ProjectionRequest::from_schema(ds.schema().project(&["payload"]).unwrap()),
+ )
+ .await
+ .unwrap();
+ assert_eq!(base.num_rows(), TOTAL);
+
+ let covered_payload = results["payload"].as_primitive::();
+ let base_payload = base["payload"].as_primitive::();
+ let mut updated_seen = 0usize;
+ for i in 0..TOTAL {
+ assert_eq!(
+ covered_payload.value(i),
+ base_payload.value(i),
+ "covered read disagrees with the base table for row id {}: index says {}, \
+ table says {}",
+ row_ids.value(i),
+ covered_payload.value(i),
+ base_payload.value(i)
+ );
+ if base_payload.value(i) % 10 == 7 {
+ updated_seen += 1;
+ }
+ }
+ // Guards the assertion above from passing vacuously if the merge silently no-ops.
+ assert_eq!(
+ updated_seen, ROWS_PER_CLUSTER,
+ "expected {ROWS_PER_CLUSTER} rewritten rows in the covered results, saw {updated_seen}"
+ );
+
+ // The comparison above would still agree if the move had taken the WRONG addresses --
+ // the base table would carry the same corruption, so both reads would match. Row
+ // integrity is what catches that: the move rebuilds each row from a full-row take at an
+ // address, so a wrongly addressed take pairs one row's `id` with another's payload and
+ // duplicates or drops ids.
+ let full = ds
.scan()
- .full_text_search(query)
+ .project(&["id", "payload"])
.unwrap()
.try_into_batch()
.await
.unwrap();
-
+ assert_eq!(full.num_rows(), TOTAL, "the move changed the row count");
+ let full_ids = full["id"].as_primitive::();
+ let full_payload = full["payload"].as_primitive::();
+ let mut seen_ids = HashSet::new();
+ for i in 0..TOTAL {
+ let id = full_ids.value(i);
+ assert!(seen_ids.insert(id), "id {id} duplicated by the move");
+ let expected = if id >= UPDATED_FROM {
+ id * 10 + 7
+ } else {
+ id * 10
+ };
+ assert_eq!(
+ full_payload.value(i),
+ expected,
+ "row {id} came back paired with another row's payload"
+ );
+ }
+ assert_eq!(seen_ids.len(), TOTAL, "the move dropped rows");
+ }
+
+ /// Bounded-streaming regression for the covered row-move: a covered-column update spanning
+ /// MORE than one scan batch (> `BATCH_SIZE_FALLBACK` = 8192 rows) must move every row exactly
+ /// once with its fresh value. `rewrite_covered_update_as_move` streams the take/overlay/write
+ /// per batch and accumulates the tombstoned addresses across batches, so this exercises the
+ /// multi-batch accumulation path the small covered tests never reach.
+ #[tokio::test]
+ async fn test_covered_update_move_streams_multiple_batches() {
+ use arrow_array::cast::AsArray;
+ use std::collections::HashSet;
+
+ let dim = 2i32;
+ let rows_per_frag = 5000usize; // 2 frags => 10_000 rows > BATCH_SIZE_FALLBACK (8192)
+ let num_frags = 2usize;
+ let total_rows = rows_per_frag * num_frags;
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new(
+ "vec",
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), dim),
+ false,
+ ),
+ ]));
+ let make_batch = |frag_idx: usize| {
+ let start = frag_idx * rows_per_frag;
+ let ids: Vec = (start..start + rows_per_frag)
+ .map(|j| format!("id-{j:05}"))
+ .collect();
+ let cats: Vec = (start..start + rows_per_frag)
+ .map(|j| format!("cat-{j:05}"))
+ .collect();
+ let values: Vec = (0..rows_per_frag * dim as usize)
+ .map(|i| (start * dim as usize + i) as f32)
+ .collect();
+ let vectors =
+ FixedSizeListArray::try_new_from_values(Float32Array::from(values), dim).unwrap();
+ RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(cats)),
+ Arc::new(vectors),
+ ],
+ )
+ .unwrap()
+ };
+
+ let reader = Box::new(RecordBatchIterator::new(
+ [Ok(make_batch(0))],
+ schema.clone(),
+ ));
+ let mut ds = Dataset::write(reader, "memory://covered_move_multibatch", None)
+ .await
+ .unwrap();
+ for frag_idx in 1..num_frags {
+ let reader = Box::new(RecordBatchIterator::new(
+ [Ok(make_batch(frag_idx))],
+ schema.clone(),
+ ));
+ ds.append(reader, None).await.unwrap();
+ }
+
+ // BTree on the join key + a vector index covering `category`.
+ ds.create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+ let mut params = VectorIndexParams::ivf_flat(2, MetricType::L2);
+ params.covering_columns(vec!["category".to_string()]);
+ ds.create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await
+ .unwrap();
+ let ds = Arc::new(ds);
+
+ // Partial merge_insert rewriting `category` for EVERY row -> a >1-batch covered move.
+ let ids: Vec = (0..total_rows).map(|j| format!("id-{j:05}")).collect();
+ let new_cats: Vec = (0..total_rows).map(|j| format!("updated-{j:05}")).collect();
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ ]));
+ let update_batch = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(new_cats)),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(update_batch)], sub_schema));
+ let (ds, _) = MergeInsertBuilder::try_new(ds, vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::UpdateAll)
+ .when_not_matched(WhenNotMatched::DoNothing)
+ .try_build()
+ .unwrap()
+ .execute_reader(reader)
+ .await
+ .unwrap();
+
+ // Every row must survive exactly once with its fresh category -- nothing lost or
+ // duplicated across the batch boundary the streaming move crosses.
+ let batch = ds
+ .scan()
+ .project(&["id", "category"])
+ .unwrap()
+ .try_into_batch()
+ .await
+ .unwrap();
+ assert_eq!(
+ batch.num_rows(),
+ total_rows,
+ "row count changed after a multi-batch covered move"
+ );
+ let ids_col = batch["id"].as_string::();
+ let cats_col = batch["category"].as_string::();
+ let mut seen = HashSet::new();
+ for i in 0..batch.num_rows() {
+ let id = ids_col.value(i);
+ assert!(
+ seen.insert(id.to_string()),
+ "duplicate id {id} after covered move"
+ );
+ let n: usize = id[3..].parse().unwrap();
+ assert_eq!(
+ cats_col.value(i),
+ format!("updated-{n:05}"),
+ "stale or missing covered value for {id}"
+ );
+ }
+ assert_eq!(seen.len(), total_rows);
+ }
+
+ /// Runs a partial-schema merge_insert whose source carries a STRUCT with only one of the
+ /// target's two children, alongside `category`. `cover` decides whether the vector index
+ /// includes `category`, i.e. whether the covered row-move branch is taken at all.
+ async fn partial_struct_source_merge(uri: &str, cover: bool) -> Result> {
+ const ROWS: usize = 256;
+ const DIM: i32 = 2;
+ let struct_fields: arrow_schema::Fields = vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Int32, false),
+ ]
+ .into();
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new("s", DataType::Struct(struct_fields.clone()), false),
+ Field::new(
+ "vec",
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), DIM),
+ false,
+ ),
+ ]));
+ let ids: Vec = (0..ROWS).map(|j| format!("id-{j:03}")).collect();
+ let cats: Vec = (0..ROWS).map(|j| format!("cat-{j:03}")).collect();
+ let a = Int32Array::from((0..ROWS as i32).collect::>());
+ let b = Int32Array::from((0..ROWS as i32).map(|v| v + 1000).collect::>());
+ let s = StructArray::new(struct_fields, vec![Arc::new(a), Arc::new(b)], None);
+ let values: Vec = (0..ROWS * DIM as usize).map(|i| i as f32).collect();
+ let vectors =
+ FixedSizeListArray::try_new_from_values(Float32Array::from(values), DIM).unwrap();
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids.clone())),
+ Arc::new(StringArray::from(cats)),
+ Arc::new(s),
+ Arc::new(vectors),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(batch)], schema.clone()));
+ let mut ds = Dataset::write(reader, uri, None).await.unwrap();
+ ds.create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+ let mut params = VectorIndexParams::ivf_flat(2, MetricType::L2);
+ if cover {
+ params.covering_columns(vec!["category".to_string()]);
+ }
+ ds.create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await
+ .unwrap();
+ let ds = Arc::new(ds);
+
+ // Partial source: the join key, the (maybe-covered) `category`, and a STRUCT with
+ // only one of its two children.
+ let partial_struct_fields: arrow_schema::Fields =
+ vec![Field::new("a", DataType::Int32, false)].into();
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new("s", DataType::Struct(partial_struct_fields.clone()), false),
+ ]));
+ let new_cats: Vec = (0..ROWS).map(|j| format!("upd-{j:03}")).collect();
+ let new_a = Int32Array::from((0..ROWS as i32).map(|v| v + 500).collect::>());
+ let new_s = StructArray::new(partial_struct_fields, vec![Arc::new(new_a)], None);
+ let update_batch = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(new_cats)),
+ Arc::new(new_s),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(update_batch)], sub_schema));
+ MergeInsertBuilder::try_new(ds, vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::UpdateAll)
+ .when_not_matched(WhenNotMatched::DoNothing)
+ .try_build()
+ .unwrap()
+ .execute_reader(reader)
+ .await
+ .map(|(ds, _)| ds)
+ }
+
+ /// `id -> (s.a, s.b, category)` for the whole dataset, so the covered and uncovered runs of
+ /// `partial_struct_source_merge` can be compared row for row.
+ async fn partial_struct_merge_contents(ds: &Dataset) -> HashMap {
+ let batch = ds
+ .scan()
+ .project(&["id", "category", "s"])
+ .unwrap()
+ .try_into_batch()
+ .await
+ .unwrap();
+ let ids = batch["id"].as_string::();
+ let cats = batch["category"].as_string::();
+ let s = batch["s"].as_struct();
+ let a = s.column_by_name("a").unwrap().as_primitive::();
+ let b = s.column_by_name("b").unwrap().as_primitive::();
+ (0..batch.num_rows())
+ .map(|i| {
+ (
+ ids.value(i).to_string(),
+ (a.value(i), b.value(i), cats.value(i).to_string()),
+ )
+ })
+ .collect()
+ }
+
+ /// The covered row-move overlays each source column onto a full-row take by REPLACING the
+ /// whole column, so it needs the source column's Arrow type to equal the target's exactly. A
+ /// partial-schema source is only required to be a *subschema*, and `compare_fields` recurses
+ /// with the same options, so a struct may legally arrive carrying only some of its children.
+ /// Left unguarded this fails with an opaque Arrow "column types must match schema types" from
+ /// `replace_column_by_name`; guarded, it falls back to the in-place patch.
+ ///
+ /// The control is the whole point: the same merge on a dataset whose index does NOT cover
+ /// `category` takes the in-place branch, and both runs must produce **identical data**. That
+ /// pins the fallback as behaviour-preserving rather than merely non-failing -- adding
+ /// `covering_columns` to a vector index must not change what a merge does, only how much index
+ /// coverage survives it.
+ #[tokio::test]
+ async fn test_merge_insert_covered_update_falls_back_with_partial_struct_source() {
+ let plain = partial_struct_source_merge("memory://partial_struct_plain", false)
+ .await
+ .expect(
+ "control: a partial struct source is handled by the in-place branch, so if this \
+ fails the comparison below is no longer evidence of anything",
+ );
+ let covered = partial_struct_source_merge("memory://partial_struct_covered", true)
+ .await
+ .expect("a partial struct source must fall back to in-place, not fail");
+
+ let plain_rows = partial_struct_merge_contents(&plain).await;
+ let covered_rows = partial_struct_merge_contents(&covered).await;
+ assert_eq!(plain_rows.len(), 256);
+ assert_eq!(
+ plain_rows, covered_rows,
+ "covering a column must not change what the merge writes"
+ );
+ // Spot-check against the expected values too, so an identical-but-wrong pair of runs
+ // cannot pass: `s.a` was updated, `s.b` was never in the source and must survive.
+ assert_eq!(covered_rows["id-007"], (507, 1007, "upd-007".to_string()));
+
+ // Confinement is what covering forfeits here, and only that.
+ let indices = covered.load_indices().await.unwrap();
+ let vec_idx = indices.iter().find(|i| i.name == "vec_idx").unwrap();
+ let bitmap = vec_idx.fragment_bitmap.as_ref().unwrap();
+ assert!(
+ bitmap.is_empty(),
+ "the in-place fallback should have evicted the only fragment from the vector index; \
+ still covered means the fallback was not taken: {bitmap:?}"
+ );
+ }
+
+ /// The covered row-move must not depend on the order its source arrives in: it accumulates
+ /// tombstoned addresses with `extend` rather than the bulk `RoaringTreemap::append`, which
+ /// rejects any value <= the running maximum and would abort *after* fragments were written,
+ /// orphaning them.
+ ///
+ /// Note what this can and cannot reach. The BTree is required, not incidental: without a
+ /// scalar index on every join key, `can_use_create_plan` routes the merge to v2 and this
+ /// function never runs (the earlier version of this test omitted the index specifically to
+ /// get the hash join, and so tested nothing here). With it, the merge takes the indexed-scan
+ /// join -- the only route to the move -- which reads in fragment order, so descending
+ /// addresses cannot be produced through the public API at all. What is pinned is that a
+ /// reversed *source* is handled correctly; `extend` remains a deliberate defence against a
+ /// future routing change rather than something reachable today.
+ #[tokio::test]
+ async fn test_covered_update_move_accepts_unsorted_source_order() {
+ use arrow_array::cast::AsArray;
+ use std::collections::HashSet;
+
+ let dim = 2i32;
+ let total_rows = 128usize;
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new(
+ "vec",
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), dim),
+ false,
+ ),
+ ]));
+ let ids: Vec = (0..total_rows).map(|j| format!("id-{j:04}")).collect();
+ let cats: Vec = (0..total_rows).map(|j| format!("cat-{j:04}")).collect();
+ let values: Vec = (0..total_rows * dim as usize).map(|i| i as f32).collect();
+ let vectors =
+ FixedSizeListArray::try_new_from_values(Float32Array::from(values), dim).unwrap();
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids.clone())),
+ Arc::new(StringArray::from(cats)),
+ Arc::new(vectors),
+ ],
+ )
+ .unwrap();
+
+ let reader = Box::new(RecordBatchIterator::new([Ok(batch)], schema.clone()));
+ let mut ds = Dataset::write(reader, "memory://covered_move_unsorted", None)
+ .await
+ .unwrap();
+ // The BTree is what makes this test reach `rewrite_covered_update_as_move` at all --
+ // see the doc comment.
+ ds.create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+ let mut params = VectorIndexParams::ivf_flat(2, MetricType::L2);
+ params.covering_columns(vec!["category".to_string()]);
+ ds.create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await
+ .unwrap();
+ let ds = Arc::new(ds);
+
+ // Feed the source fully reversed.
+ let mut update_ids: Vec = ids.clone();
+ update_ids.reverse();
+ let new_cats: Vec = update_ids
+ .iter()
+ .map(|id| format!("updated-{}", &id[3..]))
+ .collect();
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ ]));
+ let update_batch = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(StringArray::from(update_ids)),
+ Arc::new(StringArray::from(new_cats)),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(update_batch)], sub_schema));
+ let frags_before: HashSet = ds.get_fragments().iter().map(|f| f.id()).collect();
+ let (ds, _) = MergeInsertBuilder::try_new(ds, vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::UpdateAll)
+ .when_not_matched(WhenNotMatched::DoNothing)
+ .try_build()
+ .unwrap()
+ .execute_reader(reader)
+ .await
+ .unwrap();
+
+ // Proof this test reached the move rather than the in-place patch: the move re-appends the
+ // matched rows to a new fragment, where the in-place patch leaves the fragment ids alone.
+ // Without this the value checks below pass on either path.
+ //
+ // This does NOT by itself exclude the v2 path, which also writes full rows and would also
+ // append. v2 is excluded structurally instead: the scalar index created above means
+ // `can_use_create_plan`'s `!would_use_scalar_index` is false. That coupling is load-bearing
+ // -- it is exactly what this test lacked when it silently never ran the move at all.
+ let frags_after: HashSet = ds.get_fragments().iter().map(|f| f.id()).collect();
+ assert!(
+ !frags_after
+ .difference(&frags_before)
+ .collect::>()
+ .is_empty(),
+ "expected the covered row-move to append a new fragment; fragments went from \
+ {frags_before:?} to {frags_after:?}, so this test never ran the move"
+ );
+
+ let batch = ds
+ .scan()
+ .project(&["id", "category"])
+ .unwrap()
+ .try_into_batch()
+ .await
+ .unwrap();
+ assert_eq!(
+ batch.num_rows(),
+ total_rows,
+ "row count changed after an out-of-order covered move"
+ );
+ let ids_col = batch["id"].as_string::();
+ let cats_col = batch["category"].as_string::();
+ let mut seen = HashSet::new();
+ for i in 0..batch.num_rows() {
+ let id = ids_col.value(i);
+ assert!(
+ seen.insert(id.to_string()),
+ "duplicate id {id} after covered move"
+ );
+ assert_eq!(
+ cats_col.value(i),
+ format!("updated-{}", &id[3..]),
+ "stale or missing covered value for {id}"
+ );
+ }
+ assert_eq!(seen.len(), total_rows);
+ }
+
+ /// The covered row-move must publish the moved rows as `affected_rows`, exactly as the
+ /// full-schema `RewriteRows` paths do. Without them `check_update_txn` short-circuits on
+ /// `affected_rows.is_none()` and turns any concurrent delete/update touching the same
+ /// fragments into a retryable conflict, so covered datasets would silently lose the
+ /// merge_insert/delete concurrency that non-covered ones keep.
+ #[tokio::test]
+ async fn test_covered_update_move_publishes_affected_rows() {
+ let dim = 2i32;
+ let total_rows = 64usize;
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new(
+ "vec",
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), dim),
+ false,
+ ),
+ ]));
+ let ids: Vec = (0..total_rows).map(|j| format!("id-{j:04}")).collect();
+ let cats: Vec = (0..total_rows).map(|j| format!("cat-{j:04}")).collect();
+ let values: Vec = (0..total_rows * dim as usize).map(|i| i as f32).collect();
+ let vectors =
+ FixedSizeListArray::try_new_from_values(Float32Array::from(values), dim).unwrap();
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(cats)),
+ Arc::new(vectors),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(batch)], schema.clone()));
+ let mut ds = Dataset::write(reader, "memory://covered_move_affected_rows", None)
+ .await
+ .unwrap();
+ // Required to reach `rewrite_covered_update_as_move`: without a scalar index on every
+ // join key, `can_use_create_plan` routes the merge to v2, which publishes affected rows
+ // of its own and would satisfy this test's assertion without the move running at all.
+ ds.create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+ let mut params = VectorIndexParams::ivf_flat(2, MetricType::L2);
+ params.covering_columns(vec!["category".to_string()]);
+ ds.create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await
+ .unwrap();
+ let ds = Arc::new(ds);
+
+ // Single fragment written in id order, so row N sits at address N.
+ let targets = [2usize, 5, 7];
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ ]));
+ let update_batch = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(StringArray::from(
+ targets
+ .iter()
+ .map(|n| format!("id-{n:04}"))
+ .collect::>(),
+ )),
+ Arc::new(StringArray::from(
+ targets
+ .iter()
+ .map(|n| format!("updated-{n:04}"))
+ .collect::>(),
+ )),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(update_batch)], sub_schema));
+ let UncommittedMergeInsert { affected_rows, .. } =
+ MergeInsertBuilder::try_new(ds, vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::UpdateAll)
+ .when_not_matched(WhenNotMatched::DoNothing)
+ .try_build()
+ .unwrap()
+ .execute_uncommitted(reader)
+ .await
+ .unwrap();
+
+ let expected =
+ RowAddrTreeMap::from(RoaringTreemap::from_iter(targets.iter().map(|n| *n as u64)));
+ assert_eq!(
+ affected_rows,
+ Some(expected),
+ "covered row-move must publish the moved addresses so a concurrent delete can rebase"
+ );
+ }
+
+ /// Builds a 3-fragment dataset with an IVF_PQ index covering `category`, then does a
+ /// partial merge_insert that rewrites only `category` for fragment 1. That update is a
+ /// row-MOVE: fragment 1's rows are tombstoned and rewritten into a new fragment, while
+ /// the old segment's storage still physically holds their pre-update covering values.
+ /// Returns `(post-move dataset, total_rows, rows_per_frag, dim)`.
+ /// The covered move must report EVERY field id -- leaves included -- in
+ /// `fields_for_preserving_frag_bitmap`. The consumer compares that list against
+ /// each index's leaf-expanded dependency set, so a top-level-only list would let
+ /// an index keyed on a nested field (its dependency set holds the LEAF id)
+ /// escape the exclude-everything contract and get its bitmap wrongly extended
+ /// onto the moved fragment. Unobservable through query results today (the move
+ /// runs only on address-style datasets, where the consumer skips every index),
+ /// so this pins the committed transaction payload itself.
+ #[tokio::test]
+ async fn test_covered_move_reports_leaf_field_ids_for_preserving() {
+ use crate::io::commit::read_transaction_file;
+
+ let dim = 4i32;
+ let rows_per_frag = 256usize; // >= 256 rows so PQ has enough to train
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new(
+ "meta",
+ DataType::Struct(vec![Field::new("code", DataType::Int32, false)].into()),
+ false,
+ ),
+ Field::new(
+ "vec",
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), dim),
+ false,
+ ),
+ ]));
+ let make_batch = |frag_idx: usize| {
+ let start = frag_idx * rows_per_frag;
+ let ids: Vec = (start..start + rows_per_frag)
+ .map(|j| format!("id-{j:04}"))
+ .collect();
+ let cats: Vec = (start..start + rows_per_frag)
+ .map(|j| format!("cat-{j:04}"))
+ .collect();
+ let codes =
+ Int32Array::from_iter_values((start..start + rows_per_frag).map(|j| j as i32));
+ let meta = StructArray::from(vec![(
+ Arc::new(Field::new("code", DataType::Int32, false)),
+ Arc::new(codes) as arrow_array::ArrayRef,
+ )]);
+ let values: Vec = (0..rows_per_frag * dim as usize)
+ .map(|i| (start * dim as usize + i) as f32)
+ .collect();
+ let vectors =
+ FixedSizeListArray::try_new_from_values(Float32Array::from(values), dim).unwrap();
+ RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(cats)),
+ Arc::new(meta),
+ Arc::new(vectors),
+ ],
+ )
+ .unwrap()
+ };
+ let reader = Box::new(RecordBatchIterator::new(
+ [Ok(make_batch(0))],
+ schema.clone(),
+ ));
+ let mut ds = Dataset::write(reader, "memory://covered_move_preserving", None)
+ .await
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new(
+ [Ok(make_batch(1))],
+ schema.clone(),
+ ));
+ ds.append(reader, None).await.unwrap();
+ // Scalar index on the join key so the merge joins via the indexed scan and
+ // reaches `rewrite_covered_update_as_move` (the full-scan join commits its
+ // move through the insert exec instead, which is not the producer under
+ // test here).
+ ds.create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+ let mut params = VectorIndexParams::ivf_pq(1, 8, 2, MetricType::L2, 2);
+ params.covering_columns(vec!["category".to_string()]);
+ ds.create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await
+ .unwrap();
+ let ds = Arc::new(ds);
+
+ // Partial covered-column update over fragment 1 -> takes the row-move path.
+ let frag1_start = rows_per_frag;
+ let ids: Vec = (frag1_start..frag1_start + rows_per_frag)
+ .map(|j| format!("id-{j:04}"))
+ .collect();
+ let new_cats: Vec = (frag1_start..frag1_start + rows_per_frag)
+ .map(|j| format!("updated-{j:04}"))
+ .collect();
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ ]));
+ let update_batch = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(new_cats)),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(update_batch)], sub_schema));
+ let (ds, _) = MergeInsertBuilder::try_new(ds, vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::UpdateAll)
+ .when_not_matched(WhenNotMatched::DoNothing)
+ .try_build()
+ .unwrap()
+ .execute_reader(reader)
+ .await
+ .unwrap();
+
+ let tx_path = ds
+ .manifest()
+ .transaction_file
+ .clone()
+ .expect("the covered move writes a transaction file");
+ let tx = read_transaction_file(ds.object_store.as_ref(), &ds.base, &tx_path)
+ .await
+ .unwrap();
+ let Operation::Update {
+ fields_for_preserving_frag_bitmap,
+ update_mode,
+ ..
+ } = &tx.operation
+ else {
+ panic!("expected Operation::Update, got: {:?}", tx.operation);
+ };
+ assert_eq!(*update_mode, Some(RewriteRows), "the move path committed");
+ let reported: HashSet = fields_for_preserving_frag_bitmap.iter().copied().collect();
+ let expected: HashSet = ds
+ .schema()
+ .fields_pre_order()
+ .map(|f| f.id as u32)
+ .collect();
+ let leaf_id = ds.schema().field("meta").unwrap().children[0].id as u32;
+ assert!(
+ reported.contains(&leaf_id),
+ "the preserving list must include nested LEAF ids (missing {leaf_id}): {reported:?}"
+ );
+ assert_eq!(
+ reported, expected,
+ "the covered move must report every field id, leaves included"
+ );
+ }
+
+ async fn build_moved_covering_dataset(uri: &str) -> (Arc, usize, usize, i32) {
+ let dim = 4i32;
+ let rows_per_frag = 256usize;
+ let num_frags = 3usize;
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new(
+ "vec",
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), dim),
+ false,
+ ),
+ ]));
+
+ let make_batch = |frag_idx: usize| {
+ let start = frag_idx * rows_per_frag;
+ let ids: Vec = (start..start + rows_per_frag)
+ .map(|j| format!("id-{j:04}"))
+ .collect();
+ let cats: Vec = (start..start + rows_per_frag)
+ .map(|j| format!("cat-{j:04}"))
+ .collect();
+ let values: Vec = (0..rows_per_frag * dim as usize)
+ .map(|i| (start * dim as usize + i) as f32)
+ .collect();
+ let vectors =
+ FixedSizeListArray::try_new_from_values(Float32Array::from(values), dim).unwrap();
+ RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(cats)),
+ Arc::new(vectors),
+ ],
+ )
+ .unwrap()
+ };
+
+ let reader = Box::new(RecordBatchIterator::new(
+ [Ok(make_batch(0))],
+ schema.clone(),
+ ));
+ let mut ds = Dataset::write(reader, uri, None).await.unwrap();
+ for frag_idx in 1..num_frags {
+ let reader = Box::new(RecordBatchIterator::new(
+ [Ok(make_batch(frag_idx))],
+ schema.clone(),
+ ));
+ ds.append(reader, None).await.unwrap();
+ }
+
+ // Scalar index on the join key so the merge joins via the indexed scan. The write
+ // strategy is unaffected: a covered-column update is always a row-move.
+ ds.create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+ let mut params = VectorIndexParams::ivf_pq(1, 8, 2, MetricType::L2, 2);
+ params.covering_columns(vec!["category".to_string()]);
+ ds.create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await
+ .unwrap();
+ let ds = Arc::new(ds);
+
+ // Partial merge_insert rewriting ONLY `category` for fragment 1 -> prunes
+ // fragment 1 from the vector index's coverage bitmap.
+ let frag1_start = rows_per_frag;
+ let ids: Vec = (frag1_start..frag1_start + rows_per_frag)
+ .map(|j| format!("id-{j:04}"))
+ .collect();
+ let new_cats: Vec = (frag1_start..frag1_start + rows_per_frag)
+ .map(|j| format!("updated-{j:04}"))
+ .collect();
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ ]));
+ let update_batch = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(new_cats)),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(update_batch)], sub_schema));
+ let (ds, _) = MergeInsertBuilder::try_new(ds, vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::UpdateAll)
+ .when_not_matched(WhenNotMatched::DoNothing)
+ .try_build()
+ .unwrap()
+ .execute_reader(reader)
+ .await
+ .unwrap();
+
+ // Precondition every caller relies on: the covering column really is recorded on
+ // the index metadata, since the move/prune logic keys on it.
+ let category_field_id = ds.schema().field("category").unwrap().id;
+ let indices = ds.load_indices().await.unwrap();
+ let vec_idx = indices.iter().find(|i| i.name == "vec_idx").unwrap();
+ assert_eq!(vec_idx.covering_fields, vec![category_field_id]);
+
+ (ds, num_frags * rows_per_frag, rows_per_frag, dim)
+ }
+
+ /// A covered query over every row must return each row exactly once (no stale duplicate
+ /// from the old storage) with the fresh `category` value.
+ async fn assert_covered_query_no_duplicates_fresh(
+ ds: &Dataset,
+ total_rows: usize,
+ rows_per_frag: usize,
+ dim: i32,
+ ) {
+ use arrow_array::cast::AsArray;
+ use arrow_array::types::UInt64Type;
+ use lance_core::ROW_ID;
+ use std::collections::HashSet;
+
+ let frag1_start = rows_per_frag;
+ let query: Float32Array = (0..dim)
+ .map(|i| (frag1_start * dim as usize + i as usize) as f32)
+ .collect();
+ let results = ds
+ .scan()
+ .nearest("vec", &query, total_rows)
+ .unwrap()
+ .with_row_id()
+ .project(&["id", "category"])
+ .unwrap()
+ .try_into_batch()
+ .await
+ .unwrap();
+
+ let rowids = results[ROW_ID].as_primitive::();
+ let unique: HashSet = rowids.values().iter().copied().collect();
+ assert_eq!(
+ unique.len(),
+ rowids.len(),
+ "optimize double-counted the pruned fragment: duplicate row ids in covered results"
+ );
+ assert_eq!(
+ results.num_rows(),
+ total_rows,
+ "covered query returned {} rows, expected {total_rows} (dropped/duplicated rows)",
+ results.num_rows()
+ );
+
+ let ids_col = results["id"].as_string::();
+ let cats = results["category"].as_string::();
+ for i in 0..ids_col.len() {
+ let id = ids_col.value(i);
+ let n: usize = id[3..].parse().unwrap();
+ let expected = if (frag1_start..frag1_start + rows_per_frag).contains(&n) {
+ format!("updated-{n:04}")
+ } else {
+ format!("cat-{n:04}")
+ };
+ assert_eq!(cats.value(i), expected, "stale covering value for row {id}");
+ }
+ }
+
+ /// After a covered-column update moves rows out of a fragment, an optimize must not
+ /// resurrect the stale copies still sitting in the old segment's storage. `merge`
+ /// re-scans the moved rows as unindexed data *and* folds in the old segment, so it must
+ /// not double-count them; the default (append) optimize leaves the old segment in place,
+ /// so the deletion mask must keep hiding them.
+ #[rstest::rstest]
+ #[case::merge(lance_index::optimize::OptimizeOptions::merge(1))]
+ #[case::append_default(lance_index::optimize::OptimizeOptions::default())]
+ #[tokio::test]
+ async fn test_optimize_after_covering_move_no_duplicate_rows(
+ #[case] options: lance_index::optimize::OptimizeOptions,
+ ) {
+ let (ds, total_rows, rows_per_frag, dim) =
+ build_moved_covering_dataset("memory://covered_opt").await;
+ let mut ds = Arc::try_unwrap(ds).unwrap_or_else(|arc| (*arc).clone());
+ ds.optimize_indices(&options).await.unwrap();
+ assert_covered_query_no_duplicates_fresh(&ds, total_rows, rows_per_frag, dim).await;
+ }
+
+ /// The covered move cannot carry stable row ids onto its new fragments -- it never rechunks
+ /// the captured `row_id_sequence` the way the full-schema path does, so `assign_row_ids`
+ /// would mint fresh ids for every moved row and break the one guarantee stable row ids exist
+ /// to provide. The move is an optimisation over an already-correct in-place patch, so this
+ /// must degrade to that patch rather than fail the merge.
+ ///
+ /// The assertions are the property the old rejection protected, stated positively: the merge
+ /// SUCCEEDS, every row keeps the row id it had (the in-place patch moves no rows, so ids are
+ /// preserved by construction -- strictly safer here than the move), and confinement was the
+ /// only thing given up, i.e. the updated fragment left the vector index's bitmap. Run against
+ /// a real directory rather than `memory://` so the ids are checked after a genuine reopen.
+ #[tokio::test]
+ async fn test_merge_insert_covered_update_falls_back_on_stable_row_ids() {
+ use arrow_array::types::UInt64Type;
+ use lance_core::ROW_ID;
+
+ /// `id -> _rowid` for the whole dataset, so "ids were preserved" is provable per row
+ /// rather than as a count.
+ async fn row_ids_by_key(ds: &Dataset) -> HashMap {
+ let batch = ds
+ .scan()
+ .with_row_id()
+ .project(&["id"])
+ .unwrap()
+ .try_into_batch()
+ .await
+ .unwrap();
+ let ids = batch["id"].as_string::();
+ let row_ids = batch[ROW_ID].as_primitive::();
+ (0..batch.num_rows())
+ .map(|i| (ids.value(i).to_string(), row_ids.value(i)))
+ .collect()
+ }
+
+ let test_uri = TempStrDir::default();
+ let dim = 4i32;
+ let n = 256usize; // enough rows to train PQ
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new(
+ "vec",
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), dim),
+ false,
+ ),
+ ]));
+ let ids: Vec = (0..n).map(|j| format!("id-{j:04}")).collect();
+ let cats: Vec = (0..n).map(|j| format!("cat-{j:04}")).collect();
+ let values: Vec = (0..n * dim as usize).map(|i| i as f32).collect();
+ let vectors =
+ FixedSizeListArray::try_new_from_values(Float32Array::from(values), dim).unwrap();
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids.clone())),
+ Arc::new(StringArray::from(cats)),
+ Arc::new(vectors),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(batch)], schema.clone()));
+ let mut ds = Dataset::write(
+ reader,
+ &test_uri,
+ Some(WriteParams {
+ enable_stable_row_ids: true,
+ ..Default::default()
+ }),
+ )
+ .await
+ .unwrap();
+ ds.create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+ let mut params = VectorIndexParams::ivf_pq(1, 8, 2, MetricType::L2, 2);
+ params.covering_columns(vec!["category".to_string()]);
+ ds.create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await
+ .unwrap();
+
+ // Partial merge_insert updating only the covered `category`.
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ ]));
+ let update = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(
+ (0..n).map(|j| format!("new-{j:04}")).collect::>(),
+ )),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(update)], sub_schema));
+ let row_ids_before = row_ids_by_key(&ds).await;
+ let updated_frag = ds.get_fragments().iter().map(|f| f.id()).max().unwrap();
+ let (ds, _) = MergeInsertBuilder::try_new(Arc::new(ds), vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::UpdateAll)
+ .when_not_matched(WhenNotMatched::DoNothing)
+ .try_build()
+ .unwrap()
+ .execute_reader(reader)
+ .await
+ .expect("a covered update on a stable-row-id dataset must fall back, not fail");
+
+ // Reopen so the ids are read back from committed bytes, not from the in-memory handle.
+ let reopened = Dataset::open(test_uri.as_ref()).await.unwrap();
+ let after = reopened
+ .scan()
+ .project(&["id", "category"])
+ .unwrap()
+ .try_into_batch()
+ .await
+ .unwrap();
+ assert_eq!(after.num_rows(), n, "the fallback must not lose rows");
+ let cats = after["category"].as_string::();
+ assert!(
+ (0..cats.len()).all(|i| cats.value(i).starts_with("new-")),
+ "the fallback must still apply the update"
+ );
+
+ // The property the old rejection existed to protect: no row changed identity. The
+ // in-place patch moves no rows, so every id survives, paired with the same key.
+ let row_ids_after = row_ids_by_key(&reopened).await;
+ assert_eq!(
+ row_ids_before, row_ids_after,
+ "falling back to the in-place patch must preserve every stable row id"
+ );
+
+ // And the only thing forfeited is confinement: the updated fragment left the bitmap.
+ let indices = ds.load_indices().await.unwrap();
+ let vec_idx = indices.iter().find(|i| i.name == "vec_idx").unwrap();
+ let bitmap = vec_idx.fragment_bitmap.as_ref().unwrap();
+ assert!(
+ !bitmap.contains(updated_frag as u32),
+ "the in-place fallback should have evicted fragment {updated_frag} from the vector \
+ index; if it is still covered the merge did not take the fallback and this test is \
+ not exercising it: {bitmap:?}"
+ );
+ }
+
+ /// A partial-schema merge that does NOT update matched rows (DoNothing + InsertAll)
+ /// must not be treated as a covered-column update, even when the source carries a
+ /// covered column -- matched rows are untouched and unmatched rows enter unindexed
+ /// fragments, so nothing is patched into the index.
+ #[tokio::test]
+ async fn test_merge_insert_covered_donothing_insert_not_rejected() {
+ let dim = 4i32;
+ let n = 20usize;
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new(
+ "vec",
+ // nullable so InsertAll with a partial source is permitted
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), dim),
+ true,
+ ),
+ ]));
+ let ids: Vec = (0..n).map(|j| format!("id-{j:04}")).collect();
+ let cats: Vec = (0..n).map(|j| format!("cat-{j:04}")).collect();
+ let values: Vec = (0..n * dim as usize).map(|i| i as f32).collect();
+ let vectors =
+ FixedSizeListArray::try_new_from_values(Float32Array::from(values), dim).unwrap();
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(cats)),
+ Arc::new(vectors),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(batch)], schema.clone()));
+ let mut ds = Dataset::write(reader, "memory://covered_donothing", None)
+ .await
+ .unwrap();
+ ds.create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+ let mut params = VectorIndexParams::ivf_flat(2, MetricType::L2);
+ params.covering_columns(vec!["category".to_string()]);
+ ds.create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await
+ .unwrap();
+
+ // DoNothing on matched + InsertAll unmatched, partial source (id + category)
+ // including new rows to insert.
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ ]));
+ let new_ids: Vec = (n..n + 5).map(|j| format!("id-{j:04}")).collect();
+ let new_cats: Vec = (n..n + 5).map(|j| format!("cat-{j:04}")).collect();
+ let update = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(StringArray::from(new_ids)),
+ Arc::new(StringArray::from(new_cats)),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(update)], sub_schema));
+ let result = MergeInsertBuilder::try_new(Arc::new(ds), vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::DoNothing)
+ .when_not_matched(WhenNotMatched::InsertAll)
+ .try_build()
+ .unwrap()
+ .execute_reader(reader)
+ .await;
+ assert!(
+ result.is_ok(),
+ "DoNothing + InsertAll with a covered column in a partial source must not be \
+ rejected as a covered update; got {:?}",
+ result.err()
+ );
+ }
+
+ /// An upsert touching a covered column -- `UpdateAll` matched plus `InsertAll` unmatched --
+ /// must fall back to the in-place patch. The move reads row addresses through the null mask,
+ /// and the unmatched group arrives with a NULL `_rowaddr`, so entering the move would
+ /// tombstone unrelated rows at address 0. Never entering it prevents that as completely as
+ /// refusing the merge did, and `update_fragments` already routes the NULL group to
+ /// `handle_new_fragments`.
+ ///
+ /// The data-loss assertion is the point: the row count must be exactly the original plus the
+ /// inserted row, and every pre-existing row must still be there. That is what a bogus
+ /// address-0 tombstone would break.
+ #[tokio::test]
+ async fn test_merge_insert_covered_update_with_insert_falls_back() {
+ let dim = 4i32;
+ let n = 20usize;
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new(
+ "vec",
+ // nullable so InsertAll with a partial source is permitted
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), dim),
+ true,
+ ),
+ ]));
+ let ids: Vec = (0..n).map(|j| format!("id-{j:04}")).collect();
+ let cats: Vec = (0..n).map(|j| format!("cat-{j:04}")).collect();
+ let values: Vec = (0..n * dim as usize).map(|i| i as f32).collect();
+ let vectors =
+ FixedSizeListArray::try_new_from_values(Float32Array::from(values), dim).unwrap();
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(cats)),
+ Arc::new(vectors),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(batch)], schema.clone()));
+ let mut ds = Dataset::write(reader, "memory://covered_update_insert", None)
+ .await
+ .unwrap();
+ ds.create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+ let mut params = VectorIndexParams::ivf_flat(2, MetricType::L2);
+ params.covering_columns(vec!["category".to_string()]);
+ ds.create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await
+ .unwrap();
+
+ // Partial source mixing an update to an existing row with a brand-new row.
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ ]));
+ let update = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(StringArray::from(vec![
+ "id-0001".to_string(),
+ format!("id-{:04}", n),
+ ])),
+ Arc::new(StringArray::from(vec![
+ "updated-0001".to_string(),
+ "brand-new".to_string(),
+ ])),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(update)], sub_schema));
+ let updated_frag = ds.get_fragments().iter().map(|f| f.id()).max().unwrap();
+ let (ds, stats) = MergeInsertBuilder::try_new(Arc::new(ds), vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::UpdateAll)
+ .when_not_matched(WhenNotMatched::InsertAll)
+ .try_build()
+ .unwrap()
+ .execute_reader(reader)
+ .await
+ .expect("a covered update carrying inserts must fall back, not fail");
+ assert_eq!(stats.num_updated_rows, 1);
+ assert_eq!(stats.num_inserted_rows, 1);
+
+ // No row was tombstoned at a bogus address: exactly the original rows plus the insert,
+ // with every original key still present.
+ let after = ds
+ .scan()
+ .project(&["id", "category"])
+ .unwrap()
+ .try_into_batch()
+ .await
+ .unwrap();
+ assert_eq!(
+ after.num_rows(),
+ n + 1,
+ "the fallback must insert the new row and tombstone nothing else"
+ );
+ let ids_col = after["id"].as_string::();
+ let cats_col = after["category"].as_string::();
+ let found: HashMap<&str, &str> = (0..after.num_rows())
+ .map(|i| (ids_col.value(i), cats_col.value(i)))
+ .collect();
+ for j in 0..n {
+ assert!(
+ found.contains_key(format!("id-{j:04}").as_str()),
+ "the fallback dropped pre-existing row id-{j:04}"
+ );
+ }
+ assert_eq!(
+ found["id-0001"], "updated-0001",
+ "the update was not applied"
+ );
+ assert_eq!(
+ found[format!("id-{n:04}").as_str()],
+ "brand-new",
+ "the insert was not applied"
+ );
+
+ // Confinement is what was forfeited.
+ let indices = ds.load_indices().await.unwrap();
+ let vec_idx = indices.iter().find(|i| i.name == "vec_idx").unwrap();
+ let bitmap = vec_idx.fragment_bitmap.as_ref().unwrap();
+ assert!(
+ !bitmap.contains(updated_frag as u32),
+ "the in-place fallback should have evicted fragment {updated_frag} from the vector \
+ index; still covered means the fallback was not taken: {bitmap:?}"
+ );
+ }
+
+ /// The covered-column row-move re-reads full rows via a take, which cannot round-trip
+ /// legacy (v1) blob columns as binary, so a dataset carrying one must fall back to the
+ /// in-place patch -- which never reads the blob column at all. The assertion that matters
+ /// is therefore that the blob survives the merge intact, not that the merge was refused.
+ #[tokio::test]
+ async fn test_merge_insert_covered_update_falls_back_with_v1_blob() {
+ use arrow_array::LargeBinaryArray;
+ use lance_arrow::BLOB_META_KEY;
+
+ let dim = 4i32;
+ let n = 20usize;
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new(
+ "vec",
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), dim),
+ true,
+ ),
+ Field::new("blobs", DataType::LargeBinary, true).with_metadata(HashMap::from([(
+ BLOB_META_KEY.to_string(),
+ "true".to_string(),
+ )])),
+ ]));
+ let ids: Vec = (0..n).map(|j| format!("id-{j:04}")).collect();
+ let cats: Vec = (0..n).map(|j| format!("cat-{j:04}")).collect();
+ let blobs: Vec> = (0..n).map(|_| Some(b"payload".as_slice())).collect();
+ let values: Vec = (0..n * dim as usize).map(|i| i as f32).collect();
+ let vectors =
+ FixedSizeListArray::try_new_from_values(Float32Array::from(values), dim).unwrap();
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(cats)),
+ Arc::new(vectors),
+ Arc::new(LargeBinaryArray::from(blobs)),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(batch)], schema.clone()));
+ let mut ds = Dataset::write(
+ reader,
+ "memory://covered_v1_blob",
+ Some(WriteParams {
+ data_storage_version: Some(LanceFileVersion::V2_1),
+ ..Default::default()
+ }),
+ )
+ .await
+ .unwrap();
+ ds.create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+ let mut params = VectorIndexParams::ivf_flat(2, MetricType::L2);
+ params.covering_columns(vec!["category".to_string()]);
+ ds.create_index(&["vec"], IndexType::Vector, None, ¶ms, false)
+ .await
+ .unwrap();
+
+ // Partial source updating the covered `category` on matched rows only.
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ ]));
+ let upd_ids: Vec = (0..3).map(|j| format!("id-{j:04}")).collect();
+ let upd_cats: Vec = (0..3).map(|j| format!("new-{j:04}")).collect();
+ let update = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(StringArray::from(upd_ids)),
+ Arc::new(StringArray::from(upd_cats)),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(update)], sub_schema));
+ let updated_frag = ds.get_fragments().iter().map(|f| f.id()).max().unwrap();
+ // A v1 blob column scans as a position/size description struct, not binary -- writing
+ // those descriptors back as data is exactly the corruption the move would cause. Capture
+ // them so "the blob was left alone" is provable rather than assumed.
+ let blobs_before = ds
+ .scan()
+ .project(&["blobs"])
+ .unwrap()
+ .try_into_batch()
+ .await
+ .unwrap();
+ let (ds, _) = MergeInsertBuilder::try_new(Arc::new(ds), vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::UpdateAll)
+ .when_not_matched(WhenNotMatched::DoNothing)
+ .try_build()
+ .unwrap()
+ .execute_reader(reader)
+ .await
+ .expect("a covered update on a v1-blob dataset must fall back, not fail");
+
+ // The update landed, and the blob the move would have corrupted is untouched. Read the
+ // blobs through the binary path the v1 column is meant to be read by.
+ let after = ds
+ .scan()
+ .project(&["id", "category"])
+ .unwrap()
+ .try_into_batch()
+ .await
+ .unwrap();
+ assert_eq!(after.num_rows(), n);
+ let ids_col = after["id"].as_string::();
+ let cats_col = after["category"].as_string::();
+ for i in 0..after.num_rows() {
+ let key: usize = ids_col.value(i)[3..].parse().unwrap();
+ let expected = if key < 3 {
+ format!("new-{key:04}")
+ } else {
+ format!("cat-{key:04}")
+ };
+ assert_eq!(cats_col.value(i), expected, "wrong category after fallback");
+ }
+ let blobs_after = ds
+ .scan()
+ .project(&["blobs"])
+ .unwrap()
+ .try_into_batch()
+ .await
+ .unwrap();
+ assert_eq!(
+ blobs_before, blobs_after,
+ "the in-place fallback must leave the v1 blob column untouched; the move would have \
+ rewritten these descriptors into a new fragment as ordinary struct data"
+ );
+ // And the bytes are still readable through the blob API they were written with.
+ let blob_handles = ds
+ .take_blobs_by_indices(&(0..n as u64).collect::>(), "blobs")
+ .await
+ .unwrap();
+ assert_eq!(blob_handles.len(), n);
+ assert!(
+ blob_handles.iter().all(|b| b
+ .as_ref()
+ .is_some_and(|b| b.size() == b"payload".len() as u64)),
+ "every v1 blob must still read back at its original size"
+ );
+
+ // Confinement is what was forfeited.
+ let indices = ds.load_indices().await.unwrap();
+ let vec_idx = indices.iter().find(|i| i.name == "vec_idx").unwrap();
+ let bitmap = vec_idx.fragment_bitmap.as_ref().unwrap();
+ assert!(
+ !bitmap.contains(updated_frag as u32),
+ "the in-place fallback should have evicted fragment {updated_frag} from the vector \
+ index; still covered means the fallback was not taken: {bitmap:?}"
+ );
+ }
+
+ // Regression test: after a partial-schema merge_insert drops a fragment from the FTS
+ // index bitmap, a full text search should not return duplicate rows. The stale inverted
+ // index data still references the dropped fragment, and the scanner also flat-scans
+ // unindexed fragments, causing the same rows to appear from both paths.
+ #[tokio::test]
+ async fn test_partial_merge_insert_stale_fts_index_duplicates() {
+ let rows_per_frag = 10usize;
+ let num_frags = 3usize;
+
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("category", DataType::Utf8, false),
+ Field::new("text", DataType::Utf8, false),
+ ]));
+
+ let make_batch = |frag_idx: usize| {
+ let start = frag_idx * rows_per_frag;
+ let ids: Vec = (start..start + rows_per_frag)
+ .map(|j| format!("id-{j:04}"))
+ .collect();
+ let cats: Vec<&str> = vec!["A"; rows_per_frag];
+ // Every row contains "common" so we can search for it and expect all rows
+ let texts: Vec = (start..start + rows_per_frag)
+ .map(|j| format!("common unique{j:04}"))
+ .collect();
+ RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(cats)),
+ Arc::new(StringArray::from(texts)),
+ ],
+ )
+ .unwrap()
+ };
+
+ // Write 3 fragments
+ let batch0 = make_batch(0);
+ let reader = Box::new(RecordBatchIterator::new([Ok(batch0)], schema.clone()));
+ let mut ds = Dataset::write(reader, "memory://fts_stale_test", None)
+ .await
+ .unwrap();
+ for frag_idx in 1..num_frags {
+ let batch = make_batch(frag_idx);
+ let reader = Box::new(RecordBatchIterator::new([Ok(batch)], schema.clone()));
+ ds.append(reader, None).await.unwrap();
+ }
+
+ // Create inverted index on text
+ let params = InvertedIndexParams::default();
+ ds.create_index(&["text"], IndexType::Inverted, None, ¶ms, true)
+ .await
+ .unwrap();
+
+ let ds = Arc::new(ds);
+
+ // Partial merge_insert with (id, text) on fragment 1 rows.
+ // Text still contains "common" so FTS will find them via both paths.
+ // This drops fragment 1 from the inverted index bitmap.
+ let frag1_start = rows_per_frag;
+ let ids: Vec = (frag1_start..frag1_start + rows_per_frag)
+ .map(|j| format!("id-{j:04}"))
+ .collect();
+ let texts: Vec = (frag1_start..frag1_start + rows_per_frag)
+ .map(|j| format!("common updated{j:04}"))
+ .collect();
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("text", DataType::Utf8, false),
+ ]));
+ let update_batch = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(StringArray::from(ids)),
+ Arc::new(StringArray::from(texts)),
+ ],
+ )
+ .unwrap();
+ let reader = Box::new(RecordBatchIterator::new([Ok(update_batch)], sub_schema));
+ let (ds, _) = MergeInsertBuilder::try_new(ds, vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::UpdateAll)
+ .when_not_matched(WhenNotMatched::DoNothing)
+ .try_build()
+ .unwrap()
+ .execute_reader(reader)
+ .await
+ .unwrap();
+
+ // FTS search for "common" — every row should match exactly once
+ let query = FullTextSearchQuery::new("common".to_string());
+ let results = ds
+ .scan()
+ .full_text_search(query)
+ .unwrap()
+ .try_into_batch()
+ .await
+ .unwrap();
+
// Check no duplicate ids
let ids = results
.column_by_name("id")
@@ -14921,11 +17071,11 @@ MergeInsert: on=[id], when_matched=DoNothing, when_not_matched=InsertAll, when_n
#[tokio::test]
async fn test_merge_insert_with_blob_v1_source_provides_blob() {
use arrow_array::LargeBinaryArray;
- use arrow_schema::Schema as ArrowSchema;
+ use arrow_schema::Schema;
use lance_arrow::BLOB_META_KEY;
let test_dir = TempStrDir::default();
- let schema = Arc::new(ArrowSchema::new(vec![
+ let schema = Arc::new(Schema::new(vec![
Field::new("blobs", DataType::LargeBinary, true).with_metadata(HashMap::from([(
BLOB_META_KEY.to_string(),
"true".to_string(),
@@ -15000,10 +17150,10 @@ MergeInsert: on=[id], when_matched=DoNothing, when_not_matched=InsertAll, when_n
#[tokio::test]
async fn test_merge_insert_with_blob_v2_source_provides_blob() {
use crate::{BlobArrayBuilder, blob_field};
- use arrow_schema::Schema as ArrowSchema;
+ use arrow_schema::Schema;
let test_dir = TempStrDir::default();
- let schema = Arc::new(ArrowSchema::new(vec![
+ let schema = Arc::new(Schema::new(vec![
blob_field("blobs", true),
Field::new("id", DataType::Int64, true),
Field::new("other", DataType::Int64, true),
diff --git a/rust/lance/src/index.rs b/rust/lance/src/index.rs
index b5803624520..ada522328bb 100644
--- a/rust/lance/src/index.rs
+++ b/rust/lance/src/index.rs
@@ -137,7 +137,7 @@ fn validate_segment_metadata(index_name: &str, segments: &[IndexMetadata]) -> Re
Ok(())
}
-fn collect_subtree_field_ids(field: &Field, field_ids: &mut HashSet) {
+pub(crate) fn collect_subtree_field_ids(field: &Field, field_ids: &mut HashSet) {
field_ids.insert(field.id);
for child in &field.children {
collect_subtree_field_ids(child, field_ids);
@@ -163,7 +163,16 @@ struct PhysicalDataFileIdentity<'a> {
base_id: Option,
base_binding: PhysicalBaseBinding<'a>,
path: &'a str,
- fields: &'a [i32],
+ // Deliberately NOT the file's `fields` list. This identity is built per field, but
+ // `fields` is file-wide, so a tombstone of any *other* field in a shared file (an
+ // unrelated `RewriteColumns` turns its id negative) would change every field's
+ // identity and prune coverage for indexes whose data did not move. A tombstone of
+ // *this* field is still caught: its id stops matching and it drops out of the map
+ // `fragment_field_files` builds. `column_indices` covers an actual layout change.
+ //
+ // Upstream cannot hit this without a covering producer -- with no payload written,
+ // two indexes never share a file with different field sets -- which is why the guard
+ // held until this commit introduced one.
column_indices: &'a [i32],
file_major_version: u32,
file_minor_version: u32,
@@ -185,7 +194,6 @@ impl<'a> PhysicalDataFileIdentity<'a> {
base_id: file.base_id,
base_binding,
path: &file.path,
- fields: file.fields.as_ref(),
column_indices: file.column_indices.as_ref(),
file_major_version: file.file_major_version,
file_minor_version: file.file_minor_version,
@@ -265,7 +273,32 @@ async fn prune_stale_segment_coverage(
.iter_mut()
.filter(|segment| segment.dataset_version() == version)
{
- let indexed_field_ids = segment_indexed_field_ids(dataset, segment)?;
+ // `segment_indexed_field_ids` already walks every entry of `segment.fields()`,
+ // keyed and carried alike -- `covering_fields` is always the trailing slice of
+ // `fields` -- so no separate expansion over `covering_fields()` is needed here.
+ let relevant_field_ids = segment_indexed_field_ids(dataset, segment)?;
+ for included_id in segment.covering_fields() {
+ // A covered field whose subtree changed between the segment's build version
+ // and now -- e.g. a struct that gained a child via `add_columns` (which
+ // commits while this segment is still uncommitted, so the Merge-commit guard
+ // never saw it) -- makes the segment's stored covering payload type-
+ // incompatible with the current schema for *every* row it covers. No
+ // per-fragment file-path prune below can repair a global type change, so
+ // reject the commit; the segment must be rebuilt against the current schema.
+ if Transaction::covered_field_subtree_changed(
+ historical.schema(),
+ dataset.schema(),
+ *included_id,
+ ) {
+ return Err(Error::invalid_input(format!(
+ "CreateIndex: covered (included) field id {included_id} changed its \
+ subtree between the segment's build version {version} and the current \
+ dataset version {}; the segment's stored covering payload no longer \
+ matches the schema. Rebuild the segment against the current schema.",
+ dataset.manifest.version
+ )));
+ }
+ }
let stale_fragments = segment
.fragment_bitmap()
.iter()
@@ -277,9 +310,9 @@ async fn prune_stale_segment_coverage(
return true;
};
let historical_files =
- fragment_field_files(&historical, historical_fragment, &indexed_field_ids);
+ fragment_field_files(&historical, historical_fragment, &relevant_field_ids);
let current_files =
- fragment_field_files(dataset, current_fragment, &indexed_field_ids);
+ fragment_field_files(dataset, current_fragment, &relevant_field_ids);
let changed_files =
historical_files.is_none() || historical_files != current_files;
let changed_overlays = prune_newer_overlays
@@ -289,7 +322,7 @@ async fn prune_stale_segment_coverage(
.data_file
.fields
.iter()
- .any(|field_id| indexed_field_ids.contains(field_id))
+ .any(|field_id| relevant_field_ids.contains(field_id))
});
changed_files || changed_overlays
})
@@ -1170,34 +1203,21 @@ pub(crate) async fn remap_index(
// withdrawal below would otherwise swallow it as an ordinary covered index.
matched.validate_covering_fields()?;
- // A covered index cannot survive a remap yet. Nothing writes the carried
- // values in the first place, and each index type's `remap` rewrites only the
- // schema that type knows about, so the remapped segment would still declare
- // payload its storage no longer holds. Withdraw it instead, exactly as an
- // index whose type reports `can_remap() == false` is withdrawn below: an
- // absent index costs a fallback scan, whereas a surviving false declaration
- // is answered from data that is not there. Erroring here would instead block
- // compaction of the whole table.
- if !matched.covering_fields.is_empty() {
- log::warn!(
- "Index '{}' declares covering fields {:?}, which no index builder \
- writes or preserves yet. Index will be dropped during compaction \
- and must be rebuilt.",
- matched.name,
- matched.covering_fields,
- );
- return Ok(RemapResult::Drop);
- }
-
- // With covering withdrawn above, `fields` is entirely keyed, so more than one
- // entry means a genuinely composite index.
- if matched.fields.len() > 1 {
+ // Remap handles a single keyed field. Carried fields are irrelevant to the
+ // remap itself: the index is opened by its keyed field, and each storage
+ // format's `remap` preserves its covering columns row-aligned (the format
+ // commit dropped covered indexes here because nothing wrote carried values
+ // yet; the builders in this commit do). Only a genuinely composite index is
+ // rejected -- `keyed_fields` fails closed on malformed metadata, but the
+ // validation above already rejected that.
+ if matched.keyed_fields().len() > 1 {
return Err(Error::index(format!(
- "Remapping index '{}' is not supported: it has {} keyed fields {:?}; \
- only one keyed field is supported",
+ "Remapping index '{}' is not supported: it has {} keyed fields {:?} \
+ (carried fields {:?}); only one keyed field is supported",
matched.name,
- matched.fields.len(),
+ matched.keyed_fields().len(),
matched.fields,
+ matched.covering_fields,
)));
}
@@ -2311,42 +2331,6 @@ impl DatasetIndexExt for Dataset {
continue;
}
- // Optimizing a covered index would republish its declaration on a
- // segment rebuilt without the carried values: `scan_vector_fragments`
- // projects the keyed field and `_rowid` only, and the scalar merges
- // reconstruct value plus row id.
- //
- // What decides is the caller's intent, not whether this group is
- // stale. An unfiltered `optimize_indices()` is a table-wide
- // maintenance request, and erroring aborts the loop before the
- // replacements accumulated for the other groups are committed -- so
- // one index this build cannot rebuild would leave every other index
- // on the table stale. Skip it with a warning instead.
- //
- // A caller that listed this index in `index_names` asked for it
- // specifically, so refuse out loud. The loop is already filtered by
- // that list, so reaching here with it set means this group was named.
- if let Some(covered) = deltas
- .iter()
- .find(|index| !index.covering_fields.is_empty())
- {
- if options.index_names.is_none() {
- log::warn!(
- "Skipping index '{}': it declares covering fields {:?}, \
- which no index builder writes or preserves yet.",
- covered.name,
- covered.covering_fields,
- );
- continue;
- }
- return Err(Error::index(format!(
- "Optimizing index '{}' is not supported: it declares \
- covering fields {:?}, which no index builder writes or \
- preserves yet",
- covered.name, covered.covering_fields,
- )));
- }
-
// Optimizing a name means replacing its segments with one that
// covers their union, which this build cannot compute when it
// cannot read one of them: the merged segment would overlap the
@@ -3637,6 +3621,18 @@ impl DatasetIndexInternalExt for Dataset {
)));
}
+ // `field_paths` is already the keyed prefix -- it was built from `keyed_fields()`
+ // above -- so a carried column is deliberately NOT resolved or type-checked here.
+ // That is the point: the rebuild writes fresh, non-covered metadata and never reads
+ // the carried payload, so demanding those columns exist in the target would reject
+ // a target that can hold this index perfectly well.
+ //
+ // `initialize_vector_index` then re-resolves the carried columns itself, by name,
+ // against the target schema. `initialize_scalar_index` does NOT -- it recreates
+ // with default params and never reads `covering_fields`, so a covered scalar index
+ // would land on the target with covering silently dropped. Unreachable today:
+ // `covering_columns` exists only on `VectorIndexParams`, so nothing produces a
+ // covered scalar index. Whoever adds a scalar producer must fix that path too.
let field_names = field_paths.iter().map(String::as_str).collect::>();
if let Some(index_details) = &source_index.index_details {
let index_details_wrapper = IndexDetails(index_details.clone());
@@ -6567,6 +6563,90 @@ mod tests {
assert_eq!(desc.rows_indexed(), 10);
}
+ /// `describe_indices` with a column criteria must still find a covered index.
+ ///
+ /// `index_matches_criteria` is documented to receive only the column named by
+ /// `for_column` -- it rejects anything else with `fields.len() != 1`. A covered
+ /// index lists its carried columns in `fields` too, so resolving all of `fields`
+ /// silently fails that check and drops the index from the description with no
+ /// error: `describe_indices(for_column)` reports "no index on this column" for a
+ /// column that is indexed.
+ #[tokio::test]
+ async fn test_describe_indices_for_column_finds_covered_index() {
+ 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::())
+ .col(
+ "vector",
+ array::rand_vec::(8.into()),
+ )
+ .into_reader_rows(RowCount::from(20), BatchCount::from(1));
+ let mut dataset = Dataset::write(reader, test_uri, None).await.unwrap();
+
+ let mut params = crate::index::vector::VectorIndexParams::ivf_flat(
+ 1,
+ lance_linalg::distance::MetricType::L2,
+ );
+ params.covering_columns(vec!["id".to_string()]);
+ dataset
+ .create_index(
+ &["vector"],
+ IndexType::Vector,
+ Some("covered_idx".to_string()),
+ ¶ms,
+ false,
+ )
+ .await
+ .unwrap();
+ let covered = dataset.load_indices_by_name("covered_idx").await.unwrap();
+ assert_eq!(covered[0].fields.len(), 2, "index should be covered");
+
+ let descriptions = dataset
+ .describe_indices(Some(IndexCriteria::default().for_column("vector")))
+ .await
+ .unwrap();
+ assert_eq!(
+ descriptions.len(),
+ 1,
+ "a covered index must still be described for its keyed column"
+ );
+ assert_eq!(descriptions[0].name(), "covered_idx");
+
+ // The carried column is not the indexed column, so it must not match.
+ let by_carried = dataset
+ .describe_indices(Some(IndexCriteria::default().for_column("id")))
+ .await
+ .unwrap();
+ assert!(
+ by_carried.is_empty(),
+ "a carried column is not indexed and must not match `for_column`"
+ );
+
+ // `field_ids()` is a capability statement -- "the fields the index is built on" --
+ // so it must report the keyed column only. Reporting the carried column here tells
+ // every consumer (Python `describe_indices().fields`, Java `getFieldIds()`, the
+ // namespace REST `columns` list) that this index can serve `id`; Python's
+ // `_default_vector_index_for_column` then hands back `vec`'s IVF centroids for the
+ // int32 column `id` instead of raising.
+ let vector_field_id = dataset.schema().field("vector").unwrap().id as u32;
+ let id_field_id = dataset.schema().field("id").unwrap().id as u32;
+ let desc = &descriptions[0];
+ assert_eq!(
+ desc.field_ids(),
+ &[vector_field_id],
+ "field_ids() must report the keyed column only, not the carried one"
+ );
+ // The carried set is not lost -- it stays reachable per segment.
+ assert_eq!(
+ desc.metadata()[0].covering_fields,
+ vec![id_field_id as i32],
+ "the carried column must remain reachable via metadata()[i].covering_fields"
+ );
+ }
+
#[tokio::test]
async fn test_describe_indices_derives_type_from_url_without_plugin() {
// When index details exist but no plugin is registered for the type
@@ -7959,6 +8039,68 @@ mod tests {
);
}
+ /// `initialize_index` must copy a covered index, not reject it.
+ ///
+ /// It derives the column names it initializes from `source_index.fields`, and a
+ /// covered index lists its carried columns there too. Both initializers accept
+ /// exactly one name (they re-resolve the carried columns themselves from
+ /// `covering_fields`), so passing all of `fields` makes every covered index
+ /// un-copyable -- and `initialize_indices` then fails the whole dataset copy.
+ #[tokio::test]
+ async fn test_initialize_index_copies_covered_index() {
+ use arrow_array::types::Float32Type;
+ use lance_datagen::{BatchCount, RowCount, array};
+
+ let test_dir = TempStrDir::default();
+ let source_uri = format!("{}/{}", test_dir, "source");
+ let target_uri = format!("{}/{}", test_dir, "target");
+
+ let make_reader = || {
+ lance_datagen::gen_batch()
+ .col("id", array::step::())
+ .col("vector", array::rand_vec::(8.into()))
+ .into_reader_rows(RowCount::from(300), BatchCount::from(1))
+ };
+
+ let mut source_dataset = Dataset::write(make_reader(), &source_uri, None)
+ .await
+ .unwrap();
+ let mut params = crate::index::vector::VectorIndexParams::ivf_flat(
+ 4,
+ lance_linalg::distance::MetricType::L2,
+ );
+ params.covering_columns(vec!["id".to_string()]);
+ source_dataset
+ .create_index(
+ &["vector"],
+ IndexType::Vector,
+ Some("covered_idx".to_string()),
+ ¶ms,
+ false,
+ )
+ .await
+ .unwrap();
+ let source_dataset = Dataset::open(&source_uri).await.unwrap();
+
+ let mut target_dataset = Dataset::write(make_reader(), &target_uri, None)
+ .await
+ .unwrap();
+ target_dataset
+ .initialize_index(&source_dataset, "covered_idx")
+ .await
+ .unwrap();
+
+ let target_indices = target_dataset.load_indices().await.unwrap();
+ assert_eq!(target_indices.len(), 1);
+ let id_field_id = target_dataset.schema().field("id").unwrap().id;
+ assert_eq!(
+ target_indices[0].covering_fields,
+ vec![id_field_id],
+ "the copied index must keep its covering declaration"
+ );
+ target_indices[0].validate_covering_fields().unwrap();
+ }
+
#[rstest]
#[case::simple("value", "data.value")]
#[case::quoted("value.with.dot", "data.`value.with.dot`")]
@@ -8831,6 +8973,377 @@ mod tests {
);
}
+ /// A covering segment's `covering_fields` must survive `commit_existing_index_segments`.
+ /// The segment files still hold the payload, so dropping the declaration would silently
+ /// fall covered queries back to a base-table take.
+ #[tokio::test]
+ async fn test_commit_existing_index_segments_preserves_covering_fields() {
+ 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::())
+ .col(
+ "vector",
+ array::rand_vec::(8.into()),
+ )
+ .into_reader_rows(RowCount::from(20), BatchCount::from(2));
+
+ let mut dataset = Dataset::write(
+ reader,
+ test_uri,
+ Some(WriteParams {
+ max_rows_per_file: 10,
+ max_rows_per_group: 10,
+ ..Default::default()
+ }),
+ )
+ .await
+ .unwrap();
+
+ let id_field_id = dataset.schema().field("id").unwrap().id;
+ // A real covered segment: its auxiliary storage physically holds the "id"
+ // payload, which the commit-boundary cross-check verifies.
+ let mut params = crate::index::vector::VectorIndexParams::ivf_flat(
+ 1,
+ lance_linalg::distance::MetricType::L2,
+ );
+ params.covering_columns(vec!["id".to_string()]);
+ let seg = dataset
+ .create_index_builder(&["vector"], IndexType::Vector, ¶ms)
+ .name("vector_idx".to_string())
+ .execute_uncommitted()
+ .await
+ .unwrap();
+ assert_eq!(seg.covering_fields, vec![id_field_id]);
+
+ dataset
+ .commit_existing_index_segments(
+ "vector_idx",
+ "vector",
+ vec![segment_from_metadata(&seg)],
+ )
+ .await
+ .unwrap();
+
+ let committed = dataset.load_indices_by_name("vector_idx").await.unwrap();
+ assert_eq!(committed.len(), 1);
+ assert_eq!(
+ committed[0].covering_fields,
+ vec![id_field_id],
+ "committed segment must preserve its covering `covering_fields`"
+ );
+ }
+
+ /// A second commit whose incoming segment covers a different column set than an
+ /// existing, disjoint segment we would *retain* must be rejected: all segments of one
+ /// logical index must agree on their covering, or the committed `covering_fields`
+ /// would misdescribe some segment's storage.
+ #[tokio::test]
+ async fn test_commit_existing_index_segments_rejects_inconsistent_retained_covering() {
+ 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::())
+ .col(
+ "vector",
+ array::rand_vec::(8.into()),
+ )
+ .into_reader_rows(RowCount::from(20), BatchCount::from(1));
+
+ let mut dataset = Dataset::write(
+ reader,
+ test_uri,
+ Some(WriteParams {
+ max_rows_per_file: 10,
+ max_rows_per_group: 10,
+ ..Default::default()
+ }),
+ )
+ .await
+ .unwrap();
+
+ let id_field_id = dataset.schema().field("id").unwrap().id;
+
+ // First commit: a real segment covering `id`, over fragment 0 only.
+ let mut covered_params = crate::index::vector::VectorIndexParams::ivf_flat(
+ 1,
+ lance_linalg::distance::MetricType::L2,
+ );
+ covered_params.covering_columns(vec!["id".to_string()]);
+ let seg0 = dataset
+ .create_index_builder(&["vector"], IndexType::Vector, &covered_params)
+ .name("vector_idx".to_string())
+ .fragments(vec![0])
+ .execute_uncommitted()
+ .await
+ .unwrap();
+ assert_eq!(seg0.covering_fields, vec![id_field_id]);
+ dataset
+ .commit_existing_index_segments(
+ "vector_idx",
+ "vector",
+ vec![segment_from_metadata(&seg0)],
+ )
+ .await
+ .unwrap();
+
+ // Second commit: a disjoint segment (fragment 1) that covers *nothing*. It would
+ // be retained alongside seg0, whose covering is `[id]` -- an inconsistency.
+ let plain_params = crate::index::vector::VectorIndexParams::ivf_flat(
+ 1,
+ lance_linalg::distance::MetricType::L2,
+ );
+ let seg1 = dataset
+ .create_index_builder(&["vector"], IndexType::Vector, &plain_params)
+ .name("vector_idx".to_string())
+ // The name already exists (seg0 was committed); replace(true) only skips
+ // the duplicate-name check for this uncommitted build.
+ .replace(true)
+ .fragments(vec![1])
+ .execute_uncommitted()
+ .await
+ .unwrap();
+ assert!(seg1.covering_fields.is_empty());
+ let result = dataset
+ .commit_existing_index_segments(
+ "vector_idx",
+ "vector",
+ vec![segment_from_metadata(&seg1)],
+ )
+ .await;
+ let err =
+ result.expect_err("mismatched covering across retained segments must be rejected");
+ assert!(
+ err.to_string()
+ .contains("a logical index cannot mix declarations")
+ && err.to_string().contains("covering_fields"),
+ "expected a covering-consistency rejection, got: {err}"
+ );
+ }
+
+ /// When a segment covers a column whose data file was rewritten in place after the
+ /// segment was built, `prune_stale_segment_coverage` must drop the affected fragment
+ /// from the segment's coverage -- even though the *indexed* column's file is untouched.
+ /// Otherwise a covered query reads the segment's stale copy of that column.
+ #[tokio::test]
+ async fn test_commit_existing_index_segments_prunes_stale_covered_fragment() {
+ use crate::dataset::{MergeInsertBuilder, WhenMatched, WhenNotMatched};
+
+ let test_dir = tempfile::tempdir().unwrap();
+ let test_uri = test_dir.path().to_str().unwrap();
+
+ let dim = 8i32;
+ let n = 20i32;
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Int32, false),
+ Field::new("tag", DataType::Int32, true),
+ Field::new(
+ "vector",
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), dim),
+ false,
+ ),
+ ]));
+ let vectors = FixedSizeListArray::try_new_from_values(
+ Float32Array::from((0..n * dim).map(|v| v as f32).collect::>()),
+ dim,
+ )
+ .unwrap();
+ let batch = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(Int32Array::from((0..n).collect::>())),
+ Arc::new(Int32Array::from((0..n).collect::>())),
+ Arc::new(vectors),
+ ],
+ )
+ .unwrap();
+ // Two fragments (ids 0..10 -> frag 0, 10..20 -> frag 1).
+ let mut dataset = Dataset::write(
+ RecordBatchIterator::new([Ok(batch)], schema.clone()),
+ test_uri,
+ Some(WriteParams {
+ max_rows_per_file: 10,
+ max_rows_per_group: 10,
+ ..Default::default()
+ }),
+ )
+ .await
+ .unwrap();
+
+ // A scalar index on the merge key routes the partial-schema update through the
+ // in-place column-rewrite path (RewriteColumns) instead of a row-move, so the
+ // covered column's file changes while the fragment id is preserved.
+ dataset
+ .create_index(
+ &["id"],
+ IndexType::BTree,
+ None,
+ &ScalarIndexParams::default(),
+ false,
+ )
+ .await
+ .unwrap();
+
+ let tag_field_id = dataset.schema().field("tag").unwrap().id;
+
+ // Build a real covering segment BEFORE the update, so it is stamped at the
+ // pre-update version and covers `tag`, whose files still hold the old values.
+ let mut params = crate::index::vector::VectorIndexParams::ivf_flat(
+ 1,
+ lance_linalg::distance::MetricType::L2,
+ );
+ params.covering_columns(vec!["tag".to_string()]);
+ let seg = dataset
+ .create_index_builder(&["vector"], IndexType::Vector, ¶ms)
+ .name("vector_idx".to_string())
+ .execute_uncommitted()
+ .await
+ .unwrap();
+ assert_eq!(seg.covering_fields, vec![tag_field_id]);
+
+ // In-place rewrite of the covered `tag` column for fragment 0 only (no covering
+ // index committed yet, so this patches the column file in place and keeps the
+ // fragment id). The indexed `vector` column's file is untouched.
+ let sub_schema = Arc::new(Schema::new(vec![
+ Field::new("id", DataType::Int32, false),
+ Field::new("tag", DataType::Int32, true),
+ ]));
+ let update = RecordBatch::try_new(
+ sub_schema.clone(),
+ vec![
+ Arc::new(Int32Array::from((0..5).collect::>())),
+ Arc::new(Int32Array::from((100..105).collect::>())),
+ ],
+ )
+ .unwrap();
+ let (updated, _) = MergeInsertBuilder::try_new(Arc::new(dataset), vec!["id".to_string()])
+ .unwrap()
+ .when_matched(WhenMatched::UpdateAll)
+ .when_not_matched(WhenNotMatched::DoNothing)
+ .try_build()
+ .unwrap()
+ .execute_reader(Box::new(RecordBatchIterator::new([Ok(update)], sub_schema)))
+ .await
+ .unwrap();
+ let mut dataset = Arc::try_unwrap(updated).unwrap_or_else(|arc| (*arc).clone());
+
+ dataset
+ .commit_existing_index_segments(
+ "vector_idx",
+ "vector",
+ vec![segment_from_metadata(&seg)],
+ )
+ .await
+ .unwrap();
+
+ let committed = dataset.load_indices_by_name("vector_idx").await.unwrap();
+ assert_eq!(committed.len(), 1);
+ let bitmap = committed[0].fragment_bitmap.as_ref().unwrap();
+ assert!(
+ !bitmap.contains(0),
+ "fragment 0's covered column was rewritten; its stale coverage must be pruned"
+ );
+ assert!(
+ bitmap.contains(1),
+ "fragment 1's covered column was untouched; it stays covered"
+ );
+ }
+
+ /// A covered *struct* that grows a child (via `add_columns`) after a segment was built
+ /// makes the segment's stored covering payload type-incompatible for every covered row --
+ /// a global change no per-fragment prune can repair. Since the covering index is not
+ /// committed while the segment is staged, the Merge-commit guard never sees the add;
+ /// `prune_stale_segment_coverage` must reject the segment commit instead.
+ #[tokio::test]
+ async fn test_commit_existing_index_segments_rejects_grown_covered_struct() {
+ use arrow_array::StructArray;
+ use arrow_schema::Fields;
+ use lance_file::version::LanceFileVersion;
+
+ use crate::dataset::NewColumnTransform;
+
+ let test_dir = tempfile::tempdir().unwrap();
+ let test_uri = test_dir.path().to_str().unwrap();
+
+ let dim = 8i32;
+ let n = 10i32;
+ let meta_fields = Fields::from(vec![Field::new("a", DataType::Int32, false)]);
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("meta", DataType::Struct(meta_fields.clone()), true),
+ Field::new(
+ "vector",
+ DataType::FixedSizeList(Arc::new(Field::new("item", DataType::Float32, true)), dim),
+ false,
+ ),
+ ]));
+ let a = Arc::new(Int32Array::from((0..n).collect::>()));
+ let meta = Arc::new(StructArray::new(meta_fields, vec![a], None));
+ let vectors = FixedSizeListArray::try_new_from_values(
+ Float32Array::from((0..n * dim).map(|v| v as f32).collect::