diff --git a/benchmarks/deferred/README.md b/benchmarks/deferred/README.md index 0f8a089ee..234e144e9 100644 --- a/benchmarks/deferred/README.md +++ b/benchmarks/deferred/README.md @@ -24,10 +24,12 @@ GQT can benchmark them. GQT must not introduce separate maintenance commands. active instrument to guard. Ordinary engine correctness and deterministic cost tests remain at their owning layers. - `crates/omnigraph/tests/compaction_memory.rs.disabled`: the stock-Lance - compaction comparator and the engine optimize peak-allocation assertion. - The latter is deferred pending GQ optimization support; it is no longer an - executable resource regression test. Existing deterministic Blob compaction - tests remain active. + compaction comparator, the engine optimize peak-allocation assertion, the + schema-apply peak that stays flat in a table's Blob bytes, and the optimize + that erases a dropped Blob property within the compaction batch budget. + These are deferred pending GQ schema and optimization support; they are no + longer executable resource regression tests. Existing deterministic Blob + compaction, schema-evolution and erasure tests remain active. - `crates/omnigraph/tests/manifest_history_curve.rs.disabled`: four ignored acquisition instruments covering publication and schema-outcome histories, request/byte counts, retained storage and diagnostic timings. Restoring them diff --git a/benchmarks/deferred/crates/omnigraph/tests/compaction_memory.rs.disabled b/benchmarks/deferred/crates/omnigraph/tests/compaction_memory.rs.disabled index 878a1accf..b1947895c 100644 --- a/benchmarks/deferred/crates/omnigraph/tests/compaction_memory.rs.disabled +++ b/benchmarks/deferred/crates/omnigraph/tests/compaction_memory.rs.disabled @@ -1,4 +1,4 @@ -//! Peak allocation of Blob-table compaction. +//! Peak allocation of Blob-table compaction and schema apply. //! //! Lance 11 compaction materializes every managed Blob payload of one scanner //! batch, and a batch reads up to a whole fragment by default. `optimize` @@ -9,9 +9,14 @@ //! of a stock Lance compaction at the default and at the derived batch size, //! then asserts the engine's `optimize` peaks within the budget plus the two //! named allowances on fragments twice as wide as the derived batch. Every -//! value is 1 MiB, a packed placement. It is ignored by default: the -//! allocator counts the whole process, so the measurement needs the test -//! binary to itself. +//! value is 1 MiB, a packed placement. A second instrument measures schema +//! apply's peak over Blob tables of two sizes: its column changes are +//! metadata-only, so the peak must not grow with the table's Blob bytes. A +//! third measures the `optimize` that erases a dropped Blob property: the +//! rewrite reads only the remaining columns, so it peaks within the same +//! bound as an ordinary compaction although every row also stored a dropped +//! 1 MiB value. All are ignored by default: the allocator counts the whole +//! process, so a measurement needs the test binary to itself (`--exact`). mod helpers; @@ -238,3 +243,149 @@ async fn compaction_peak_allocation_on_blob_tables() { "optimize allocated {peak} bytes at its peak, above the {bound}-byte bound" ); } + +/// Schema apply on a Doc table of `rows` 1 MiB Blob values: one apply adds a +/// nullable property and renames another, a second drops it. Returns the peak +/// heap above the level at entry across both applies. +async fn schema_apply_peak(rows: usize) -> usize { + let graph = tempfile::tempdir().unwrap(); + let uri = graph.path().to_str().unwrap(); + let db = helpers::session( + Omnigraph::init( + uri, + "node Doc {\n slug: String @key\n content: Blob?\n label: String?\n}\n", + ) + .await + .unwrap(), + ); + for (load, start) in (0..rows).step_by(16).enumerate() { + let mode = if load == 0 { + LoadMode::Overwrite + } else { + LoadMode::Merge + }; + load_rows(&db, start..(start + 16).min(rows), mode).await; + } + let (applied, peak) = peak_above_baseline(async { + db.apply_schema( + "node Doc {\n slug: String @key\n content: Blob?\n \ + name: String? @rename_from(\"label\")\n note: String?\n}\n", + ) + .await?; + db.apply_schema("node Doc {\n slug: String @key\n content: Blob?\n}\n") + .await + }) + .await; + assert!(applied.unwrap().applied); + peak +} + +/// Everything schema apply allocates that is not table data: plan, catalog, +/// manifest reads and publication. An allowance, not a derived figure. +const SCHEMA_APPLY_ALLOWANCE: usize = 16 * MIB; + +#[tokio::test(flavor = "current_thread")] +#[ignore = "instrument: schema apply peak allocation is flat in a table's Blob bytes"] +async fn schema_apply_peak_allocation_is_flat_in_blob_bytes() { + let small_rows = 2 * DERIVED_ROWS; + let large_rows = 4 * DERIVED_ROWS; + let small = schema_apply_peak(small_rows).await; + let large = schema_apply_peak(large_rows).await; + for (rows, peak) in [(small_rows, small), (large_rows, large)] { + eprintln!( + "schema apply (add + rename, then drop) on {rows} rows of 1 MiB Blob: \ + peak {:.1} MiB (allowance {:.1} MiB)", + peak as f64 / MIB as f64, + SCHEMA_APPLY_ALLOWANCE as f64 / MIB as f64 + ); + } + assert!( + large <= SCHEMA_APPLY_ALLOWANCE, + "schema apply allocated {large} bytes at its peak over {large_rows} MiB of Blob values" + ); + assert!( + large <= small + 4 * MIB, + "schema apply's peak grew with the table's Blob bytes: {small} -> {large}" + ); +} + +#[tokio::test(flavor = "current_thread")] +#[ignore = "instrument: erasing a dropped Blob property compacts within the batch budget"] +async fn dropped_blob_erasure_peak_allocation_is_within_the_compaction_bound() { + let graph = tempfile::tempdir().unwrap(); + let uri = graph.path().to_str().unwrap(); + let db = helpers::session( + Omnigraph::init( + uri, + "node Doc {\n slug: String @key\n content: Blob?\n secret: Blob?\n}\n", + ) + .await + .unwrap(), + ); + // Eight loads of 16 MiB, compacted into one fragment of 64 rows, each row + // holding a kept and a soon-dropped 1 MiB value. + for load in 0..8 { + let lines = (load * 8..(load + 1) * 8) + .map(|row| { + let encode = |bytes: Vec| { + format!( + "base64:{}", + base64::engine::general_purpose::STANDARD.encode(bytes) + ) + }; + serde_json::json!({ + "type": "Doc", + "data": { + "slug": format!("d{row:03}"), + "content": encode(payload(row)), + "secret": encode(payload(row + 1)), + }, + }) + .to_string() + }) + .collect::>(); + let mode = if load == 0 { + LoadMode::Overwrite + } else { + LoadMode::Merge + }; + db.load_jsonl(&lines.join("\n"), mode).await.unwrap(); + } + db.optimize().await.unwrap(); + let doc = |stats: Vec| { + stats + .into_iter() + .find(|stat| stat.type_key == "node:Doc") + .unwrap() + }; + assert!( + !doc(db.optimize().await.unwrap()).committed, + "test precondition: nothing left to compact before the drop" + ); + let fragment_rows = helpers::open_pinned_dataset_for_test(&db, "main", "node:Doc") + .await + .get_fragments() + .iter() + .map(|fragment| fragment.metadata().physical_rows.unwrap()) + .collect::>(); + assert_eq!(fragment_rows, vec![WIDE_FRAGMENT_ROWS], "test precondition"); + db.apply_schema("node Doc {\n slug: String @key\n content: Blob?\n}\n") + .await + .unwrap(); + + let (stats, peak) = peak_above_baseline(db.optimize()).await; + let doc = doc(stats.unwrap()); + assert!(doc.committed); + assert_eq!(doc.fragments_removed, 1, "the measured optimize erases"); + let bound = BUDGET + IN_FLIGHT_VALUE_ALLOWANCE + NON_PAYLOAD_ALLOWANCE; + eprintln!( + "engine optimize erasing a dropped Blob, {WIDE_FRAGMENT_ROWS} rows of 1 MiB kept \ + + 1 MiB dropped: peak {:.1} MiB (bound {:.1} MiB)", + peak as f64 / MIB as f64, + bound as f64 / MIB as f64 + ); + assert!( + peak <= bound, + "the erasing optimize allocated {peak} bytes at its peak, above the {bound}-byte bound" + ); +} diff --git a/changelog.d/dropped-property-storage.changed.md b/changelog.d/dropped-property-storage.changed.md new file mode 100644 index 000000000..b7c954a84 --- /dev/null +++ b/changelog.d/dropped-property-storage.changed.md @@ -0,0 +1,12 @@ +- Dropping a property no longer rewrites its table. Through v0.13.0 schema + apply rewrote the table without the dropped values, so `omnigraph cleanup` alone + erased them; now they stay in the table's data files until + `omnigraph optimize` rewrites every fragment that holds them, whatever its + size. To erase them, run optimize, delete every branch created from a commit + before it and every tag naming such a commit (merging a branch does not + release its files), then run `cleanup` with a retention that excludes the + commits before the optimize, for example `--keep 1`. Dropped types are still + reclaimed by `cleanup` alone. See + [dropping declarations][dropped-property-storage-drops]. + +[dropped-property-storage-drops]: ../docs/user/schema/index.md#schema-changes diff --git a/changelog.d/schema-apply-metadata-only.performance.md b/changelog.d/schema-apply-metadata-only.performance.md new file mode 100644 index 000000000..ae2716fc9 --- /dev/null +++ b/changelog.d/schema-apply-metadata-only.performance.md @@ -0,0 +1,12 @@ +- Adding a nullable property, renaming a property and dropping a property no + longer rewrite the table. Schema apply changes only the table's metadata, so + its memory and I/O no longer grow with the rows and Blob values the table + stores, only with its metadata (fragments, columns and indexes): a schema + change on a large Blob table, which previously held every managed Blob value + of the table in memory and could exhaust a server running the deployment, + now reads no row and no Blob value. Existing indexes keep covering their rows + instead of waiting for the next `optimize`, and a stored external Blob + reference to a byte range, which schema apply previously refused, is kept as + stored. See [schema changes][schema-apply-metadata-only-changes]. + +[schema-apply-metadata-only-changes]: ../docs/user/schema/index.md#schema-changes diff --git a/crates/omnigraph-compiler/src/catalog/schema_plan.rs b/crates/omnigraph-compiler/src/catalog/schema_plan.rs index c47827eb6..e59f3f2b6 100644 --- a/crates/omnigraph-compiler/src/catalog/schema_plan.rs +++ b/crates/omnigraph-compiler/src/catalog/schema_plan.rs @@ -97,9 +97,11 @@ pub enum SchemaMigrationStep { type_kind: SchemaTypeKind, name: String, }, - /// Remove a property from an existing type: the table is rewritten - /// without the column. Older table versions keep it, and older graph - /// commits read it until `omnigraph cleanup` stops retaining them. + /// Remove a property from an existing type: a metadata-only commit takes + /// the column out of the table's schema and rewrites no row. Its values + /// stay in the existing data files until a compaction rewrites them, and + /// older graph commits read it until `omnigraph cleanup` stops retaining + /// them. DropProperty { type_kind: SchemaTypeKind, type_name: String, @@ -716,11 +718,9 @@ fn plan_properties( .iter() .filter(|property| !consumed.contains(&property.property_id)) { - // Property removed from the desired schema. Apply reuses the - // stage_overwrite rewrite path — batch_for_schema_apply_rewrite - // iterates target_schema.fields(), so the dropped column is - // naturally projected away. Retention, which the OG-DS-104 - // destructive tier expects, is stated on + // Property removed from the desired schema. Apply drops the column + // from the table's schema by a metadata-only commit. Retention, which + // the OG-DS-104 destructive tier expects, is stated on // `SchemaMigrationStep::DropProperty`. steps.push(SchemaMigrationStep::DropProperty { type_kind, @@ -1443,8 +1443,8 @@ node Account @rename_from("User") { // Removing a property from the desired schema emits // DropProperty (schema-lint v1 chassis commit #3, // MR-694). The plan is `supported = true` — the apply path - // handles the drop via the existing stage_overwrite rewrite - // projection. Verified at the integration level by + // drops the column by a metadata-only commit. Verified at the + // integration level by // `apply_schema_drops_a_nullable_property_and_preserves_prior_version` // in `crates/omnigraph/tests/schema_apply.rs`. let accepted = ir(r#" diff --git a/crates/omnigraph-core/src/instrumentation.rs b/crates/omnigraph-core/src/instrumentation.rs index e296d1e1f..9e2952741 100644 --- a/crates/omnigraph-core/src/instrumentation.rs +++ b/crates/omnigraph-core/src/instrumentation.rs @@ -1237,7 +1237,7 @@ pub struct MergeWriteProbes { /// while its assignments resolve must leave this at zero. pub mutation_table_open_calls: Arc, /// Blob payload values a rewrite consumed while rebuilding descriptor rows - /// into a logical source (a keyed write or a schema rewrite), or that + /// into a logical source (a keyed write or a branch merge), or that /// export, a change image or an entity read rendered: one per managed /// value, counted after the batched managed read returned it and its /// length matched, and one per external object read (rewrites only). diff --git a/crates/omnigraph/src/blob.rs b/crates/omnigraph/src/blob.rs index 45b481884..95c581480 100644 --- a/crates/omnigraph/src/blob.rs +++ b/crates/omnigraph/src/blob.rs @@ -1169,7 +1169,7 @@ impl<'a> BlobDescriptorDecoder<'a> { /// resolves managed (inline/packed/dedicated) bytes from the row's owning /// data file, then `position`/`size`/`blob_id` within it. So the identity /// must be qualified by *which* data file. It uses the file's stable path - /// (`data_file_path`) — a per-file v4 UUID Lance mints once and never + /// (`managed_data_file`) — a per-file v4 UUID Lance mints once and never /// rewrites; compaction, `Overwrite`, and per-branch writes all produce a /// NEW UUID. Unlike the numeric fragment id, this is globally unique: it /// does not restart at 0 on `Overwrite` and is not branch-local, so equal @@ -1178,7 +1178,16 @@ impl<'a> BlobDescriptorDecoder<'a> { /// placement, so their identity is source-independent. Null uses the /// classified state, never sentinel child values, so the two physical null /// encodings share one identity. - pub(crate) fn physical_identity(&self, row: usize, data_file_path: &str) -> Result { + /// + /// `managed_data_file` is called only for a managed descriptor. A null or + /// external identity names no file, and a fragment written before its Blob + /// column was added has no data file for that column at all: Lance reads + /// the column there as null. + pub(crate) fn physical_identity<'path>( + &self, + row: usize, + managed_data_file: impl FnOnce() -> Result<&'path str>, + ) -> Result { match self.classify(row)? { BlobDescriptor::Null => Ok("null".to_string()), BlobDescriptor::External { @@ -1190,7 +1199,8 @@ impl<'a> BlobDescriptorDecoder<'a> { length.map(|len| len.to_string()).unwrap_or_default(), )), BlobDescriptor::Managed { .. } => Ok(format!( - "mgd:{data_file_path}:{}:{}:{}:{}", + "mgd:{}:{}:{}:{}:{}", + managed_data_file()?, self.kinds.value(row), self.positions.value(row), self.sizes.value(row), @@ -2406,6 +2416,55 @@ mod tests { ); } + /// Only a managed identity names its owning data file. A null or external + /// descriptor never asks for one, since a fragment written before its Blob + /// column was added has none; a managed descriptor whose file cannot be + /// resolved keeps the resolver's integrity error. + #[test] + fn physical_identity_resolves_a_data_file_only_for_managed_descriptors() { + let no_file = + || -> Result<&'static str> { panic!("a null or external identity names no data file") }; + let null = StructArray::new_null(fields(), 1); + assert_eq!( + BlobDescriptorDecoder::try_new(&null) + .unwrap() + .physical_identity(0, no_file) + .unwrap(), + "null" + ); + let external = descriptor( + Some(3), + Some(4), + Some(8), + Some(0), + Some("s3://bucket/object"), + ); + assert_eq!( + BlobDescriptorDecoder::try_new(&external) + .unwrap() + .physical_identity(0, no_file) + .unwrap(), + "ext:4:8:s3://bucket/object" + ); + + let managed = descriptor(Some(1), Some(16), Some(8), Some(2), Some("")); + let decoder = BlobDescriptorDecoder::try_new(&managed).unwrap(); + assert_eq!( + decoder.physical_identity(0, || Ok("a.lance")).unwrap(), + "mgd:a.lance:1:16:8:2" + ); + assert_blob_integrity( + decoder + .physical_identity(0, || { + Err(OmniError::blob_integrity( + "fragment 0 has no data file for blob field 3", + )) + }) + .unwrap_err(), + "has no data file", + ); + } + #[test] fn exact_v2_shape_unknown_kind_bounds_and_arithmetic_fail_closed() { for index in 0..5 { diff --git a/crates/omnigraph/src/changes/row_compare.rs b/crates/omnigraph/src/changes/row_compare.rs index ce1bbc9bc..e43316528 100644 --- a/crates/omnigraph/src/changes/row_compare.rs +++ b/crates/omnigraph/src/changes/row_compare.rs @@ -288,15 +288,19 @@ impl BatchCursor { .downcast_ref::() .expect("Blob descriptor shape was validated when the batch arrived"); let decoder = BlobDescriptorDecoder::try_new(descriptions)?; - let fragment_id = row_addresses - .expect("Blob batches carry row addresses") - .value(row) - >> 32; - let data_file_path = - data_file_path_for_field(dataset.fragments(), fragment_id, blob_column.field_id)?; + // Only a managed descriptor names a data file. A fragment written + // before this Blob column was added has none for it, and its + // descriptor is the null Lance reads there. + let identity = decoder.physical_identity(row, || { + let fragment_id = row_addresses + .expect("Blob batches carry row addresses") + .value(row) + >> 32; + data_file_path_for_field(dataset.fragments(), fragment_id, blob_column.field_id) + })?; blob_signatures.push(BlobColumnSig { name: blob_column.name.clone(), - identity: decoder.physical_identity(row, data_file_path)?, + identity, managed: matches!(decoder.classify(row)?, BlobDescriptor::Managed { .. }), }); } @@ -429,7 +433,9 @@ impl OrderedRows { /// Resolve the immutable data-file path that holds `field_id` in the fragment /// owning a row — the stable UUID qualifier for a managed-Blob identity. Reads -/// only the in-memory manifest. +/// only the in-memory manifest. Called for managed descriptors only, so a +/// fragment without a data file for the field is an integrity failure: the +/// descriptor claims bytes nothing stores. fn data_file_path_for_field( fragments: &[lance_table::format::Fragment], fragment_id: u64, diff --git a/crates/omnigraph/src/db/omnigraph.rs b/crates/omnigraph/src/db/omnigraph.rs index 5cd330a74..99ee43f92 100644 --- a/crates/omnigraph/src/db/omnigraph.rs +++ b/crates/omnigraph/src/db/omnigraph.rs @@ -3,10 +3,10 @@ use std::io::Write; use std::sync::Arc; use arc_swap::ArcSwap; -use arrow_array::{Array, RecordBatch, StringArray, StructArray, UInt64Array, new_null_array}; +use arrow_array::{Array, RecordBatch, StringArray, StructArray, UInt64Array}; use arrow_schema::{DataType, Field, Schema}; use lance::Dataset; -use lance::blob::{BlobArrayBuilder, blob_field}; +use lance::blob::blob_field; use lance::datatypes::{LANCE_UNENFORCED_PRIMARY_KEY, LANCE_UNENFORCED_PRIMARY_KEY_POSITION}; use omnigraph_compiler::catalog::{Catalog, EdgeType, NodeType}; use omnigraph_compiler::schema::parser::parse_schema; @@ -3295,17 +3295,6 @@ async fn warm_resolved_target( }) } -fn concat_or_empty_batches(schema: Arc, batches: Vec) -> Result { - if batches.is_empty() { - return Ok(RecordBatch::new_empty(schema)); - } - if batches.len() == 1 { - return Ok(batches.into_iter().next().unwrap()); - } - let batch_schema = batches[0].schema(); - arrow_select::concat::concat_batches(&batch_schema, &batches).map_err(OmniError::arrow_internal) -} - fn blob_properties_for_table_key<'a>( catalog: &'a Catalog, table_key: &str, @@ -3729,7 +3718,7 @@ pub(crate) fn schema_for_table_key(catalog: &Catalog, table_key: &str) -> Result #[cfg(test)] mod tests { - use arrow_array::Int32Array; + use arrow_array::{Int32Array, new_null_array}; use super::*; use crate::db::manifest::ManifestCoordinator; diff --git a/crates/omnigraph/src/db/omnigraph/optimize.rs b/crates/omnigraph/src/db/omnigraph/optimize.rs index 44cbd5a43..6387580a5 100644 --- a/crates/omnigraph/src/db/omnigraph/optimize.rs +++ b/crates/omnigraph/src/db/omnigraph/optimize.rs @@ -472,10 +472,12 @@ async fn prepare_optimize_table( .open_pinned_for_write(&task.full_path, &task.entry) .await?; + // The same plan `stage_compaction` executes: a fragment still holding a + // dropped property's values is compaction work even when Lance's planner + // would leave the table alone. let options = CompactionOptions::default(); - let will_compact = plan_compaction(snapshot.dataset(), &options) - .await - .map_err(OmniError::storage)? + let will_compact = TableStore::plan_table_compaction(snapshot.dataset(), &options) + .await? .num_tasks() > 0; let needs_reindex = TableStore::has_foldable_unindexed_fragments(snapshot.dataset()).await?; diff --git a/crates/omnigraph/src/db/omnigraph/schema_apply.rs b/crates/omnigraph/src/db/omnigraph/schema_apply.rs index cabdf0b43..d32866c00 100644 --- a/crates/omnigraph/src/db/omnigraph/schema_apply.rs +++ b/crates/omnigraph/src/db/omnigraph/schema_apply.rs @@ -1,10 +1,5 @@ use super::*; -use crate::blob::{BlobDescriptor, BlobDescriptorDecoder, ExternalBlobRef}; use crate::seams::{decide_seam, fail}; -use futures::TryStreamExt; - -const SCHEMA_BLOB_DESCRIPTOR_SCAN_ROWS: usize = 1024; -const SCHEMA_BLOB_DESCRIPTOR_SCAN_BYTES: u64 = 4 * 1024 * 1024; mod prepared; mod settlement; @@ -139,8 +134,9 @@ decide_seam! { } decide_seam! { - /// After each SchemaApply table effect commits (a detached rewrite or a - /// new-table create), before the next table effect or the publication. + /// After each SchemaApply table effect commits (a detached schema-evolution + /// commit, one of at most two per table, or a new-table create), before + /// the next table effect or the publication. pub static SCHEMA_APPLY_POST_TABLE_COMMIT = ("schema_apply.post_table_commit", Unreachable, [Fail]); } @@ -329,7 +325,7 @@ where // Resolve every rename before classifying dependent property steps. The // planner currently emits RenameType first, but correctness must not depend // on step ordering: a same-apply rename + property drop still routes the - // rewrite to the source table captured under its old alias. + // schema evolution to the source table captured under its old alias. let renamed_tables = plan .steps .iter() @@ -345,7 +341,10 @@ where _ => None, }) .collect::>(); - let mut rewritten_tables = BTreeSet::new(); + // Existing tables whose columns change: added, renamed or dropped + // properties. Each evolves by metadata-only Lance commits that keep every + // data file; no row is read or rewritten. + let mut evolved_tables = BTreeSet::new(); let mut dropped_tables = BTreeSet::new(); let mut property_renames = HashMap::>::new(); let mut changed_edge_tables = false; @@ -389,7 +388,7 @@ where if table_key.starts_with("edge:") { changed_edge_tables = true; } - rewritten_tables.insert(table_key); + evolved_tables.insert(table_key); } SchemaMigrationStep::RenameProperty { type_kind, @@ -404,7 +403,7 @@ where if table_key.starts_with("edge:") { changed_edge_tables = true; } - rewritten_tables.insert(table_key.clone()); + evolved_tables.insert(table_key.clone()); property_renames .entry(table_key) .or_default() @@ -432,17 +431,16 @@ where if matches!(type_kind, SchemaTypeKind::Interface) { continue; } - // A property drop routes through the existing - // stage_overwrite rewrite path. batch_for_schema_apply_rewrite - // iterates the *target* schema fields, so a property - // absent from desired_catalog is naturally projected - // away in the rebuilt batch. Nothing is reclaimed after the - // publish; see `SchemaMigrationStep::DropProperty`. + // A property drop removes the column from the table's + // schema by a metadata-only commit: its values stay in the + // existing data files, which older versions still read. + // Nothing is reclaimed at apply; see + // `SchemaMigrationStep::DropProperty`. let table_key = schema_table_key(*type_kind, type_name); if table_key.starts_with("edge:") { changed_edge_tables = true; } - rewritten_tables.insert(table_key); + evolved_tables.insert(table_key); } SchemaMigrationStep::DropType { type_kind, name } => { if matches!(type_kind, SchemaTypeKind::Interface) { @@ -474,7 +472,7 @@ where let mut table_tombstones = BTreeMap::)>::new(); - for table_key in &rewritten_tables { + for table_key in &evolved_tables { if added_tables.contains(table_key) { continue; } @@ -495,7 +493,7 @@ where let accepted_identity = table_identity_for_schema_key(&accepted_ir, source_table_key)?; if identity != accepted_identity || identity != entry.identity { return Err(OmniError::manifest_internal(format!( - "schema apply rewrite identity mismatch: source '{}' is {}, target '{}' is {}", + "schema apply evolution identity mismatch: source '{}' is {}, target '{}' is {}", source_table_key, entry.identity, table_key, identity ))); } @@ -629,30 +627,40 @@ where existing_heads.insert(entry.type_key.clone(), head); } - // Lance's logical Blob rewrite input cannot represent an existing - // external offset/length range. Discover that unsupported persisted state - // across the complete rewrite set before any added / lexically earlier - // table can move. The builder repeats this check as a defensive invariant. - for table_key in &rewritten_tables { + // Plan every table's schema evolution from its manifest before any table + // moves: a column whose type or nullability would change, a required + // added column or a mismatched property identity refuses here, with the + // graph unchanged. The effects stage exactly these plans. + let mut evolutions = BTreeMap::::new(); + for table_key in &evolved_tables { if added_tables.contains(table_key) { continue; } let source_table_key = renamed_tables.get(table_key).unwrap_or(table_key); let source_ds = existing_heads.get(source_table_key).ok_or_else(|| { OmniError::manifest_internal(format!( - "missing preflighted source table '{}' for schema Blob range validation", + "missing preflighted source table '{}' for schema evolution", source_table_key )) })?; - validate_schema_rewrite_external_ranges( - source_ds, - source_table_key, - accepted_catalog.as_ref(), - table_key, - &desired_catalog, - property_renames.get(table_key), - ) - .await?; + let target = schema_for_table_key(&desired_catalog, table_key)?; + let mut renames = property_renames + .get(table_key) + .map(|renames| { + renames + .iter() + .map(|(to, from)| (from.clone(), to.clone())) + .collect::>() + }) + .unwrap_or_default(); + renames.sort(); + let evolution = TableStore::plan_schema_evolution(source_ds.dataset(), &target, &renames) + .map_err(|error| { + OmniError::manifest(format!( + "schema apply cannot evolve table '{source_table_key}' in place: {error}" + )) + })?; + evolutions.insert(table_key.clone(), evolution); } let mut published_commit: Option = None; @@ -744,10 +752,8 @@ where fail(&SCHEMA_APPLY_POST_TABLE_COMMIT)?; } - for table_key in &rewritten_tables { - if added_tables.contains(table_key) { - continue; - } + for (table_key, mut evolution) in evolutions { + let table_key = &table_key; let source_table_key = renamed_tables.get(table_key).unwrap_or(table_key); let entry = snapshot.dataset(source_table_key).ok_or_else(|| { OmniError::manifest(format!( @@ -755,45 +761,49 @@ where source_table_key, table_key )) })?; - let source_ds = existing_heads.remove(source_table_key).ok_or_else(|| { + // Reuse the handle that was opened and pin-checked before the + // effects; reopening here would introduce a second HEAD observation. + let mut tip = existing_heads.remove(source_table_key).ok_or_else(|| { OmniError::manifest_internal(format!( "missing preflighted source table '{}' for schema apply", source_table_key )) })?; - let batch = batch_for_schema_apply_rewrite( - db, - &source_ds, - source_table_key, - accepted_catalog.as_ref(), - table_key, - &desired_catalog, - property_renames.get(table_key), - ) - .await?; - let dataset_uri = db.storage().dataset_uri(&entry.dataset_path); - // Reuse the handle that was opened and pin-checked before the - // effects; reopening here would introduce a second HEAD observation. - let staged = db.storage().stage_overwrite(&source_ds, batch).await?; let identity = table_identity_for_schema_key(&desired_ir, table_key)?; if identity != entry.identity { return Err(OmniError::manifest_internal(format!( - "SchemaApply rewrite '{}' changed table identity {} to {}", + "SchemaApply evolution '{}' changed table identity {} to {}", table_key, entry.identity, identity ))); } + let dataset_uri = db.storage().dataset_uri(&entry.dataset_path); let witness = crate::table_store::StagingWitness::new( &base_branch_identifier, base_graph_head.as_deref(), )?; - let (detached, transaction) = db + // A Project for renames and drops, then a Merge for additions, + // each a detached commit of the previous; the pin names the tip. + // Neither writes a data file, so every surviving index keeps its + // coverage and no row or Blob payload is read. + let mut tip_transaction = None; + while let Some(staged) = db .storage() - .commit_staged_detached(source_ds, staged, &witness) - .await?; - // The rewrite drops the table's existing index coverage; it is - // restored off the critical path by optimize's optimize_indices / - // ensure_indices (iss-848). Reads scan uncovered fragments meanwhile. - let state = db.storage().table_state(&dataset_uri, &detached).await?; + .stage_schema_evolution(&tip, &mut evolution) + .await? + { + let (next, transaction) = db + .storage() + .commit_staged_detached(tip, staged, &witness) + .await?; + tip = next; + tip_transaction = Some(transaction); + fail(&SCHEMA_APPLY_POST_TABLE_COMMIT)?; + } + let Some(transaction) = tip_transaction else { + // The table already has the target schema: nothing to publish. + continue; + }; + let state = db.storage().table_state(&dataset_uri, &tip).await?; let published_dataset_version = entry.published_dataset_version + 1; let version_metadata = state .version_metadata @@ -811,7 +821,6 @@ where version_metadata, }, ); - fail(&SCHEMA_APPLY_POST_TABLE_COMMIT)?; } // Index-only changes (AddConstraint, i.e. adding an `@index`) are pure @@ -971,261 +980,3 @@ where contract: prepared.desired_contract().clone(), }) } - -pub(super) async fn batch_for_schema_apply_rewrite( - db: &Omnigraph, - source_ds: &SnapshotHandle, - source_table_key: &str, - source_catalog: &Catalog, - target_table_key: &str, - target_catalog: &Catalog, - property_renames: Option<&HashMap>, -) -> Result { - let target_schema = schema_for_table_key(target_catalog, target_table_key)?; - let source_blob_properties = blob_properties_for_table_key(source_catalog, source_table_key)?; - let target_blob_properties = blob_properties_for_table_key(target_catalog, target_table_key)?; - let needs_row_ids = !source_blob_properties.is_empty() || !target_blob_properties.is_empty(); - let batches = if needs_row_ids { - db.storage() - .scan_with_row_id(source_ds, None, None, None, true) - .await? - } else { - db.storage().scan_batches(source_ds).await? - }; - if batches.is_empty() { - return Ok(RecordBatch::new_empty(target_schema)); - } - let source_schema = batches[0].schema(); - let batch = concat_or_empty_batches(source_schema, batches)?; - - let row_ids = if needs_row_ids { - Some( - batch - .column_by_name("_rowid") - .and_then(|col| col.as_any().downcast_ref::()) - .ok_or_else(|| { - OmniError::manifest_internal(format!( - "expected _rowid column when rewriting '{}'", - source_table_key - )) - })? - .values() - .iter() - .copied() - .collect::>(), - ) - } else { - None - }; - - let mut columns = Vec::with_capacity(target_schema.fields().len()); - for field in target_schema.fields() { - let source_name = property_renames - .and_then(|renames| renames.get(field.name())) - .map(String::as_str) - .unwrap_or_else(|| field.name().as_str()); - if let Some(column) = batch.column_by_name(source_name) { - if target_blob_properties.contains(field.name()) - && source_blob_properties.contains(source_name) - { - let descriptions = - column - .as_any() - .downcast_ref::() - .ok_or_else(|| { - OmniError::blob_integrity(format!( - "expected blob descriptions for '{}.{}'", - source_table_key, source_name - )) - })?; - let rebuilt = rebuild_blob_column( - db, - source_ds, - source_name, - descriptions, - row_ids.as_deref().unwrap_or(&[]), - ) - .await?; - columns.push(rebuilt); - } else { - columns.push(column.clone()); - } - } else { - columns.push(new_null_array(field.data_type(), batch.num_rows())); - } - } - - RecordBatch::try_new(target_schema, columns).map_err(OmniError::arrow_internal) -} - -/// Descriptor-only pre-effect validation for external Blob cells that a schema -/// rewrite will carry. Project only the source Blob columns that survive in -/// the target schema; this performs no external-object lookup or payload read. -async fn validate_schema_rewrite_external_ranges( - source_ds: &SnapshotHandle, - source_table_key: &str, - source_catalog: &Catalog, - target_table_key: &str, - target_catalog: &Catalog, - property_renames: Option<&HashMap>, -) -> Result<()> { - let source_blob_properties = blob_properties_for_table_key(source_catalog, source_table_key)?; - let target_blob_properties = blob_properties_for_table_key(target_catalog, target_table_key)?; - let mut source_columns = target_blob_properties - .iter() - .filter_map(|target_name| { - let source_name = property_renames - .and_then(|renames| renames.get(target_name)) - .unwrap_or(target_name); - source_blob_properties - .contains(source_name) - .then(|| source_name.clone()) - }) - .collect::>(); - source_columns.sort(); - source_columns.dedup(); - if source_columns.is_empty() { - return Ok(()); - } - - let projection = source_columns - .iter() - .map(String::as_str) - .collect::>(); - let mut batches = crate::table_store::TableStore::scan_stream_bounded( - source_ds.dataset(), - Some(&projection), - None, - None, - false, - SCHEMA_BLOB_DESCRIPTOR_SCAN_ROWS, - SCHEMA_BLOB_DESCRIPTOR_SCAN_BYTES, - ) - .await?; - while let Some(batch) = batches.try_next().await.map_err(OmniError::storage)? { - for source_name in &source_columns { - let descriptions = batch - .column_by_name(source_name) - .and_then(|column| column.as_any().downcast_ref::()) - .ok_or_else(|| { - OmniError::blob_integrity(format!( - "expected blob descriptions for '{}.{}' during pre-arm schema validation", - source_table_key, source_name - )) - })?; - let decoder = BlobDescriptorDecoder::try_new(descriptions)?; - for row in 0..descriptions.len() { - if let BlobDescriptor::External { - uri, - offset, - length, - } = decoder.classify(row)? - { - whole_external_uri_for_schema_rewrite(uri, offset, length)?; - } - } - } - } - Ok(()) -} - -async fn rebuild_blob_column( - _db: &Omnigraph, - source_ds: &SnapshotHandle, - column_name: &str, - descriptions: &StructArray, - row_ids: &[u64], -) -> Result> { - let decoder = BlobDescriptorDecoder::try_new(descriptions)?; - let mut builder = BlobArrayBuilder::new(row_ids.len()); - let mut managed_row_ids = Vec::new(); - let mut row_descriptors = Vec::with_capacity(row_ids.len()); - - for (row, row_id) in row_ids.iter().enumerate() { - let descriptor = decoder.classify(row)?; - if matches!(descriptor, BlobDescriptor::Managed { .. }) { - managed_row_ids.push(*row_id); - } - row_descriptors.push(descriptor); - } - - let mut managed_blobs = - TableStore::managed_blob_payloads(source_ds.dataset(), column_name, managed_row_ids) - .await?; - - for descriptor in row_descriptors { - match descriptor { - BlobDescriptor::Null => builder.push_null().map_err(OmniError::lance_internal)?, - BlobDescriptor::External { - uri, - offset, - length, - } => { - let uri = whole_external_uri_for_schema_rewrite(uri, offset, length)?; - builder.push_uri(uri).map_err(OmniError::lance_internal)?; - } - BlobDescriptor::Managed { length } => { - builder - .push_bytes(managed_blobs.next(length).await?) - .map_err(OmniError::lance_internal)?; - } - } - } - - managed_blobs.finish().await?; - - builder.finish().map_err(OmniError::lance_internal) -} - -/// Lance's logical Blob input can retain a whole-object URI but cannot encode -/// a descriptor range. Refuse a valid ranged descriptor before schema staging -/// instead of silently widening it to the entire target object. -fn whole_external_uri_for_schema_rewrite( - uri: String, - offset: u64, - length: Option, -) -> Result { - let reference = ExternalBlobRef { - uri, - offset, - length, - }; - if let Err(ranged) = reference.whole_object_uri() { - return Err(OmniError::manifest(format!( - "schema rewrite cannot preserve {ranged}" - ))); - } - Ok(reference.uri) -} - -#[cfg(test)] -mod blob_rewrite_tests { - use super::whole_external_uri_for_schema_rewrite; - use crate::error::OmniError; - - #[test] - fn schema_rewrite_never_widens_an_external_blob_range() { - assert_eq!( - whole_external_uri_for_schema_rewrite("s3://bucket/base/object".to_string(), 0, None,) - .unwrap(), - "s3://bucket/base/object" - ); - for (offset, length) in [(1, None), (0, Some(1)), (7, Some(0))] { - let error = whole_external_uri_for_schema_rewrite( - "s3://user:secret@bucket/base/object?signature=private".to_string(), - offset, - length, - ) - .unwrap_err(); - assert!(matches!(error, OmniError::Manifest(_))); - assert!( - error - .to_string() - .contains("cannot preserve ranged external Blob descriptor") - ); - assert!(!error.to_string().contains("secret")); - assert!(!error.to_string().contains("signature")); - assert!(!error.to_string().contains("private")); - } - } -} diff --git a/crates/omnigraph/src/storage_layer.rs b/crates/omnigraph/src/storage_layer.rs index 687f458e8..dc9dcf623 100644 --- a/crates/omnigraph/src/storage_layer.rs +++ b/crates/omnigraph/src/storage_layer.rs @@ -780,6 +780,15 @@ pub trait TableStorage: sealed::Sealed + Send + Sync + Debug { renames: &[(String, String)], ) -> Result; + /// Stage the next metadata-only commit of a planned schema evolution, or + /// `None` when no step remains; see `TableStore::stage_schema_evolution`. + /// Committed through `commit_staged_detached`. + async fn stage_schema_evolution( + &self, + snapshot: &SnapshotHandle, + evolution: &mut crate::table_store::SchemaEvolution, + ) -> Result>; + /// Stage a delete (two-phase, no HEAD advance) of the rows `filter`, a typed /// DataFusion expression, selects; `None` when 0 rows match and the table is /// not touched (no transaction, no version). See `TableStore::stage_delete`. @@ -1310,6 +1319,18 @@ impl TableStorage for TableStore { .map(StagedHandle::new) } + async fn stage_schema_evolution( + &self, + snapshot: &SnapshotHandle, + evolution: &mut crate::table_store::SchemaEvolution, + ) -> Result> { + Ok( + TableStore::stage_schema_evolution(self, snapshot.dataset(), evolution) + .await? + .map(StagedHandle::new), + ) + } + async fn stage_delete( &self, snapshot: &SnapshotHandle, diff --git a/crates/omnigraph/src/table_store.rs b/crates/omnigraph/src/table_store.rs index 60cbe6fd9..ff5c05dc5 100644 --- a/crates/omnigraph/src/table_store.rs +++ b/crates/omnigraph/src/table_store.rs @@ -23,7 +23,9 @@ use datafusion::prelude::Expr; use futures::{StreamExt, TryStreamExt, future::BoxFuture}; use lance::Dataset; use lance::blob::BlobArrayBuilder; -use lance::dataset::optimize::{CompactionMetrics, CompactionOptions, plan_compaction}; +use lance::dataset::optimize::{ + CompactionMetrics, CompactionOptions, CompactionPlan, TaskData, plan_compaction, +}; use lance::dataset::scanner::{ColumnOrdering, DatasetRecordBatchStream, Scanner}; use lance::dataset::transaction::{ Operation, RewriteGroup, Transaction, TransactionBuilder, UpdateMode, @@ -703,6 +705,48 @@ pub struct StagedCompaction { pub metrics: CompactionMetrics, } +/// Whether `fragment` still physically stores a column the current schema no +/// longer has: a field id that one of its data or overlay files lists and +/// `live` (the schema's field ids) lacks. A metadata-only drop keeps the +/// dropped id in every file that also holds a surviving column; a file +/// holding no surviving column leaves the fragment with the drop. Lance's +/// overlay tombstone is negative and names no column, and a Blob-v2 column is +/// listed by its own id, never by its descriptor children's, so a table +/// without a dropped column never matches. Manifest metadata only. +fn fragment_holds_dropped_column(fragment: &Fragment, live: &HashSet) -> bool { + fragment + .files + .iter() + .chain(fragment.overlays.iter().map(|overlay| &overlay.data_file)) + .any(|file| file.fields.iter().any(|id| *id >= 0 && !live.contains(id))) +} + +/// The metadata-only commits of one table's schema evolution, planned once by +/// [`TableStore::plan_schema_evolution`] from the dataset's manifest alone and +/// consumed step by step by [`TableStore::stage_schema_evolution`]. +#[derive(Debug, Clone)] +pub struct SchemaEvolution { + /// Renames and drops: the surviving columns in target order, committed as + /// `Operation::Project`. `None` when no column is renamed, dropped or + /// otherwise changed. + project: Option, + /// Additions: the complete target schema, committed as `Operation::Merge` + /// over the unchanged fragments. `None` when no column is added. + merge: Option, + /// The `(field id, name)` columns, in order, of the dataset the next step + /// must be staged against: the planned base, then each step's result. + expected: Vec<(i32, String)>, +} + +/// A schema's columns as `(field id, name)` in pre-order: the identity a +/// staged evolution step checks its dataset against. +fn schema_columns(schema: &LanceSchema) -> Vec<(i32, String)> { + schema + .fields_pre_order() + .map(|field| (field.id, field.name.clone())) + .collect() +} + #[derive(Debug, Clone)] pub struct StagedWrite { transaction: Transaction, @@ -3867,22 +3911,82 @@ impl TableStore { Ok((dataset, committed_identity)) } - /// RFC 0067: plan and execute Lance compaction against a pinned base and - /// stage the result as one `Rewrite` transaction. The new fragments take - /// ids above the base's high-water mark, so the commit needs no - /// `ReserveFragments` (whose replay would not conflict with its twin). A - /// stable-row-id rewrite carries every index's coverage over to the new - /// fragments when Lance applies it. `None` when the plan has no task. - /// Every task of a Blob table is sized before any executes, so a sizing - /// refusal leaves no rewritten file behind. + /// Plan a graph table's compaction: Lance's plan for `options`, plus one + /// task of its own for every other fragment that still physically holds + /// a dropped column ([`fragment_holds_dropped_column`]). + /// + /// This is how a dropped property's values are erased. Schema apply drops + /// a property with a metadata-only `Operation::Project`, which keeps every + /// data file that also holds a surviving column, the dropped values + /// included, and Lance's planner selects fragments by size, deletions and + /// overlays, never by dropped columns. Planning those fragments here makes + /// every `optimize` rewrite them: the rewrite scans the current schema + /// only, so the new data files and Blob sidecars hold no dropped value, + /// and a later `cleanup` that no longer retains the versions before the + /// rewrite deletes the old files. A single-fragment task is the shape + /// Lance itself plans for a fragment that compacts alone (deletion + /// materialization), and its `Rewrite` group replaces the fragment in + /// place. A fragment a Lance task already rewrites, or one + /// `options.excluded_fragment_ids` names, gets no task of its own. + /// Lance applies its per-run source budgets (`max_source_fragments`, + /// `max_source_rows`, `max_source_bytes`) to its own tasks only, so + /// options that set one are refused rather than let the added tasks run + /// outside it. The selection reads only the manifest, so a table holding + /// no dropped column costs nothing beyond Lance's own planning. + pub(crate) async fn plan_table_compaction( + ds: &Dataset, + options: &CompactionOptions, + ) -> Result { + if options.max_source_fragments.is_some() + || options.max_source_rows.is_some() + || options.max_source_bytes.is_some() + { + return Err(OmniError::manifest_internal(format!( + "compaction of {} sets a per-run source budget, which Lance applies only to \ + its own tasks; the dropped-column tasks would run outside it", + ds.uri() + ))); + } + let mut plan = plan_compaction(ds, options) + .await + .map_err(OmniError::storage)?; + let live = ds.schema().field_ids().into_iter().collect::>(); + let mut skip = plan + .tasks + .iter() + .flat_map(|task| task.fragments.iter().map(|fragment| fragment.id)) + .chain( + options + .excluded_fragment_ids + .iter() + .map(|id| u64::from(*id)), + ) + .collect::>(); + for fragment in ds.manifest.fragments.iter() { + if fragment_holds_dropped_column(fragment, &live) && skip.insert(fragment.id) { + plan.tasks.push(TaskData { + fragments: vec![fragment.clone()], + }); + } + } + Ok(plan) + } + + /// RFC 0067: plan ([`Self::plan_table_compaction`]) and execute Lance + /// compaction against a pinned base and stage the result as one `Rewrite` + /// transaction. The new fragments take ids above the base's high-water + /// mark, so the commit needs no `ReserveFragments` (whose replay would + /// not conflict with its twin). A stable-row-id rewrite carries every + /// index's coverage over to the new fragments when Lance applies it. + /// `None` when the plan has no task. Every task of a Blob table is sized + /// before any executes, so a sizing refusal leaves no rewritten file + /// behind. pub async fn stage_compaction( &self, ds: &Dataset, options: &CompactionOptions, ) -> Result> { - let plan = plan_compaction(ds, options) - .await - .map_err(OmniError::storage)?; + let plan = Self::plan_table_compaction(ds, options).await?; if plan.num_tasks() == 0 { return Ok(None); } @@ -4102,6 +4206,216 @@ impl TableStore { Ok(StagedWrite::new(transaction, Vec::new(), Vec::new())) } + /// Plan the metadata-only commits that move a table from its dataset + /// schema to `target`, the desired physical schema in catalog order, with + /// `renames` naming each renamed column as `(from, to)`. Reads only the + /// manifest. + /// + /// A target column maps to the dataset column it is renamed from, or to + /// the column of its own name, and keeps that column's field id, type, + /// nullability and metadata (the unenforced primary key and the Blob-v2 + /// extension included), so the data files and indexes that reference the + /// id keep serving it; only its name and its + /// `omnigraph.stable_property_id` marker follow the target. A marker that + /// names another property refuses. Every other target column is new: it + /// must be nullable, and it takes a field id above every id the table's + /// data files still reference (`Manifest::max_field_id`), so a fragment + /// that lacks it reads it as null. A dataset column no target column maps + /// to is dropped. A column whose type or nullability would change refuses: + /// that needs a rewrite, which schema apply never plans. + pub(crate) fn plan_schema_evolution( + ds: &Dataset, + target: &arrow_schema::Schema, + renames: &[(String, String)], + ) -> Result { + let current = ds.schema(); + let mut source_of = HashMap::<&str, &str>::with_capacity(renames.len()); + for (from, to) in renames { + if source_of.insert(to.as_str(), from.as_str()).is_some() { + return Err(OmniError::manifest_internal(format!( + "schema evolution renames two columns to '{to}'" + ))); + } + } + let mut fields = Vec::with_capacity(target.fields().len()); + let mut consumed = HashSet::<&str>::new(); + let mut renamed = false; + let mut added = false; + for target_field in target.fields() { + let name = target_field.name().as_str(); + let source = match source_of.get(name) { + Some(from) if current.field(from).is_some() => Some(*from), + // The rename was applied by an earlier step of this evolution. + Some(_) if current.field(name).is_some() => Some(name), + Some(from) => { + return Err(OmniError::manifest_internal(format!( + "column '{name}' is renamed from '{from}', which the dataset does not have" + ))); + } + None => current.field(name).map(|_| name), + }; + let Some(source) = source else { + if !target_field.is_nullable() { + return Err(OmniError::manifest_internal(format!( + "new column '{name}' is not nullable; adding it without a rewrite needs a nullable column" + ))); + } + fields.push( + lance::datatypes::Field::try_from(target_field.as_ref()) + .map_err(OmniError::lance_internal)?, + ); + added = true; + continue; + }; + if !consumed.insert(source) { + return Err(OmniError::manifest_internal(format!( + "two target columns map to dataset column '{source}'" + ))); + } + let existing = current + .field(source) + .expect("the source column was found above"); + let desired = lance::datatypes::Field::try_from(target_field.as_ref()) + .map_err(OmniError::lance_internal)?; + if existing.data_type() != desired.data_type() || existing.nullable != desired.nullable + { + return Err(OmniError::manifest_internal(format!( + "column '{source}' cannot become '{name}' without a rewrite: {:?} (nullable {}) -> {:?} (nullable {})", + existing.data_type(), + existing.nullable, + desired.data_type(), + desired.nullable, + ))); + } + let mut field = existing.clone(); + if source != name { + field.name = name.to_string(); + renamed = true; + } + match target_field + .metadata() + .get(crate::db::STABLE_PROPERTY_ID_METADATA_KEY) + { + Some(property_id) => { + match field + .metadata + .get(crate::db::STABLE_PROPERTY_ID_METADATA_KEY) + { + Some(existing_id) if existing_id != property_id => { + return Err(OmniError::manifest_internal(format!( + "column '{source}' carries property identity {existing_id}, not {property_id}" + ))); + } + Some(_) => {} + None => { + field.metadata.insert( + crate::db::STABLE_PROPERTY_ID_METADATA_KEY.to_string(), + property_id.clone(), + ); + } + } + } + None => { + if field + .metadata + .contains_key(crate::db::STABLE_PROPERTY_ID_METADATA_KEY) + { + return Err(OmniError::manifest_internal(format!( + "column '{source}' carries a property identity its target '{name}' lacks" + ))); + } + } + } + fields.push(field); + } + let dropped = current + .fields + .iter() + .any(|field| !consumed.contains(field.name.as_str())); + + let mut evolved = LanceSchema { + fields, + metadata: current.metadata.clone(), + }; + // Only the new columns (id -1) take ids, above every id the table's + // data files still reference. + evolved.set_field_id(Some(ds.manifest.max_field_id())); + evolved.validate().map_err(OmniError::lance_internal)?; + + let expected = schema_columns(current); + if !added { + let project = (&evolved != current).then_some(evolved); + return Ok(SchemaEvolution { + project, + merge: None, + expected, + }); + } + let project = if renamed || dropped { + let surviving = evolved + .fields + .iter() + .filter(|field| current.field_by_id(field.id).is_some()) + .cloned() + .collect(); + Some(LanceSchema { + fields: surviving, + metadata: current.metadata.clone(), + }) + } else { + None + }; + Ok(SchemaEvolution { + project, + merge: Some(evolved), + expected, + }) + } + + /// Stage the next metadata-only commit of a planned schema evolution (see + /// [`Self::plan_schema_evolution`]) against `ds`, or `None` when no step + /// remains. Renames and drops stage first, as `Operation::Project` + /// (Lance's own rename and drop); additions follow, as `Operation::Merge` + /// over `ds`'s unchanged fragments (Lance's own all-null add). Lance + /// refuses a Merge that renames a field, so a table with both kinds + /// commits the Project detached and stages the Merge against that + /// version: at most two commits. `ds` must have the columns the previous + /// step left (the planned base first), or the step refuses. Neither step + /// writes or rewrites a data file or reads a row; every surviving index + /// keeps its coverage, and an index on a dropped column leaves with it. + /// HEAD does NOT advance. + pub async fn stage_schema_evolution( + &self, + ds: &Dataset, + evolution: &mut SchemaEvolution, + ) -> Result> { + if schema_columns(ds.schema()) != evolution.expected { + return Err(OmniError::manifest_internal(format!( + "schema evolution step staged against {} version {}, whose columns are not the ones the plan expects", + ds.uri(), + ds.manifest.version + ))); + } + let operation = if let Some(schema) = evolution.project.take() { + evolution.expected = schema_columns(&schema); + Operation::Project { + schema, + preserves_nullability: true, + } + } else if let Some(schema) = evolution.merge.take() { + evolution.expected = schema_columns(&schema); + Operation::Merge { + fragments: ds.manifest.fragments.as_ref().clone(), + schema, + preserves_nullability: true, + } + } else { + return Ok(None); + }; + let transaction = TransactionBuilder::new(ds.manifest.version, operation).build(); + Ok(Some(StagedWrite::new(transaction, Vec::new(), Vec::new()))) + } + /// Stage an overwrite (write_fragments + Operation::Overwrite { schema, fragments }). /// Returns a StagedWrite carrying the replacement fragments. HEAD does /// NOT advance. diff --git a/crates/omnigraph/src/table_store/staged_tests.rs b/crates/omnigraph/src/table_store/staged_tests.rs index b014a169a..3c6715c50 100644 --- a/crates/omnigraph/src/table_store/staged_tests.rs +++ b/crates/omnigraph/src/table_store/staged_tests.rs @@ -2667,3 +2667,276 @@ fn compaction_blob_batch_rows_bounds_one_batch() { assert_eq!(compaction_blob_batch_rows(BUDGET / 2 + 1), 1); assert_eq!(compaction_blob_batch_rows(BUDGET / 2), 2); } + +/// Lance applies a per-run source budget only to the tasks its own planner +/// returns, so the dropped-column tasks the graph planner appends would run +/// outside it: options that set any budget are refused before planning. +#[tokio::test] +async fn table_compaction_planning_refuses_source_budgets() { + use lance::dataset::optimize::CompactionOptions; + + let dir = tempfile::tempdir().unwrap(); + let uri = format!("{}/people.lance", dir.path().to_str().unwrap()); + let ds = TableStore::write_dataset(&uri, person_batch(&[("alice", Some(30))])) + .await + .unwrap(); + + let unbudgeted = TableStore::plan_table_compaction(&ds, &CompactionOptions::default()).await; + assert!(unbudgeted.is_ok(), "{unbudgeted:?}"); + + let budgets = [ + CompactionOptions { + max_source_fragments: Some(1), + ..CompactionOptions::default() + }, + CompactionOptions { + max_source_rows: Some(1), + ..CompactionOptions::default() + }, + CompactionOptions { + max_source_bytes: Some(1), + ..CompactionOptions::default() + }, + ]; + for options in budgets { + match TableStore::plan_table_compaction(&ds, &options).await { + Err(OmniError::Manifest(manifest)) + if manifest.kind == crate::error::ManifestErrorKind::Internal => {} + other => panic!("a source budget must be refused, got {other:?}"), + } + } +} + +/// The schema-evolution planner reads only the manifest: renames and drops +/// plan one Project, additions one Merge, both a Project then a Merge, an +/// unchanged schema nothing; every change that would need a rewrite refuses. +#[tokio::test] +async fn schema_evolution_plans_metadata_only_steps_and_refuses_rewrites() { + use crate::db::STABLE_PROPERTY_ID_METADATA_KEY as MARKER; + + fn pk_id() -> Field { + Field::new("id", DataType::Utf8, false).with_metadata( + [(LANCE_UNENFORCED_PRIMARY_KEY.to_string(), "true".to_string())] + .into_iter() + .collect(), + ) + } + fn marked(name: &str, data_type: DataType, nullable: bool, id: &str) -> Field { + Field::new(name, data_type, nullable) + .with_metadata([(MARKER.to_string(), id.to_string())].into_iter().collect()) + } + fn renames(pairs: &[(&str, &str)]) -> Vec<(String, String)> { + pairs + .iter() + .map(|(from, to)| (from.to_string(), to.to_string())) + .collect() + } + + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().join("evolve.lance"); + let schema = Arc::new(Schema::new(vec![ + pk_id(), + marked("age", DataType::Int32, true, "1"), + marked("note", DataType::Utf8, true, "2"), + ])); + let batch = RecordBatch::try_new( + schema.clone(), + vec![ + Arc::new(StringArray::from(vec!["a", "b"])), + Arc::new(Int32Array::from(vec![Some(1), None])), + Arc::new(StringArray::from(vec![Some("x"), None])), + ], + ) + .unwrap(); + let ds = Dataset::write( + arrow_array::RecordBatchIterator::new(vec![Ok(batch)], schema.clone()), + uri.to_str().unwrap(), + Some(lance::dataset::WriteParams { + enable_stable_row_ids: true, + data_storage_version: Some(lance_file::version::LanceFileVersion::V2_2), + ..Default::default() + }), + ) + .await + .unwrap(); + let store = TableStore::new(dir.path().to_str().unwrap(), test_session()); + let age_id = ds.schema().field("age").unwrap().id; + let max_id = ds.manifest.max_field_id(); + + let mut unchanged = TableStore::plan_schema_evolution(&ds, &schema, &[]).unwrap(); + assert!(unchanged.project.is_none() && unchanged.merge.is_none()); + assert!( + store + .stage_schema_evolution(&ds, &mut unchanged) + .await + .unwrap() + .is_none() + ); + + let renamed = Schema::new(vec![pk_id(), marked("years", DataType::Int32, true, "1")]); + let mut plan = + TableStore::plan_schema_evolution(&ds, &renamed, &renames(&[("age", "years")])).unwrap(); + assert!(plan.merge.is_none()); + let project = plan.project.clone().unwrap(); + assert_eq!(project.field("years").unwrap().id, age_id); + assert!(project.field("note").is_none()); + let staged = store + .stage_schema_evolution(&ds, &mut plan) + .await + .unwrap() + .unwrap(); + assert!(matches!( + staged.transaction.operation, + Operation::Project { + preserves_nullability: true, + .. + } + )); + assert_eq!(staged.transaction.read_version, ds.version().version); + // The plan is consumed; a step staged against a dataset the previous + // step did not leave refuses. + assert!(plan.project.is_none()); + let error = store + .stage_schema_evolution(&ds, &mut plan) + .await + .unwrap_err(); + assert!( + error.to_string().contains("not the ones the plan expects"), + "{error}" + ); + + let added = Schema::new(vec![ + pk_id(), + marked("age", DataType::Int32, true, "1"), + marked("city", DataType::Utf8, true, "3"), + marked("note", DataType::Utf8, true, "2"), + ]); + let mut plan = TableStore::plan_schema_evolution(&ds, &added, &[]).unwrap(); + assert!(plan.project.is_none()); + let merge = plan.merge.clone().unwrap(); + assert_eq!( + merge + .fields + .iter() + .map(|field| field.name.as_str()) + .collect::>(), + ["id", "age", "city", "note"], + "the evolved schema keeps the target's column order" + ); + assert_eq!(merge.field("city").unwrap().id, max_id + 1); + let staged = store + .stage_schema_evolution(&ds, &mut plan) + .await + .unwrap() + .unwrap(); + match &staged.transaction.operation { + Operation::Merge { fragments, .. } => { + assert_eq!(fragments.as_slice(), ds.manifest.fragments.as_slice()) + } + other => panic!("expected a Merge, got {other:?}"), + } + + let both = Schema::new(vec![ + pk_id(), + marked("city", DataType::Utf8, true, "3"), + marked("years", DataType::Int32, true, "1"), + ]); + let plan = + TableStore::plan_schema_evolution(&ds, &both, &renames(&[("age", "years")])).unwrap(); + let project = plan.project.unwrap(); + assert_eq!( + project + .fields + .iter() + .map(|field| field.name.as_str()) + .collect::>(), + ["id", "years"], + "the Project carries the renames and drops, the Merge the additions" + ); + assert!(plan.merge.unwrap().field("city").is_some()); + + let refusals = [ + ( + Schema::new(vec![pk_id(), marked("age", DataType::Int64, true, "1")]), + Vec::new(), + "without a rewrite", + ), + ( + Schema::new(vec![pk_id(), marked("age", DataType::Int32, false, "1")]), + Vec::new(), + "without a rewrite", + ), + ( + Schema::new(vec![ + pk_id(), + marked("age", DataType::Int32, true, "1"), + marked("city", DataType::Utf8, false, "3"), + ]), + Vec::new(), + "is not nullable", + ), + ( + Schema::new(vec![pk_id(), marked("age", DataType::Int32, true, "9")]), + Vec::new(), + "carries property identity 1, not 9", + ), + ( + Schema::new(vec![pk_id(), marked("years", DataType::Int32, true, "1")]), + renames(&[("ghost", "years")]), + "which the dataset does not have", + ), + ]; + for (target, renames, expected) in refusals { + let error = TableStore::plan_schema_evolution(&ds, &target, &renames).unwrap_err(); + assert!( + error.to_string().contains(expected), + "expected '{expected}', got: {error}" + ); + } + + // A column written before property markers existed adopts its identity + // from the target, as the replaced full-table rewrite did. + let unmarked_schema = Arc::new(Schema::new(vec![ + pk_id(), + Field::new("age", DataType::Int32, true), + ])); + let unmarked = Dataset::write( + arrow_array::RecordBatchIterator::new( + vec![Ok(RecordBatch::try_new( + unmarked_schema.clone(), + vec![ + Arc::new(StringArray::from(vec!["a"])), + Arc::new(Int32Array::from(vec![Some(1)])), + ], + ) + .unwrap())], + unmarked_schema, + ), + dir.path().join("unmarked.lance").to_str().unwrap(), + Some(lance::dataset::WriteParams { + enable_stable_row_ids: true, + data_storage_version: Some(lance_file::version::LanceFileVersion::V2_2), + ..Default::default() + }), + ) + .await + .unwrap(); + let marked_target = Schema::new(vec![pk_id(), marked("age", DataType::Int32, true, "1")]); + let adopted = TableStore::plan_schema_evolution(&unmarked, &marked_target, &[]) + .unwrap() + .project + .unwrap(); + assert_eq!( + adopted + .field("age") + .unwrap() + .metadata + .get(MARKER) + .map(String::as_str), + Some("1") + ); + assert_eq!( + adopted.field("age").unwrap().id, + unmarked.schema().field("age").unwrap().id + ); +} diff --git a/crates/omnigraph/tests/branching.rs b/crates/omnigraph/tests/branching.rs index ef5571033..75e4440ff 100644 --- a/crates/omnigraph/tests/branching.rs +++ b/crates/omnigraph/tests/branching.rs @@ -742,6 +742,112 @@ async fn branch_merge_with_blob_columns_preserves_blob_data() { ); } +/// Adding a nullable Blob property writes no data, so rows written before the +/// add sit in a fragment with no data file for it, which Lance reads as null. +/// Both sides of a general three-way merge update rows of that fragment, so +/// every row the merge classifies has its base image there; the merge compares +/// them and carries each side's values. +#[tokio::test] +async fn branch_merge_compares_rows_written_before_a_blob_property_was_added() { + const WRITES: &str = r#" +query set_note($title: String, $note: String) { + update Document set { note: $note } where title = $title +} + +query set_thumb($title: String, $thumb: Blob) { + update Document set { thumb: $thumb } where title = $title +} +"#; + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap(); + let db = helpers::session(Omnigraph::init(uri, BLOB_SCHEMA).await.unwrap()); + db.load_jsonl( + concat!( + "{\"type\":\"Document\",\"data\":{\"title\":\"a\",\"content\":\"base64:QQ==\",\"note\":\"a\"}}\n", + "{\"type\":\"Document\",\"data\":{\"title\":\"b\",\"note\":\"b\"}}\n", + "{\"type\":\"Document\",\"data\":{\"title\":\"c\",\"note\":\"c\"}}", + ), + LoadMode::Overwrite, + ) + .await + .unwrap(); + let evolved = BLOB_SCHEMA.replace( + " note: String?\n", + " note: String?\n thumb: Blob?\n", + ); + assert!(db.apply_schema(&evolved).await.unwrap().applied); + db.branch_create("feature").await.unwrap(); + + mutate_main( + &db, + WRITES, + "set_note", + ¶ms(&[("$title", "a"), ("$note", "main")]), + ) + .await + .unwrap(); + mutate_branch( + &db, + "feature", + WRITES, + "set_note", + ¶ms(&[("$title", "b"), ("$note", "feature")]), + ) + .await + .unwrap(); + mutate_branch( + &db, + "feature", + WRITES, + "set_thumb", + ¶ms(&[("$title", "c"), ("$thumb", "base64:Yw==")]), + ) + .await + .unwrap(); + + assert_eq!( + db.branch_merge("feature", "main").await.unwrap().outcome, + MergeOutcome::Merged + ); + let exported = db + .export_jsonl("main", &["Document".to_string()]) + .await + .unwrap(); + let mut rows = exported + .lines() + .map(|line| { + let row: serde_json::Value = serde_json::from_str(line).unwrap(); + let cell = |name: &str| row["data"].get(name).cloned().unwrap_or_default(); + (cell("title"), cell("note"), cell("content"), cell("thumb")) + }) + .collect::>(); + rows.sort_by_key(|row| row.0.to_string()); + let null = serde_json::Value::Null; + assert_eq!( + rows, + [ + ( + serde_json::json!("a"), + serde_json::json!("main"), + serde_json::json!("base64:QQ=="), + null.clone(), + ), + ( + serde_json::json!("b"), + serde_json::json!("feature"), + null.clone(), + null.clone(), + ), + ( + serde_json::json!("c"), + serde_json::json!("c"), + null, + serde_json::json!("base64:Yw=="), + ), + ] + ); +} + #[tokio::test] async fn blob_named_branch_delete_recreate_never_retargets_cached_or_snapshot_reads() { // Lance branch versions live in independent namespaces and a deleted diff --git a/crates/omnigraph/tests/changes.rs b/crates/omnigraph/tests/changes.rs index 3167d4085..34d4afe60 100644 --- a/crates/omnigraph/tests/changes.rs +++ b/crates/omnigraph/tests/changes.rs @@ -2444,7 +2444,7 @@ async fn commit_changes_page_token_rejections_are_typed() { #[tokio::test] async fn commit_changes_refuse_unprovable_schema_boundary() { - use omnigraph::changes::ChangeFeedScope; + use omnigraph::changes::{ChangeFeedPosition, ChangeFeedScope, ChangeFeedStart}; use omnigraph::error::OmniError; let dir = tempfile::tempdir().unwrap(); @@ -2469,20 +2469,23 @@ node Ghost { let scope = ChangeFeedScope::default(); db.load_with_receipt( "main", - r#"{"type":"Person","data":{"name":"Alice","age":30}}"#, + "{\"type\":\"Person\",\"data\":{\"name\":\"Alice\",\"age\":30}}\n\ + {\"type\":\"Person\",\"data\":{\"name\":\"Carol\",\"age\":50}}", LoadMode::Merge, ) .await .unwrap(); - // (a) A property add rewrites the table: the two pinned endpoints of the - // schema-apply commit no longer share one user schema. + // (a) A property add changes the table's user schema (metadata-only, no + // row rewritten): the two pinned endpoints of the schema-apply commit no + // longer share one user schema. db.apply_schema( r#" node Person { name: String @key age: I32? note: String? + attachment: Blob? } node Ghost { @@ -2502,6 +2505,133 @@ node Ghost { other => panic!("expected a typed schema boundary, got: {other:?}"), } + // The add kept every data file: the fragment holding Alice and Carol + // physically lacks `note` and `attachment`, and Lance reads both as null + // there; a null Blob names no data file. Later commits name only the rows + // they wrote, with exact images whose before side is read from that + // fragment, and the net diff since the add is exactly those rows, never + // every row of a fragment that lacks the added columns. + let after_add = snapshot_id(&db, "main").await.unwrap(); + db.load_with_receipt( + "main", + r#"{"type":"Person","data":{"name":"Bob","age":40,"attachment":"base64:Ym9i"}}"#, + LoadMode::Merge, + ) + .await + .unwrap(); + let writes = r#" +query set_note($name: String, $note: String) { + update Person set { note: $note } where name = $name +} + +query set_attachment($name: String, $attachment: Blob) { + update Person set { attachment: $attachment } where name = $name +} +"#; + let set_note = db + .mutate_with_receipt( + "main", + writes, + "set_note", + ¶ms(&[("$name", "Alice"), ("$note", "hello")]), + ) + .await + .unwrap() + .commit + .expect("the update publishes"); + db.mutate_with_receipt( + "main", + writes, + "set_attachment", + ¶ms(&[("$name", "Carol"), ("$attachment", "base64:Y2Fyb2w=")]), + ) + .await + .unwrap(); + type Images = Option<(serde_json::Value, serde_json::Value)>; + let images = |change: &omnigraph::changes::GraphEntityChange| -> (String, _, Images, Images) { + let note_and_attachment = |image: Option<&omnigraph::changes::EntityImage>| { + image.map(|image| { + ( + image.properties["note"].clone(), + image.properties["attachment"].clone(), + ) + }) + }; + ( + change.id.clone(), + change.op, + note_and_attachment(change.before.as_ref()), + note_and_attachment(change.after.as_ref()), + ) + }; + let null = serde_json::Value::Null; + let alice = ( + "Alice".to_string(), + omnigraph::changes::ChangeOpKind::Update, + Some((null.clone(), null.clone())), + Some((serde_json::json!("hello"), null.clone())), + ); + let page = db + .commit_changes_page(&set_note.graph_commit_id, &scope, None, None, None) + .await + .unwrap(); + assert_eq!( + page.block.changes.iter().map(images).collect::>(), + std::slice::from_ref(&alice) + ); + let feed = db + .poll_change_feed(feed_request( + None, + ChangeFeedPosition::Start(ChangeFeedStart::AfterCommit(add_commit.clone())), + )) + .await + .unwrap(); + assert_eq!( + feed.blocks + .iter() + .flat_map(|block| &block.changes) + .map(images) + .collect::>(), + [ + ( + "Bob".to_string(), + omnigraph::changes::ChangeOpKind::Insert, + None, + Some((null.clone(), serde_json::json!("base64:Ym9i"))), + ), + alice, + ( + "Carol".to_string(), + omnigraph::changes::ChangeOpKind::Update, + Some((null.clone(), null.clone())), + Some((null.clone(), serde_json::json!("base64:Y2Fyb2w="))), + ), + ] + ); + let since_add = diff_since_branch(&db, "main", after_add, &ChangeFilter::default()) + .await + .unwrap(); + assert_eq!( + change_tuples(&since_add), + [ + ( + "node:Person".to_string(), + "Alice".to_string(), + ChangeOp::Update + ), + ( + "node:Person".to_string(), + "Bob".to_string(), + ChangeOp::Insert + ), + ( + "node:Person".to_string(), + "Carol".to_string(), + ChangeOp::Update + ), + ] + ); + // (b) Dropping a type that still holds data is schema evolution with data // present — refused, never synthesized into entity deletes. db.apply_schema( diff --git a/crates/omnigraph/tests/detached_commit_matrix.rs b/crates/omnigraph/tests/detached_commit_matrix.rs index c0dbebaf8..c596cf5b8 100644 --- a/crates/omnigraph/tests/detached_commit_matrix.rs +++ b/crates/omnigraph/tests/detached_commit_matrix.rs @@ -64,10 +64,12 @@ enum Writer { /// A fast-forward merge of a branch holding one Person insert into main: /// a pointer switch with no table effect, so only its pre-publish window. Merge, - /// Schema apply adding a nullable Person property: one detached rewrite - /// of Person, no row change, the contract row published atomically. + /// Schema apply evolving two tables by metadata-only commits, no row + /// change, the contract row published atomically: Knows renames `since` + /// and adds `weight` (a Project, then a Merge chained on it), and Person + /// adds `city` (one Merge). SchemaApply, - /// The same rewrite through an intent persisted before invocation and + /// The same evolution through an intent persisted before invocation and /// deserialized by the child; recovery never reruns that original intent. PreparedSchemaApply, /// Optimize over a Person table with four small fragments: one detached @@ -89,7 +91,12 @@ enum Writer { } fn city_schema() -> String { - helpers::TEST_SCHEMA.replace("age: I32?", "age: I32?\n city: String?") + helpers::TEST_SCHEMA + .replace("age: I32?", "age: I32?\n city: String?") + .replace( + "since: Date?", + "met: Date? @rename_from(\"since\")\n weight: I32?", + ) } #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -129,7 +136,11 @@ impl Writer { Writer::Cleanup => vec![], Writer::EnsureIndices => vec![PostDetached(1), PrePublish], Writer::Merge => vec![PrePublish], - Writer::SchemaApply | Writer::PreparedSchemaApply => vec![PostDetached(1), PrePublish], + // Knows' Project and Merge, then Person's Merge: park inside + // Knows' chain, after it, and before publication. + Writer::SchemaApply | Writer::PreparedSchemaApply => { + vec![PostDetached(1), PostDetached(2), PrePublish] + } Writer::Optimize => vec![PostDetached(1), PrePublish], Writer::Load => vec![PostDetached(1), PrePublish], Writer::FtsRebuild => vec![PostDetached(1), PrePublish], diff --git a/crates/omnigraph/tests/failpoints.rs b/crates/omnigraph/tests/failpoints.rs index 6a113a21f..1384775ca 100644 --- a/crates/omnigraph/tests/failpoints.rs +++ b/crates/omnigraph/tests/failpoints.rs @@ -2696,7 +2696,7 @@ async fn schema_apply_pre_staging_failure_leaves_no_residue() { assert_no_recovery_sidecars(dir.path()); assert_no_staging_files(dir.path()); - // The Person rewrite is a detached version and the Tag create is an + // The Person column add is a detached version and the Tag create is an // unregistered dataset: nothing moved the manifest or any linear HEAD, // so reopening has nothing to roll back. let db = helpers::session(Omnigraph::open(&uri).await.unwrap()); @@ -2720,7 +2720,7 @@ async fn schema_apply_pre_staging_failure_leaves_no_residue() { assert_eq!( person_head.version().version, person_head_before, - "a detached rewrite never moves the linear HEAD" + "a detached schema evolution never moves the linear HEAD" ); assert!(snapshot.dataset("node:Tag").is_none()); let live_schema = db.schema_source(); @@ -2729,7 +2729,7 @@ async fn schema_apply_pre_staging_failure_leaves_no_residue() { db.apply_schema(&v2_schema) .await - .expect("the retry rewrites from the pin and reclaims the Tag leftover"); + .expect("the retry evolves from the pin and reclaims the Tag leftover"); assert!(db.schema_source().contains("city: String?")); assert_eq!(helpers::count_rows(&db, "node:Tag").await, 0); assert_eq!(helpers::count_rows(&db, "node:Person").await, 1); @@ -3071,7 +3071,7 @@ edge WorksAt: Human -> Company let error = db .apply_schema(desired) .await - .expect_err("rename+rewrite must stop after its detached table effect"); + .expect_err("rename + key-property rename must stop after its detached table effect"); assert!( error.to_string().contains("schema_apply.post_table_commit"), "unexpected partial rename error: {error}" @@ -3081,8 +3081,8 @@ edge WorksAt: Human -> Company assert_no_staging_files(dir.path()); drop(db); - // The rewrite is a detached version behind the source alias's pin; the - // rename was never published. Reopening finds the graph untouched. + // The column rename is a detached Project behind the source alias's pin; + // the type rename was never published. Reopening finds the graph untouched. let recovered = helpers::session(Omnigraph::open(&uri).await.unwrap()); let snapshot = recovered .snapshot_of(omnigraph::db::ReadTarget::branch("main")) @@ -3099,7 +3099,7 @@ edge WorksAt: Human -> Company recovered .apply_schema(desired) .await - .expect("the retry publishes the rename and rewrite"); + .expect("the retry publishes the type and property renames"); assert_eq!( helpers::count_rows(&recovered, "node:Human").await, people_before @@ -3164,7 +3164,7 @@ async fn schema_apply_partial_table_effect_leaves_no_residue() { assert_eq!( head.version().version, heads_before[type_name], - "{type_name}: a detached rewrite never moves the linear HEAD" + "{type_name}: a detached schema evolution never moves the linear HEAD" ); } assert_eq!( @@ -3270,7 +3270,7 @@ async fn schema_apply_loses_the_manifest_cas_to_a_concurrent_publication_without .version() .version, winner_lance_head, - "the abandoned detached rewrite never moved the linear HEAD" + "the abandoned detached schema evolution never moved the linear HEAD" ); assert_eq!( branch_head_commit_id(dir.path(), "main").await.unwrap(), diff --git a/crates/omnigraph/tests/forbidden_apis.rs b/crates/omnigraph/tests/forbidden_apis.rs index 126229ddf..428cd9f6a 100644 --- a/crates/omnigraph/tests/forbidden_apis.rs +++ b/crates/omnigraph/tests/forbidden_apis.rs @@ -727,8 +727,8 @@ gateway_surfaces! { ], "storage_layer.rs" => "TableStorage" => GatewayDisposition::StageOnly => [ "stage_create", "stage_keyed_write", "stage_proven_strict_insert", "stage_overwrite", - "stage_rename_columns", "stage_delete", "stage_create_indices", "stage_compaction", - "stage_index_fold", + "stage_rename_columns", "stage_schema_evolution", "stage_delete", "stage_create_indices", + "stage_compaction", "stage_index_fold", ], "storage_layer.rs" => "TableStorage" => GatewayDisposition::Durable(WriteProtocol::NativeRefControl) => [ "force_delete_branch", @@ -768,11 +768,13 @@ gateway_surfaces! { "predicted_materialized_blob_batch_bytes", "materialize_blob_batch_bounded_with_preflight_cache", "managed_blob_payloads", "can_fold_index", "has_foldable_unindexed_fragments", "index_is_vector", + "plan_table_compaction", ], "table_store.rs" => "TableStore" => GatewayDisposition::StageOnly => [ "stage_create", "stage_keyed_write", "stage_proven_strict_insert", "stage_overwrite", - "stage_rename_columns", "renamed_schema", "stage_delete", "stage_create_indices", - "stage_compaction", "stage_index_fold", + "stage_rename_columns", "renamed_schema", "stage_schema_evolution", + "plan_schema_evolution", "stage_delete", "stage_create_indices", "stage_compaction", + "stage_index_fold", ], "table_store.rs" => "TableStore" => GatewayDisposition::Durable(WriteProtocol::NativeRefControl) => [ "force_delete_branch", @@ -896,7 +898,7 @@ durable_calls! { ("storage_layer.rs", ".commit_staged_exact(", 1, WriteProtocol::Exact("sealed TableStorage forwarding")), ("storage_layer.rs", ".commit_staged_detached(", 1, WriteProtocol::Exact("sealed TableStorage forwarding")), ("db/omnigraph/promotion.rs", "SnapshotHandle::new(", 1, WriteProtocol::ReadOnlyAccess), - ("storage_layer.rs", ".dataset()", 30, WriteProtocol::Composed("sealed TableStorage forwarding")), + ("storage_layer.rs", ".dataset()", 31, WriteProtocol::Composed("sealed TableStorage forwarding")), ("storage_layer.rs", ".into_arc()", 5, WriteProtocol::Composed("sealed TableStorage forwarding")), ("storage_layer.rs", "SnapshotHandle::new(", 3, WriteProtocol::Composed("sealed TableStorage forwarding")), ("table_store.rs", ".raw_dataset_append(", 1, WriteProtocol::EphemeralScratch), @@ -913,7 +915,7 @@ durable_calls! { ("exec/merge.rs", ".commit_staged_detached(", 1, WriteProtocol::Exact("RFC 0067 detached merge chain")), ("exec/merge.rs", ".dataset()", 4, WriteProtocol::Exact("RFC 0067 detached merge chain")), ("db/omnigraph/schema_apply.rs", ".commit_staged_create_exact(", 1, SCHEMA_V9), - ("db/omnigraph/schema_apply.rs", ".commit_staged_detached(", 2, WriteProtocol::Exact("detached schema rewrite + incompatible original-empty-table retry")), + ("db/omnigraph/schema_apply.rs", ".commit_staged_detached(", 2, WriteProtocol::Exact("detached metadata-only schema evolution + incompatible original-empty-table retry")), ("db/omnigraph/table_ops.rs", ".commit_staged(", 1, WriteProtocol::Composed("shared merge/Optimize index tail")), ("db/omnigraph/table_ops.rs", ".commit_staged_detached(", 1, WriteProtocol::Exact("RFC 0067 detached index batch")), ("exec/staging.rs", ".commit_staged_detached(", 1, WriteProtocol::Exact("Mutation/Load detached staging (RFC 0067)")), @@ -979,7 +981,7 @@ durable_calls! { ("changes/mod.rs", ".dataset()", 11, WriteProtocol::ReadOnlyAccess), ("db/omnigraph/collector.rs", ".dataset()", 17, WriteProtocol::ReadOnlyAccess), ("db/omnigraph/collector.rs", ".delete(", 1, WriteProtocol::Composed("detached-only sweep through the table's own Lance store: freed files, then the published manifests no retained `__manifest` version pins (links before tips) and the dead stagings; an interrupted pass is re-swept by the next")), - ("db/omnigraph/schema_apply.rs", ".dataset()", 2, SCHEMA_V9), + ("db/omnigraph/schema_apply.rs", ".dataset()", 1, SCHEMA_V9), ("db/omnigraph/repair.rs", ".dataset()", 1, WriteProtocol::ManifestAdoption), // The sixth accessor reports deferred FTS coverage from an immutable // snapshot; it only reads index metadata and never stages or publishes. @@ -3693,11 +3695,6 @@ fn lance_batched_blob_read_call_counts_are_pinned() { "db/omnigraph/export.rs".to_string(), 1 ), - ( - "managed_blob_payloads", - "db/omnigraph/schema_apply.rs".to_string(), - 1 - ), ("managed_blob_payloads", "table_store.rs".to_string(), 1), ("read_blobs", "table_store.rs".to_string(), 1), ("take_blobs", "blob.rs".to_string(), 1), diff --git a/crates/omnigraph/tests/lance_surface_guards.rs b/crates/omnigraph/tests/lance_surface_guards.rs index 532669df5..603dcaaf1 100644 --- a/crates/omnigraph/tests/lance_surface_guards.rs +++ b/crates/omnigraph/tests/lance_surface_guards.rs @@ -808,6 +808,663 @@ async fn rename_only_project_keeps_fragments_field_ids_pk_marker_and_indexes() { assert_eq!(dataset.count_rows(None).await.unwrap(), 2); } +/// Guard: schema apply's metadata-only evolution. On a stable-row-id V2_2 +/// table holding inline, packed and dedicated Blob-v2 payloads, null and a +/// valid empty value, a detached `Operation::Project` renames and drops +/// columns and a detached `Operation::Merge` chained on it adds nullable +/// String and Blob columns over the unchanged fragments. Neither writes a +/// data file; surviving fields keep their ids, metadata and indexes with the +/// same fragment coverage; the index on the dropped column leaves; every +/// payload reads back; the added columns read null; HEAD never moves. Lance +/// refuses a Merge that renames a field, which is why the engine chains. +#[tokio::test] +async fn metadata_only_schema_evolution_keeps_blob_files_ids_and_index_coverage() { + use arrow_array::types::UInt64Type; + + const PROPERTY_ID: &str = "omnigraph.stable_property_id"; + + fn data_files(ds: &Dataset) -> Vec<(u64, Vec)> { + ds.get_fragments() + .iter() + .map(|fragment| { + ( + fragment.id() as u64, + fragment + .metadata() + .files + .iter() + .map(|file| file.path.clone()) + .collect(), + ) + }) + .collect() + } + + async fn index_coverage(ds: &Dataset) -> HashMap, Vec)> { + ds.load_indices() + .await + .unwrap() + .iter() + .filter(|index| !lance_index::is_system_index(index)) + .map(|index| { + ( + index.name.clone(), + ( + index.fields.clone(), + index + .fragment_bitmap + .as_ref() + .map(|bitmap| bitmap.iter().collect()) + .unwrap_or_default(), + ), + ) + }) + .collect() + } + + async fn commit_detached(ds: &Dataset, operation: Operation) -> lance::Result { + let transaction = Transaction::new(ds.version().version, operation, None); + CommitBuilder::new(Arc::new(ds.clone())) + .with_detached(true) + .with_skip_auto_cleanup(true) + .execute(transaction) + .await + } + + async fn ids_and_row_ids(ds: &Dataset) -> (Vec, Vec) { + let mut scanner = ds.scan(); + scanner.with_row_id(); + scanner.project(&["id"]).unwrap(); + let batch = scanner.try_into_batch().await.unwrap(); + let ids = batch + .column_by_name("id") + .unwrap() + .as_string::() + .iter() + .map(|id| id.unwrap().to_string()) + .collect(); + let row_ids = batch + .column_by_name(ROW_ID) + .unwrap() + .as_primitive::() + .values() + .to_vec(); + (ids, row_ids) + } + + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().join("evolve.lance"); + let uri = uri.to_str().unwrap(); + let payloads: Vec>> = vec![ + Some(vec![b'i'; 80]), + Some(vec![b'p'; 96 * 1024]), + None, + Some(Vec::new()), + Some(vec![b'd'; 5 * 1024 * 1024]), + ]; + let mut content = BlobArrayBuilder::new(payloads.len()); + for payload in &payloads { + match payload { + Some(payload) => content.push_bytes(payload).unwrap(), + None => content.push_null().unwrap(), + } + } + let marked = |field: Field, id: &str| { + let mut metadata = field.metadata().clone(); + metadata.insert(PROPERTY_ID.to_string(), id.to_string()); + field.with_metadata(metadata) + }; + let mut pk = HashMap::new(); + pk.insert(LANCE_UNENFORCED_PRIMARY_KEY.to_string(), "true".to_string()); + let schema = Arc::new(Schema::new(vec![ + Field::new("id", DataType::Utf8, false).with_metadata(pk), + marked(Field::new("name", DataType::Utf8, true), "11"), + marked(Field::new("note", DataType::Utf8, true), "12"), + marked(lance::blob::blob_field("content", true), "13"), + ])); + let ids = ["r0", "r1", "r2", "r3", "r4"]; + let batch = RecordBatch::try_new( + schema.clone(), + vec![ + Arc::new(StringArray::from(ids.to_vec())), + Arc::new(StringArray::from(vec!["a", "b", "c", "d", "e"])), + Arc::new(StringArray::from(vec!["n0", "n1", "n2", "n3", "n4"])), + content.finish().unwrap(), + ], + ) + .unwrap(); + let mut base = Dataset::write( + RecordBatchIterator::new(vec![Ok(batch)], schema), + uri, + Some(WriteParams { + mode: WriteMode::Create, + enable_stable_row_ids: true, + data_storage_version: Some(LanceFileVersion::V2_2), + max_rows_per_file: 3, + ..Default::default() + }), + ) + .await + .unwrap(); + for column in ["name", "note"] { + base.create_index( + &[column], + IndexType::Scalar, + None, + &ScalarIndexParams::default(), + false, + ) + .await + .unwrap(); + } + assert_eq!(base.get_fragments().len(), 2, "two fragments"); + let head = base.version().version; + let files = data_files(&base); + let coverage = index_coverage(&base).await; + let name = base.schema().field("name").unwrap().clone(); + let content_field = base.schema().field("content").unwrap().clone(); + let (_, row_ids) = ids_and_row_ids(&base).await; + + // A Merge cannot carry a rename: Lance refuses to move a field id to + // another path without rewriting it. + let mut renamed_in_merge = base.schema().clone(); + renamed_in_merge.mut_field_by_id(name.id).unwrap().name = "title".to_string(); + let error = commit_detached( + &base, + Operation::Merge { + fragments: base.manifest.fragments.as_ref().clone(), + schema: renamed_in_merge, + preserves_nullability: true, + }, + ) + .await + .expect_err("a Merge that renames a field must be refused"); + assert!( + error.to_string().contains("remaps field id"), + "unexpected refusal: {error}" + ); + + // Step 1: Project renames `name` to `title` and drops `note`. + let mut projected = base.schema().clone(); + projected.mut_field_by_id(name.id).unwrap().name = "title".to_string(); + projected.fields.retain(|field| field.name != "note"); + projected.validate().unwrap(); + let projected = commit_detached( + &base, + Operation::Project { + schema: projected, + preserves_nullability: true, + }, + ) + .await + .unwrap(); + assert!(lance_table::format::is_detached_version( + projected.version().version + )); + assert_eq!(data_files(&projected), files, "Project writes no data file"); + let title = projected.schema().field("title").unwrap(); + assert_eq!(title.id, name.id); + assert_eq!( + title.metadata, name.metadata, + "the renamed field keeps its metadata" + ); + assert!(projected.schema().field("note").is_none()); + let projected_coverage = index_coverage(&projected).await; + assert_eq!( + projected_coverage.get("name_idx"), + coverage.get("name_idx"), + "the renamed column's index keeps its field id and fragment coverage" + ); + assert!( + !projected_coverage.contains_key("note_idx"), + "the dropped column's index leaves with it" + ); + + // Step 2: Merge adds nullable `city` and Blob `thumb` over the unchanged + // fragments, with ids above every id the data files reference. + let max_field_id = projected.manifest.max_field_id(); + let mut evolved = projected.schema().clone(); + for added in [ + marked(Field::new("city", DataType::Utf8, true), "14"), + marked(lance::blob::blob_field("thumb", true), "15"), + ] { + evolved + .fields + .push(lance::datatypes::Field::try_from(&added).unwrap()); + } + evolved.set_field_id(Some(max_field_id)); + evolved.validate().unwrap(); + let evolved = commit_detached( + &projected, + Operation::Merge { + fragments: projected.manifest.fragments.as_ref().clone(), + schema: evolved, + preserves_nullability: true, + }, + ) + .await + .unwrap(); + assert_eq!(data_files(&evolved), files, "Merge writes no data file"); + for added in ["city", "thumb"] { + let field = evolved.schema().field(added).unwrap(); + assert!(field.id > max_field_id, "{added} takes a fresh field id"); + assert!(field.metadata.contains_key(PROPERTY_ID)); + } + assert!(evolved.schema().field("thumb").unwrap().is_blob()); + let kept = evolved.schema().field("content").unwrap(); + assert_eq!(kept.id, content_field.id); + assert_eq!(kept.metadata, content_field.metadata); + assert_eq!( + evolved + .schema() + .unenforced_primary_key() + .iter() + .map(|field| field.name.as_str()) + .collect::>(), + ["id"] + ); + assert_eq!( + index_coverage(&evolved).await, + projected_coverage, + "the Merge keeps every index and its coverage" + ); + + // Every managed payload, the null and the valid empty value read back by + // stable row id; the added Blob column reads null for every row. + let (evolved_ids, evolved_row_ids) = ids_and_row_ids(&evolved).await; + assert_eq!(evolved_ids, ids); + assert_eq!(evolved_row_ids, row_ids, "stable row ids are unchanged"); + let evolved = Arc::new(evolved); + let blobs = evolved + .read_blobs("content") + .unwrap() + .with_row_ids(row_ids.clone()) + .preserve_order(true) + .execute() + .await + .unwrap(); + for (blob, expected) in blobs.iter().zip(&payloads) { + assert_eq!(blob.data.as_deref(), expected.as_deref()); + } + let thumbs = evolved + .read_blobs("thumb") + .unwrap() + .with_row_ids(row_ids) + .execute() + .await + .unwrap(); + assert_eq!(thumbs.len(), payloads.len()); + assert!(thumbs.iter().all(|blob| blob.data.is_none())); + let mut scanner = evolved.scan(); + scanner.blob_handling(BlobHandling::AllBinary); + scanner + .project(&["id", "title", "city", "thumb", "content"]) + .unwrap(); + let batch = scanner.try_into_batch().await.unwrap(); + assert_eq!(batch.num_rows(), ids.len()); + assert_eq!( + batch.column_by_name("city").unwrap().null_count(), + ids.len() + ); + assert_eq!( + batch.column_by_name("thumb").unwrap().null_count(), + ids.len() + ); + assert_eq!( + batch + .column_by_name("title") + .unwrap() + .as_string::() + .iter() + .map(Option::unwrap) + .collect::>(), + ["a", "b", "c", "d", "e"] + ); + let contents = batch.column_by_name("content").unwrap().as_binary::(); + for (row, expected) in payloads.iter().enumerate() { + match expected { + Some(expected) => assert_eq!(contents.value(row), expected.as_slice()), + None => assert!(contents.is_null(row)), + } + } + + // The base version still reads its own schema, and nothing moved HEAD. + let base_again = DatasetBuilder::from_uri(uri) + .with_version(base.version().version) + .load() + .await + .unwrap(); + assert!(base_again.schema().field("note").is_some()); + assert!(base_again.schema().field("title").is_none()); + assert_eq!( + Dataset::open(uri).await.unwrap().version().version, + head, + "detached evolution commits never move HEAD" + ); +} + +/// Guard: what `optimize`'s erasure of dropped properties relies on +/// (`TableStore::plan_table_compaction`). On a stable-row-id V2_2 table holding +/// inline, packed and dedicated Blob-v2 payloads, every field id a data file +/// lists is a schema field id (a Blob-v2 column is listed by its own id, not +/// by its descriptor children's), so no fragment of an undropped table reads +/// as holding a dropped column. A detached `Operation::Project` dropping a +/// String and a Blob column keeps the data file and its `fields` list, which +/// still names exactly the two dropped ids; Lance's default planner selects +/// nothing for a lone fragment without deletions, dropped columns or not, and +/// compacts by re-encoding. A +/// `CompactionTask` naming that one fragment, executed and committed as a +/// detached single-group `Rewrite`, writes data files whose `fields` are the +/// current schema's ids only, writes no Blob sidecar for the dropped column +/// while carrying the kept one, keeps every row, value and stable row id, and +/// carries the surviving scalar index over to the new fragment. +#[tokio::test] +async fn compacting_a_chosen_fragment_drops_the_bytes_of_dropped_columns() { + use arrow_array::types::UInt64Type; + use std::collections::BTreeSet; + + use lance::dataset::optimize::{CompactionMode, CompactionTask, TaskData, plan_compaction}; + use lance::dataset::transaction::RewriteGroup; + + fn file_field_ids(ds: &Dataset) -> Vec<(String, Vec)> { + ds.get_fragments() + .iter() + .flat_map(|fragment| fragment.metadata().files.clone()) + .map(|file| (file.path.clone(), file.fields.to_vec())) + .collect() + } + + async fn sidecar_parents(ds: &Dataset) -> BTreeSet { + let store = ds.object_store(None).await.unwrap(); + store + .read_dir_all(&ds.data_dir(), None) + .try_filter(|object| { + futures::future::ready(object.location.extension() == Some("blob")) + }) + .map_ok(|object| { + let parts = object.location.parts().collect::>(); + format!("{}.lance", parts[parts.len() - 2].as_ref()) + }) + .try_collect::>() + .await + .unwrap() + } + + async fn rows(ds: &Dataset) -> Vec<(String, String, Option>, u64)> { + let mut scanner = ds.scan(); + scanner.with_row_id(); + scanner.blob_handling(BlobHandling::AllBinary); + scanner.project(&["id", "name", "content"]).unwrap(); + let batch = scanner.try_into_batch().await.unwrap(); + let ids = batch.column_by_name("id").unwrap().as_string::(); + let names = batch.column_by_name("name").unwrap().as_string::(); + let contents = batch.column_by_name("content").unwrap().as_binary::(); + let row_ids = batch + .column_by_name(ROW_ID) + .unwrap() + .as_primitive::(); + (0..batch.num_rows()) + .map(|row| { + ( + ids.value(row).to_string(), + names.value(row).to_string(), + contents.is_valid(row).then(|| contents.value(row).to_vec()), + row_ids.value(row), + ) + }) + .collect() + } + + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().join("erase.lance"); + let uri = uri.to_str().unwrap(); + let blob_column = |name: &str, payloads: &[Option>]| { + let mut builder = BlobArrayBuilder::new(payloads.len()); + for payload in payloads { + match payload { + Some(payload) => builder.push_bytes(payload).unwrap(), + None => builder.push_null().unwrap(), + } + } + ( + lance::blob::blob_field(name, true), + builder.finish().unwrap(), + ) + }; + let ids = ["r0", "r1", "r2", "r3", "r4"]; + let kept: Vec>> = vec![ + Some(vec![b'k'; 80]), + Some(vec![b'K'; 96 * 1024]), + None, + Some(Vec::new()), + Some(vec![b'D'; 5 * 1024 * 1024]), + ]; + let dropped: Vec>> = vec![ + Some(vec![b'x'; 80]), + Some(vec![b'y'; 96 * 1024]), + Some(vec![b'z'; 5 * 1024 * 1024]), + None, + Some(Vec::new()), + ]; + let (content_field, content) = blob_column("content", &kept); + let (secret_field, secret) = blob_column("secret", &dropped); + let mut pk = HashMap::new(); + pk.insert(LANCE_UNENFORCED_PRIMARY_KEY.to_string(), "true".to_string()); + let schema = Arc::new(Schema::new(vec![ + Field::new("id", DataType::Utf8, false).with_metadata(pk), + Field::new("name", DataType::Utf8, true), + Field::new("note", DataType::Utf8, true), + content_field, + secret_field, + ])); + let batch = RecordBatch::try_new( + schema.clone(), + vec![ + Arc::new(StringArray::from(ids.to_vec())), + Arc::new(StringArray::from(vec!["a", "b", "c", "d", "e"])), + Arc::new(StringArray::from(vec!["n0", "n1", "n2", "n3", "n4"])), + content, + secret, + ], + ) + .unwrap(); + let mut base = Dataset::write( + RecordBatchIterator::new(vec![Ok(batch)], schema), + uri, + Some(WriteParams { + mode: WriteMode::Create, + enable_stable_row_ids: true, + data_storage_version: Some(LanceFileVersion::V2_2), + ..Default::default() + }), + ) + .await + .unwrap(); + base.create_index( + &["name"], + IndexType::Scalar, + None, + &ScalarIndexParams::default(), + false, + ) + .await + .unwrap(); + assert_eq!(base.get_fragments().len(), 1, "one fragment"); + + // No false positive: every id a data file names is a schema field id. + let schema_ids = base + .schema() + .field_ids() + .into_iter() + .collect::>(); + for (path, fields) in file_field_ids(&base) { + assert!( + fields.iter().all(|id| schema_ids.contains(id)), + "{path} names ids {fields:?} outside the schema's {schema_ids:?}" + ); + } + let note_id = base.schema().field("note").unwrap().id; + let secret_id = base.schema().field("secret").unwrap().id; + let files_before = file_field_ids(&base); + let sidecars_before = sidecar_parents(&base).await; + assert!( + !sidecars_before.is_empty(), + "the 5 MiB payloads must create dedicated Blob sidecars" + ); + let expected = rows(&base).await; + + // Drop `note` and `secret` with a detached Project: the data file stays, + // and its `fields` still names both dropped columns. + let mut projected = base.schema().clone(); + projected + .fields + .retain(|field| field.name != "note" && field.name != "secret"); + projected.validate().unwrap(); + let projected = CommitBuilder::new(Arc::new(base.clone())) + .with_detached(true) + .with_skip_auto_cleanup(true) + .execute(Transaction::new( + base.version().version, + Operation::Project { + schema: projected, + preserves_nullability: true, + }, + None, + )) + .await + .unwrap(); + assert_eq!( + file_field_ids(&projected), + files_before, + "a drop keeps the data file and its field ids" + ); + let live = projected + .schema() + .field_ids() + .into_iter() + .collect::>(); + let held = file_field_ids(&projected) + .into_iter() + .flat_map(|(_, fields)| fields) + .filter(|id| !live.contains(id)) + .collect::>(); + // A Blob-v2 column is named by its own id; its descriptor children are + // not listed. + assert_eq!( + held, + BTreeSet::from([note_id, secret_id]), + "the file still names exactly the dropped `note` and `secret`" + ); + + // Lance's planner does not select a fragment for its dropped columns, and + // compacts by re-encoding, never by copying pages. + let options = CompactionOptions::default(); + assert_eq!(options.compaction_mode(), CompactionMode::Reencode); + assert_eq!( + plan_compaction(&projected, &options) + .await + .unwrap() + .num_tasks(), + 0, + "the default planner leaves a lone fragment without deletions alone" + ); + + // A task naming the fragment rewrites it from the current schema. + let task = CompactionTask { + task: TaskData { + fragments: vec![projected.get_fragments()[0].metadata().clone()], + }, + read_version: projected.version().version, + options, + }; + let result = task.execute(&projected).await.unwrap(); + assert!(result.row_addrs.is_none(), "stable row ids are rechunked"); + let next_id = projected.manifest.max_fragment_id().map_or(0, |id| id + 1); + let mut new_fragments = result.new_fragments.clone(); + for (fragment, id) in new_fragments.iter_mut().zip(next_id..) { + fragment.id = id; + } + let compacted = CommitBuilder::new(Arc::new(projected.clone())) + .with_detached(true) + .with_skip_auto_cleanup(true) + .execute(Transaction::new( + projected.version().version, + Operation::Rewrite { + groups: vec![RewriteGroup { + old_fragments: result.original_fragments.clone(), + new_fragments, + }], + rewritten_indices: Vec::new(), + frag_reuse_index: None, + }, + None, + )) + .await + .unwrap(); + let files_after = file_field_ids(&compacted); + assert!( + files_after + .iter() + .all(|(path, _)| files_before.iter().all(|(old, _)| old != path)), + "the rewrite replaces the data file" + ); + for (path, fields) in &files_after { + assert!( + fields.iter().all(|id| live.contains(id)), + "{path} still names a dropped field id: {fields:?}" + ); + } + let sidecars_after = sidecar_parents(&compacted).await; + let new_sidecars = sidecars_after + .difference(&sidecars_before) + .cloned() + .collect::>(); + assert!( + !new_sidecars.is_empty() + && new_sidecars + .iter() + .all(|parent| files_after.iter().any(|(path, _)| path == parent)), + "the kept 5 MiB value moves to a sidecar of the new file: {new_sidecars:?}" + ); + let compacted_dir = std::path::Path::new(uri).join("data"); + let sidecar_bytes = new_sidecars + .iter() + .map(|parent| { + std::fs::read_dir(compacted_dir.join(parent.trim_end_matches(".lance"))) + .unwrap() + .map(|entry| entry.unwrap().metadata().unwrap().len()) + .sum::() + }) + .sum::(); + assert!( + sidecar_bytes < 6 * 1024 * 1024, + "only the kept 5 MiB value is carried, not the dropped one: {sidecar_bytes} bytes" + ); + assert_eq!(rows(&compacted).await, expected, "rows survive the rewrite"); + let name_index = compacted + .load_indices() + .await + .unwrap() + .iter() + .find(|index| index.name == "name_idx") + .cloned() + .expect("the scalar index survives"); + assert_eq!( + name_index + .fragment_bitmap + .as_ref() + .map(|bitmap| bitmap.iter().collect::>()), + Some( + compacted + .get_fragments() + .iter() + .map(|fragment| fragment.id() as u32) + .collect() + ), + "the index covers the rewritten fragment" + ); +} + /// Helper: build a small fresh dataset in a tempdir. Pinned at V2_2 to match /// production write paths (blob v2 requires V2_2; see `docs/dev/lance.md`). async fn fresh_dataset(uri: &str) -> Dataset { diff --git a/crates/omnigraph/tests/maintenance.rs b/crates/omnigraph/tests/maintenance.rs index 386fa16ed..381694efb 100644 --- a/crates/omnigraph/tests/maintenance.rs +++ b/crates/omnigraph/tests/maintenance.rs @@ -1164,6 +1164,261 @@ async fn optimize_does_not_size_a_blob_batch_by_external_references() { assert_eq!(doc.fragments_removed, 2); } +/// Deterministic bytes no encoder can shorten, so a window of them found on +/// disk is the stored value. +fn incompressible(seed: u64, len: usize) -> Vec { + let mut state = seed; + (0..len) + .map(|_| { + state = state + .wrapping_mul(6_364_136_223_846_793_005) + .wrapping_add(1_442_695_040_888_963_407); + (state >> 56) as u8 + }) + .collect() +} + +/// Every file under `root` whose bytes contain `needle`. +fn files_containing(root: &std::path::Path, needle: &[u8]) -> Vec { + let mut found = Vec::new(); + let mut pending = vec![root.to_path_buf()]; + while let Some(dir) = pending.pop() { + for entry in std::fs::read_dir(dir).unwrap() { + let path = entry.unwrap().path(); + if path.is_dir() { + pending.push(path); + } else if std::fs::read(&path) + .unwrap() + .windows(needle.len()) + .any(|window| window == needle) + { + found.push(path); + } + } + } + found +} + +/// The field ids each data file of a table's pinned version lists. +async fn pinned_file_field_ids(db: &Omnigraph, table_key: &str) -> Vec<(String, Vec)> { + helpers::open_pinned_dataset_for_test(db, "main", table_key) + .await + .get_fragments() + .iter() + .flat_map(|fragment| fragment.metadata().files.clone()) + .map(|file| (file.path.clone(), file.fields.to_vec())) + .collect() +} + +/// A drop is logical at apply and erased by maintenance: `optimize` rewrites +/// every fragment still holding a dropped property's values, here one +/// fragment without deletions that Lance's planner leaves alone, in one graph +/// commit with batches sized by the remaining Blob property, and `cleanup` +/// then deletes the old files once nothing retains them, so no file of the +/// graph holds the dropped bytes. A branch made after the drop and before the +/// optimize retains them, even once merged into main, until it is deleted. A +/// table with no dropped column and nothing to compact is neither sized nor +/// committed. Bytes on disk need filesystem assertions. +#[tokio::test] +async fn optimize_then_cleanup_erases_dropped_property_values() { + const MIB: usize = 1024 * 1024; + let dir = tempfile::tempdir().unwrap(); + let initial = "node Doc {\n slug: String @key\n note: String?\n content: Blob?\n secret: Blob?\n}\n\ + node Tag {\n slug: String @key\n icon: Blob?\n}\n"; + let db = helpers::session( + Omnigraph::init(dir.path().to_str().unwrap(), initial) + .await + .unwrap(), + ); + // Inline, packed and dedicated placements of the dropped Blob, and a + // 1 MiB kept value that sizes the rewrite's batches. + let secrets = [ + ("inline", Some(incompressible(1, 512))), + ("packed", Some(incompressible(2, 96 * 1024))), + ("dedicated", Some(incompressible(3, 5 * MIB))), + ("null", None), + ]; + let content = |slug: &str| match slug { + "dedicated" => Some(incompressible(4, MIB)), + "null" => None, + _ => Some(slug.as_bytes().to_vec()), + }; + let mut lines = secrets + .iter() + .map(|(slug, secret)| { + serde_json::json!({ + "type": "Doc", + "data": { + "slug": slug, + "note": format!("note of {slug}"), + "content": content(slug).as_deref().map(base64_blob), + "secret": secret.as_deref().map(base64_blob), + }, + }) + .to_string() + }) + .collect::>(); + lines.push( + serde_json::json!({"type": "Tag", "data": {"slug": "t", "icon": base64_blob(b"icon")}}) + .to_string(), + ); + db.load_jsonl(&lines.join("\n"), LoadMode::Overwrite) + .await + .unwrap(); + // Settle index work, then prove Lance's planner leaves both tables alone. + db.optimize().await.unwrap(); + for stat in db.optimize().await.unwrap() { + assert!(!stat.committed, "test precondition: {stat:?}"); + } + assert_eq!( + helpers::open_pinned_dataset_for_test(&db, "main", "node:Doc") + .await + .get_fragments() + .len(), + 1, + "test precondition: one Doc fragment" + ); + let dropped_ids = { + let doc = helpers::open_pinned_dataset_for_test(&db, "main", "node:Doc").await; + BTreeSet::from([ + doc.schema().field("note").unwrap().id, + doc.schema().field("secret").unwrap().id, + ]) + }; + let needles = secrets + .iter() + .filter_map(|(_, secret)| secret.as_ref().map(|bytes| bytes[64..96].to_vec())) + .collect::>(); + for needle in &needles { + assert!( + !files_containing(dir.path(), needle).is_empty(), + "test precondition: the dropped value is stored" + ); + } + + let desired = initial + .replace(" note: String?\n", "") + .replace(" secret: Blob?\n", ""); + assert!(db.apply_schema(&desired).await.unwrap().applied); + let files_after_drop = pinned_file_field_ids(&db, "node:Doc").await; + assert!( + files_after_drop + .iter() + .any(|(_, fields)| dropped_ids.iter().all(|id| fields.contains(id))), + "test precondition: the drop keeps the data file naming both dropped ids" + ); + // Its head and fork point pin the table version holding the dropped values. + db.branch_create("retained").await.unwrap(); + let tag_pin = helpers::pinned_version(&db, "main", "node:Tag").await; + let commits_before = db.list_commits(None).await.unwrap().len(); + + let probes = omnigraph::instrumentation::MergeWriteProbes::default(); + let stats = omnigraph::instrumentation::with_merge_write_probes(probes.clone(), db.optimize()) + .await + .unwrap(); + let stat = |key: &str| stats.iter().find(|stat| stat.type_key == key).unwrap(); + let doc = stat("node:Doc"); + assert!( + doc.committed, + "the fragment holding dropped values is rewritten" + ); + assert_eq!((doc.fragments_removed, doc.fragments_added), (1, 1)); + assert!(!stat("node:Tag").committed, "Tag has nothing to rewrite"); + assert_eq!( + helpers::pinned_version(&db, "main", "node:Tag").await, + tag_pin + ); + assert_eq!( + ( + probes.compaction_blob_batch_calls(), + probes.compaction_blob_batch_rows() + ), + (1, 32), + "one task, sized by the kept 1 MiB value, not the dropped 5 MiB one; Tag is not scanned" + ); + assert_eq!( + db.list_commits(None).await.unwrap().len(), + commits_before + 1, + "optimize publishes one graph commit" + ); + let live = helpers::open_pinned_dataset_for_test(&db, "main", "node:Doc") + .await + .schema() + .field_ids() + .into_iter() + .collect::>(); + for (path, fields) in pinned_file_field_ids(&db, "node:Doc").await { + assert!( + fields.iter().all(|id| live.contains(id)), + "{path} still names a dropped field id: {fields:?}" + ); + assert!(files_after_drop.iter().all(|(old, _)| *old != path)); + } + let mut expected = secrets + .iter() + .map(|(slug, _)| (slug.to_string(), vec![content(slug)])) + .collect::>(); + expected.sort(); + assert!( + doc_blob_values(&db, &["content"]).await == expected, + "the remaining properties survive the rewrite" + ); + for stat in db.optimize().await.unwrap() { + assert!( + !stat.committed, + "a second optimize has nothing left: {stat:?}" + ); + } + + // Older commits still pin the old files until cleanup stops retaining them. + for needle in &needles { + assert!(!files_containing(dir.path(), needle).is_empty()); + } + // Merging the branch does not release its pins, and neither does another + // optimize of main: cleanup keeps every live branch's head and fork point. + db.load( + "retained", + r#"{"type":"Tag","data":{"slug":"u","icon":"base64:dQ=="}}"#, + LoadMode::Merge, + ) + .await + .unwrap(); + assert_eq!( + db.branch_merge("retained", "main").await.unwrap().outcome, + MergeOutcome::Merged + ); + db.optimize().await.unwrap(); + let stats = db.cleanup(keep_one()).await.unwrap(); + assert!(stats.iter().all(|row| row.error.is_none()), "{stats:?}"); + for needle in &needles { + assert!( + !files_containing(dir.path(), needle).is_empty(), + "the live branch retains the dropped bytes" + ); + } + db.branch_delete("retained").await.unwrap(); + let stats = db.cleanup(keep_one()).await.unwrap(); + assert!(stats.iter().all(|row| row.error.is_none()), "{stats:?}"); + for needle in &needles { + let found = files_containing(dir.path(), needle); + assert!(found.is_empty(), "dropped bytes remain in {found:?}"); + } + let doc_dir = std::path::PathBuf::from(node_table_uri(&db, "Doc").await).join("data"); + for (old, _) in &files_after_drop { + assert!(!doc_dir.join(old).exists(), "old data file remains: {old}"); + // A local store leaves the emptied sidecar directory behind. + assert!( + std::fs::read_dir(doc_dir.join(old.trim_end_matches(".lance"))) + .map_or(true, |mut sidecars| sidecars.next().is_none()), + "old Blob sidecars remain: {old}" + ); + } + assert!( + doc_blob_values(&db, &["content"]).await == expected, + "the remaining properties survive cleanup" + ); +} + /// `optimize` publishes its compaction to `__manifest` as a detached pin with /// fewer fragments, leaves the linear HEAD where it was, and a schema apply /// on the compacted table then succeeds. diff --git a/crates/omnigraph/tests/schema_apply.rs b/crates/omnigraph/tests/schema_apply.rs index 3209149d5..92b18fae3 100644 --- a/crates/omnigraph/tests/schema_apply.rs +++ b/crates/omnigraph/tests/schema_apply.rs @@ -1,6 +1,7 @@ mod helpers; use base64::Engine; +use std::collections::HashMap; #[cfg(feature = "failpoints")] use std::sync::Arc; @@ -1454,8 +1455,8 @@ node Document { db.load_jsonl(&data, LoadMode::Overwrite).await.unwrap(); // Admission policy is not durable graph data. Reopen with the default - // deny policy so the rewrite proves that a historical descriptor is - // carried without re-authorizing or probing its caller-owned target. + // deny policy so the drop proves that a stored descriptor stays in place + // without re-authorizing or probing its caller-owned target. let db = Omnigraph::open(uri).await.unwrap(); let documents_before = count_rows(&db, "node:Document").await; @@ -1466,10 +1467,9 @@ node Document { .graph_manifest_version(); // Drop `note` from Document. v1 + chassis commit #3 emit - // `DropProperty`; the rewrite path projects to the - // target schema (no `note`), commits via stage_overwrite. Row - // counts are unchanged — only the column is dropped from the - // current schema view. + // `DropProperty`; a metadata-only Project removes the column from the + // table's schema and keeps every data file. Row counts are unchanged — + // only the column is dropped from the current schema view. let desired = initial.replace(" note: String?\n", ""); // Confirm the plan emits DropProperty (not UnsupportedChange). @@ -1487,10 +1487,10 @@ node Document { "expected DropProperty {{ type=Document, property=note }} in plan; got {plan:?}", ); - // An unrelated schema rewrite carries the descriptor, not the external - // payload. The caller-owned target may be unavailable without blocking - // schema evolution. + // The drop reads no row, so the caller-owned target of a stored external + // descriptor may be unavailable without blocking schema evolution. std::fs::remove_file(&external_path).unwrap(); + let files_before = data_file_paths(&db, "node:Document").await; let probes = omnigraph::instrumentation::MergeWriteProbes::default(); let result = omnigraph::instrumentation::with_merge_write_probes( probes.clone(), @@ -1500,10 +1500,12 @@ node Document { .unwrap(); assert!(result.supported); assert!(result.applied); - // Three managed values (valid empty, inline, packed) in one batched read. - assert_eq!(probes.blob_managed_batch_read_calls(), 1); - assert_eq!(probes.blob_payload_read_calls(), 3); + // Metadata-only: no managed or external payload is read, and every data + // file the table had before is still the table's data. + assert_eq!(probes.blob_managed_batch_read_calls(), 0); + assert_eq!(probes.blob_payload_read_calls(), 0); assert_eq!(probes.external_blob_payload_read_calls(), 0); + assert_eq!(data_file_paths(&db, "node:Document").await, files_before); assert_exact_id_primary_key(&db, "node:Document").await; // Manifest advanced; row count unchanged. @@ -1564,7 +1566,7 @@ node Document { assert_eq!(reference.length, None); } BlobContent::Managed { .. } => { - panic!("schema rewrite must preserve the external Blob descriptor") + panic!("schema apply must preserve the external Blob descriptor") } } @@ -1658,76 +1660,488 @@ node Document { ); } +/// Adding, renaming and dropping properties is metadata-only, on a Blob table +/// (rename + drop + adds: a Project then a Merge) and a plain table (rename + +/// drop: one Project): no row or payload is read, every data file stays the +/// table's data, field ids, stable property markers and index coverage +/// survive, the historical snapshot keeps its shape, one graph commit +/// publishes it, and keyed writes, queries and optimize work afterwards. #[tokio::test] -#[cfg(feature = "failpoints")] -#[serial_test::parallel] -async fn schema_apply_rejects_ranged_external_blob_before_arm_or_effects() { - use helpers::recovery::{branch_head_commit_id, sidecar_operation_ids}; +#[cfg_attr(feature = "failpoints", serial_test::parallel)] +async fn apply_schema_column_changes_keep_data_files_ids_and_index_coverage() { + use lance::index::DatasetIndexExt; + + const MARKER: &str = "omnigraph.stable_property_id"; + + async fn fields(db: &Omnigraph, table_key: &str) -> Vec<(String, i32, Option)> { + helpers::open_pinned_dataset_for_test(db, "main", table_key) + .await + .schema() + .fields + .iter() + .map(|field| { + ( + field.name.clone(), + field.id, + field.metadata.get(MARKER).cloned(), + ) + }) + .collect() + } + + async fn coverage(db: &Omnigraph, table_key: &str) -> Vec<(Vec, Vec)> { + let mut coverage = helpers::open_pinned_dataset_for_test(db, "main", table_key) + .await + .load_indices() + .await + .unwrap() + .iter() + .filter(|index| !lance_index::is_system_index(index)) + .map(|index| { + ( + index.fields.clone(), + index + .fragment_bitmap + .as_ref() + .map(|bitmap| bitmap.iter().collect()) + .unwrap_or_default(), + ) + }) + .collect::>(); + coverage.sort(); + coverage + } + + fn field<'a>( + fields: &'a [(String, i32, Option)], + name: &str, + ) -> &'a (String, i32, Option) { + fields + .iter() + .find(|(field, _, _)| field == name) + .unwrap_or_else(|| panic!("no field '{name}' in {fields:?}")) + } let dir = tempfile::tempdir().unwrap(); let uri = dir.path().to_str().unwrap(); let initial = r#" node Document { title: String @key + rank: I32? @index + note: String? content: Blob? } + +node Tag { + name: String @key + weight: I32? @index + color: String? +} "#; + let db = helpers::session(Omnigraph::init(uri, initial).await.unwrap()); + let packed = base64::engine::general_purpose::STANDARD.encode(vec![b'p'; 96 * 1024]); + let dedicated = base64::engine::general_purpose::STANDARD.encode(vec![b'd'; 5 * 1024 * 1024]); + let rows = [ + serde_json::json!({"type": "Document", "data": {"title": "inline", "rank": 1, "note": "n1", "content": "base64:SW5saW5l"}}), + serde_json::json!({"type": "Document", "data": {"title": "packed", "rank": 2, "note": "n2", "content": format!("base64:{packed}")}}), + serde_json::json!({"type": "Document", "data": {"title": "dedicated", "rank": 3, "note": "n3", "content": format!("base64:{dedicated}")}}), + serde_json::json!({"type": "Document", "data": {"title": "empty", "rank": 4, "note": "n4", "content": "base64:"}}), + serde_json::json!({"type": "Document", "data": {"title": "null", "rank": 5, "note": "n5", "content": null}}), + serde_json::json!({"type": "Tag", "data": {"name": "red", "weight": 7, "color": "crimson"}}), + serde_json::json!({"type": "Tag", "data": {"name": "blue", "weight": 9, "color": "navy"}}), + ] + .iter() + .map(|row| row.to_string()) + .collect::>() + .join("\n"); + db.load_jsonl(&rows, LoadMode::Overwrite).await.unwrap(); + db.ensure_indices().await.unwrap(); + + let before = db.snapshot_of(ReadTarget::branch("main")).await.unwrap(); + let before_snapshot = db.resolve_snapshot("main").await.unwrap(); + let mut before_state = HashMap::new(); + for table in ["node:Document", "node:Tag"] { + let coverage_before = coverage(&db, table).await; + assert!( + coverage_before.len() >= 2, + "{table} needs its id and property indexes: {coverage_before:?}" + ); + before_state.insert( + table, + ( + data_file_paths(&db, table).await, + fields(&db, table).await, + coverage_before, + ), + ); + } + let desired = r#" node Document { title: String @key + score: I32? @rename_from("rank") @index content: Blob? - note: String? + summary: String? + thumb: Blob? +} + +node Tag { + name: String @key + strength: I32? @rename_from("weight") @index } "#; - let db = Omnigraph::init(uri, initial).await.unwrap(); - let table_uri = helpers::seed_ranged_external_blob_row(&db, uri).await; + let probes = omnigraph::instrumentation::MergeWriteProbes::default(); + let result = omnigraph::instrumentation::with_merge_write_probes( + probes.clone(), + db.apply_schema(desired), + ) + .await + .unwrap(); + assert!(result.applied); + assert_eq!(probes.blob_managed_batch_read_calls(), 0); + assert_eq!(probes.blob_payload_read_calls(), 0); + assert_eq!(probes.external_blob_payload_read_calls(), 0); + let after = db.snapshot_of(ReadTarget::branch("main")).await.unwrap(); + assert_eq!( + after.graph_manifest_version(), + before.graph_manifest_version() + 1, + "one graph commit publishes every table's evolution and the contract" + ); + for table in ["node:Document", "node:Tag"] { + let entry = after.dataset(table).unwrap(); + assert_eq!( + entry.published_dataset_version, + before.dataset(table).unwrap().published_dataset_version + 1 + ); + assert_eq!( + entry.entity_count, + before.dataset(table).unwrap().entity_count + ); + } - let before = db.snapshot_of(ReadTarget::branch("main")).await.unwrap(); - let manifest_before = before.graph_manifest_version(); - let table_before = before - .dataset("node:Document") - .unwrap() - .published_dataset_version; - let physical_head_before = lance::Dataset::open(&table_uri) + for (table, renamed, from, dropped) in [ + ("node:Document", "score", "rank", "note"), + ("node:Tag", "strength", "weight", "color"), + ] { + let (files_before, fields_before, coverage_before) = &before_state[table]; + assert_eq!( + &data_file_paths(&db, table).await, + files_before, + "{table}: no data file is written or dropped" + ); + let fields_after = fields(&db, table).await; + let (_, from_id, from_marker) = field(fields_before, from); + let (_, renamed_id, renamed_marker) = field(&fields_after, renamed); + assert_eq!(renamed_id, from_id, "{table}: a rename keeps the field id"); + assert_eq!( + renamed_marker, from_marker, + "{table}: and the property identity" + ); + assert!(from_marker.is_some()); + assert!(fields_after.iter().all(|(name, _, _)| name != dropped)); + for (name, id, marker) in fields_before { + if name != from && name != dropped { + assert_eq!( + field(&fields_after, name), + &(name.clone(), *id, marker.clone()), + "{table}: an untouched column keeps its id and identity" + ); + } + } + assert_eq!( + &coverage(&db, table).await, + coverage_before, + "{table}: every index keeps its field and fragment coverage" + ); + } + for table in ["node:Document", "node:Tag"] { + helpers::assert_stable_property_markers(&db, table).await; + assert_exact_id_primary_key(&db, table).await; + } + let document_fields = fields(&db, "node:Document").await; + let max_before = before_state["node:Document"] + .1 + .iter() + .map(|(_, id, _)| *id) + .max() + .unwrap(); + for added in ["summary", "thumb"] { + let (_, id, marker) = field(&document_fields, added); + assert!(*id > max_before, "{added} takes a fresh field id"); + assert!(marker.is_some(), "{added} carries its property identity"); + } + assert_eq!( + document_fields + .iter() + .map(|(name, _, _)| name.as_str()) + .collect::>(), + ["__id", "content", "score", "summary", "thumb", "title"], + "the physical column order follows the catalog, which orders properties by name" + ); + + // Payloads read back unchanged; the added Blob reads as null. + for (title, expected) in [ + ("inline", b"Inline".to_vec()), + ("packed", vec![b'p'; 96 * 1024]), + ("dedicated", vec![b'd'; 5 * 1024 * 1024]), + ("empty", Vec::new()), + ] { + let bytes = read_managed_blob_bytes( + &db, + ReadTarget::branch("main"), + node_blob_cell("Document", title, "content"), + ) + .await; + assert!(bytes == expected, "{title}: payload changed"); + } + for (title, property) in [("null", "content"), ("inline", "thumb")] { + let error = db + .read_blob_at( + ReadTarget::branch("main"), + node_blob_cell("Document", title, property), + ) + .await + .unwrap_err(); + assert!( + matches!(error, OmniError::Manifest(ref error) if error.kind == ManifestErrorKind::NotFound), + "{title}.{property} reads as null, got {error:?}" + ); + } + + // The pre-apply snapshot keeps its own shape and values. + let historical = db + .snapshot_at_graph_manifest_version(before.graph_manifest_version()) .await .unwrap() - .version() - .version; - let lineage_before = branch_head_commit_id(dir.path(), "main").await.unwrap(); - assert!(sidecar_operation_ids(dir.path()).is_empty()); + .open_dataset("node:Document") + .await + .unwrap(); + let historical_names = historical + .schema() + .fields + .iter() + .map(|field| field.name.clone()) + .collect::>(); + assert!(historical_names.contains(&"note".to_string())); + assert!(historical_names.contains(&"rank".to_string())); + assert!(!historical_names.contains(&"score".to_string())); + assert_eq!( + read_managed_blob_bytes( + &db, + ReadTarget::snapshot(before_snapshot), + node_blob_cell("Document", "packed", "content"), + ) + .await, + vec![b'p'; 96 * 1024] + ); - let error = db.apply_schema(desired).await.unwrap_err(); - assert!( - error - .to_string() - .contains("cannot preserve ranged external Blob descriptor"), - "unexpected schema-apply refusal: {error}" + // Keyed writes, filtered queries and optimize work on the evolved tables. + db.load_jsonl( + &serde_json::json!({ + "type": "Document", + "data": {"title": "new", "score": 6, "summary": "fresh", "thumb": "base64:VGh1bWI="}, + }) + .to_string(), + LoadMode::Merge, + ) + .await + .unwrap(); + let writes = r#" +query set_summary($title: String, $summary: String) { + update Document set { summary: $summary } where title = $title +} + +query set_strength($name: String, $strength: I32) { + update Tag set { strength: $strength } where name = $name +} +"#; + mutate_main( + &db, + writes, + "set_summary", + ¶ms(&[("$title", "dedicated"), ("$summary", "big")]), + ) + .await + .unwrap(); + mutate_main( + &db, + writes, + "set_strength", + &mixed_params(&[("$name", "red")], &[("$strength", 8)]), + ) + .await + .unwrap(); + let reads = r#" +query scored($min: I32) { + match { + $d: Document + $d.score >= $min + } + return { $d.title } +} + +query summarized() { + match { + $d: Document + $d.summary != "" + } + return { $d.title } +} + +query strong($min: I32) { + match { + $t: Tag + $t.strength >= $min + } + return { $t.name } +} +"#; + let scored = query_main(&db, reads, "scored", &int_params(&[("$min", 3)])) + .await + .unwrap(); + assert_eq!( + first_column_sorted(&scored), + ["dedicated", "empty", "new", "null"] ); - let after = db.snapshot_of(ReadTarget::branch("main")).await.unwrap(); - assert_eq!(after.graph_manifest_version(), manifest_before); + let summarized = query_main(&db, reads, "summarized", ¶ms(&[])) + .await + .unwrap(); + assert_eq!(first_column_sorted(&summarized), ["dedicated", "new"]); + let strong = query_main(&db, reads, "strong", &int_params(&[("$min", 8)])) + .await + .unwrap(); + assert_eq!(first_column_sorted(&strong), ["blue", "red"]); assert_eq!( - after - .dataset("node:Document") - .unwrap() - .published_dataset_version, - table_before + read_managed_blob_bytes( + &db, + ReadTarget::branch("main"), + node_blob_cell("Document", "new", "thumb"), + ) + .await, + b"Thumb" ); assert_eq!( - lance::Dataset::open(&table_uri) - .await - .unwrap() - .version() - .version, - physical_head_before + read_managed_blob_bytes( + &db, + ReadTarget::branch("main"), + node_blob_cell("Document", "dedicated", "content"), + ) + .await, + vec![b'd'; 5 * 1024 * 1024], + "an update carries the untouched Blob of an evolved row" ); + + db.optimize().await.unwrap(); assert_eq!( - branch_head_commit_id(dir.path(), "main").await.unwrap(), - lineage_before + read_managed_blob_bytes( + &db, + ReadTarget::branch("main"), + node_blob_cell("Document", "packed", "content"), + ) + .await, + vec![b'p'; 96 * 1024] ); - assert!( - sidecar_operation_ids(dir.path()).is_empty(), - "ranged descriptor refusal must occur before recovery arm" + let scored = query_main(&db, reads, "scored", &int_params(&[("$min", 3)])) + .await + .unwrap(); + assert_eq!( + first_column_sorted(&scored), + ["dedicated", "empty", "new", "null"] ); + let exported = db + .export_jsonl("main", &["Document".to_string()]) + .await + .unwrap(); + assert_eq!(exported.lines().count(), 6); + assert!(exported.contains("\"score\":3"), "{exported}"); + assert!(!exported.contains("\"rank\"") && !exported.contains("\"note\"")); +} + +/// A ranged external Blob descriptor survives a schema change on its table: +/// the column changes are metadata-only, so the stored descriptor is never +/// rebuilt and cannot be widened to its whole object. +#[tokio::test] +#[cfg(feature = "failpoints")] +#[serial_test::parallel] +async fn schema_apply_preserves_ranged_external_blob_across_column_changes() { + async fn assert_ranged(db: &Omnigraph, target: ReadTarget, property: &str) { + let read = db + .read_blob_at(target, node_blob_cell("Document", "ranged", property)) + .await + .unwrap(); + match read.content { + BlobContent::External(reference) => { + assert_eq!(reference.uri, "s3://bucket/object"); + assert_eq!((reference.offset, reference.length), (4, Some(8))); + } + BlobContent::Managed { .. } => panic!("the ranged descriptor must stay external"), + } + } + + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap(); + let initial = r#" +node Document { + title: String @key + content: Blob? +} +"#; + let added = r#" +node Document { + title: String @key + content: Blob? + note: String? +} +"#; + let renamed = r#" +node Document { + title: String @key + body: Blob? @rename_from("content") +} +"#; + let db = Omnigraph::init(uri, initial).await.unwrap(); + helpers::seed_ranged_external_blob_row(&db, uri).await; + let before = db.resolve_snapshot("main").await.unwrap(); + let files_before = data_file_paths(&db, "node:Document").await; + + let probes = omnigraph::instrumentation::MergeWriteProbes::default(); + let apply = |desired: &'static str| { + omnigraph::instrumentation::with_merge_write_probes( + probes.clone(), + db.apply_schema(desired), + ) + }; + assert!(apply(added).await.unwrap().applied); + assert_ranged(&db, ReadTarget::branch("main"), "content").await; + // The snapshot before the add reads the same cell under the same name. + assert_ranged(&db, ReadTarget::snapshot(before), "content").await; + + assert!(apply(renamed).await.unwrap().applied); + assert_ranged(&db, ReadTarget::branch("main"), "body").await; + + assert_eq!(probes.blob_payload_read_calls(), 0); + assert_eq!(probes.external_blob_payload_read_calls(), 0); + assert_eq!(data_file_paths(&db, "node:Document").await, files_before); +} + +/// The data files of a table's pinned version, per fragment id. +async fn data_file_paths(db: &Omnigraph, table_key: &str) -> Vec<(u64, Vec)> { + helpers::open_pinned_dataset_for_test(db, "main", table_key) + .await + .get_fragments() + .iter() + .map(|fragment| { + ( + fragment.id() as u64, + fragment + .metadata() + .files + .iter() + .map(|file| file.path.clone()) + .collect(), + ) + }) + .collect() } #[tokio::test] @@ -2471,7 +2885,10 @@ node Anchor { name: String @key } // lived) stays pinned by the older `__manifest` versions, so // `snapshot_at_graph_manifest_version(pre_drop)` still reads it. It becomes // unreachable once `omnigraph cleanup --keep 1` stops retaining those versions -// and the collector reclaims its files. +// and the collector reclaims its manifest. The dropped values stay in the +// current data files until optimize rewrites them; +// `maintenance.rs::optimize_then_cleanup_erases_dropped_property_values` owns +// that erasure. #[tokio::test] #[cfg_attr(feature = "failpoints", serial_test::parallel)] @@ -2487,7 +2904,7 @@ async fn apply_schema_property_drop_is_reclaimed_by_cleanup_not_apply() { .unwrap() .graph_manifest_version(); - // Drop the `age` column. Apply rewrites the table without it. + // Drop the `age` column, a metadata-only change of the table. let desired = TEST_SCHEMA.replace(" age: I32?\n", ""); let result = db.apply_schema(&desired).await.unwrap(); assert!(result.applied); diff --git a/docs/dev/blob.md b/docs/dev/blob.md index e19d5647b..a4dc4d4f1 100644 --- a/docs/dev/blob.md +++ b/docs/dev/blob.md @@ -62,11 +62,13 @@ Storage mode determines ownership: An existing stored external reference remains classifiable even if current ingress policy would reject creating it, and exportable when it names its whole object (below). OmniGraph never deletes the referenced object. +Schema apply never materializes a Blob. Adding, renaming or dropping a property, Blob or not, is a metadata-only commit that keeps every data file (see [writes.md](writes.md#mutation-and-load)): managed payloads, null and valid-empty cells and external descriptors, ranged ones included, stay exactly as stored, and an added Blob property reads as null in every existing row. A dropped Blob property's managed bytes stay with their data files until the next `optimize`, which rewrites every fragment still holding a dropped column without reading it; `cleanup` then deletes the old data files and their sidecars once no retained version names them. + A null assigned to a nullable Blob clears the cell; `resolve_assignments` refuses a null on a non-nullable one before the update opens its table. An update omits the Blob columns it assigns from its materializing scan (`scan_with_pending_materialized_blobs`'s `omit_blob_columns`): their old cells are never taken, authorized, probed, read, or charged to the byte budget. Every other Blob cell of a matched row is carried, which means reading it and rewriting it as managed bytes, because the keyed writer has no external-reference option. Carrying a stored external reference therefore passes the graph's policy; a refusal on that path is `OmniError::StoredExternalBlobDenied`, naming the type, id and property, distinct from `ExternalBlobPolicy`, which refuses caller input. Merge and new-input admission keep `ExternalBlobPolicy`. -A materializing rewrite (a carried update or merge cell, the bounded rewrite stream, a schema rewrite) reads the managed cells of one batch through `TableStore::managed_blob_payloads`, one `Dataset::read_blobs` call, never one `BlobFile::read` per row. It passes managed row ids only, because given an external row Lance resolves and reads the referenced object itself; external cells go through the preflight (keyed writes) or are carried as descriptors (schema rewrite). The read is streamed (`try_into_stream`, never `execute`) with an explicit `BLOB_READ_IO_BUFFER_BYTES` buffer, since Lance's default is 32 MiB times the store's I/O parallelism, and each value is checked against its descriptor length as it is consumed. Export and the change-feed baseline read the same way through `BlobRowReader`: it classifies every Blob cell of a batch from its descriptor first, then opens one such read per Blob column holding a managed cell and takes its values row by row as the rows render, so one row's values and one I/O buffer per column are resident, never the batch's payloads. Change-feed images, the change comparator's payload tie-break and entity reads use a one-row reader: the change feed admits one change at a time against its page budget, and its continuation sentinel reads no payload. Only the single-cell read facade keeps `take_blobs`, for bounded ranges of one stable row id. +A materializing rewrite (a carried update or merge cell, the bounded rewrite stream) reads the managed cells of one batch through `TableStore::managed_blob_payloads`, one `Dataset::read_blobs` call, never one `BlobFile::read` per row. It passes managed row ids only, because given an external row Lance resolves and reads the referenced object itself; external cells go through the preflight (keyed writes). The read is streamed (`try_into_stream`, never `execute`) with an explicit `BLOB_READ_IO_BUFFER_BYTES` buffer, since Lance's default is 32 MiB times the store's I/O parallelism, and each value is checked against its descriptor length as it is consumed. Export and the change-feed baseline read the same way through `BlobRowReader`: it classifies every Blob cell of a batch from its descriptor first, then opens one such read per Blob column holding a managed cell and takes its values row by row as the rows render, so one row's values and one I/O buffer per column are resident, never the batch's payloads. Change-feed images, the change comparator's payload tie-break and entity reads use a one-row reader: the change feed admits one change at a time against its page budget, and its continuation sentinel reads no payload. Only the single-cell read facade keeps `take_blobs`, for bounded ranges of one stable row id. -A whole-object surface cannot carry a ranged external descriptor: `ExternalBlobRef::whole_object_uri` returns a `RangedExternalBlob` (whose display never includes the URI) for it, and the HTTP redirect, CLI delivery, schema rewrite and export all refuse it rather than widen it. Surfaces that are not reloaded describe it instead: `BlobRowReader` takes `RangedExternalBlobs::{Refuse, Describe}` and returns `LogicalBlobValue::RangedExternal` under `Describe`, which `describe_ranged_blob_cells` writes as `{"uri", "offset", "length"}` with a positive length (the decoder below guarantees one; a missing length there is an internal error) into the row images of `logical_row_image` (change-feed images, entity reads) and the snapshot rows of the change-feed baseline, and change-row comparison compares exactly, so a ranged row never wedges a feed cursor or refuses a baseline. The baseline shares export's table walk and line format and passes `Describe` where export passes `Refuse`: it is the exact state its consumer starts from, and a refusal there would leave the graph with no baseline. The descriptor decoder refuses an external descriptor with an offset but size 0 as a Blob integrity error: Lance reads size 0 as the object's size but keeps the position, which would read past the object's end. +A whole-object surface cannot carry a ranged external descriptor: `ExternalBlobRef::whole_object_uri` returns a `RangedExternalBlob` (whose display never includes the URI) for it, and the HTTP redirect, CLI delivery and export all refuse it rather than widen it. Surfaces that are not reloaded describe it instead: `BlobRowReader` takes `RangedExternalBlobs::{Refuse, Describe}` and returns `LogicalBlobValue::RangedExternal` under `Describe`, which `describe_ranged_blob_cells` writes as `{"uri", "offset", "length"}` with a positive length (the decoder below guarantees one; a missing length there is an internal error) into the row images of `logical_row_image` (change-feed images, entity reads) and the snapshot rows of the change-feed baseline, and change-row comparison compares exactly, so a ranged row never wedges a feed cursor or refuses a baseline. The baseline shares export's table walk and line format and passes `Describe` where export passes `Refuse`: it is the exact state its consumer starts from, and a refusal there would leave the graph with no baseline. The descriptor decoder refuses an external descriptor with an offset but size 0 as a Blob integrity error: Lance reads size 0 as the object's size but keeps the position, which would read past the object's end. ## HTTP and CLI delivery diff --git a/docs/dev/lance.md b/docs/dev/lance.md index 2ebefed14..3a610ab50 100644 --- a/docs/dev/lance.md +++ b/docs/dev/lance.md @@ -169,7 +169,9 @@ history of dependency bumps. | KNN probe budget | A `nearest` scan sets `maximum_nprobes` per index delta and widens it from Lance's execution summary (`partitions_searched` / `partitions_ranked`, read through `scan_stats_callback`); a prefilter admitting fewer rows than `k` makes Lance emit the unreached rows at `_distance = +inf`, which the engine resolves with a flat exact rescan (`use_index(false)`); `count_rows(None)` is the live row count (deletions excluded), read for the ladder's exhaustion stop and as the overfetch loop's exact-pass `k`. A renamed counter or marker turns the ladder fail-closed. | `lance_surface_guards.rs`, `search.rs` | | Full-text analyzer | Every selected FTS segment needs artifact-scoped compatibility proof. Explicit branch rebuilds replace all segments from rows; old snapshots may refuse FTS. | staged-table tests, `search.rs`, `maintenance.rs` | | Blob v2 | Null, valid empty, non-empty, selector cardinality, neighboring bytes, and 3→1 compaction are pinned on Lance 11. A filtered scan that projects Blob-v2 descriptors is pinned before and after compaction: the single-cell read shape (`read_blob_at`: one descriptor column, an `id` equality filter through a BTREE plus an unindexed tail, stable row ids) and a predicate on the Blob column itself under every `BlobHandling`. Upstream fixed the projection assertion this shape once tripped (lance#7707, fixed by lance#7664 before Lance 9); OmniGraph carries no workaround or fallback, so a red guard blocks the bump. | `lance_surface_guards.rs` (`filtered_scan_projects_blob_descriptors_on_indexed_multi_fragment_table`), `maintenance.rs` | -| Blob batch reads | Every complete managed Blob read goes through `TableStore::managed_blob_payloads`, the one `Dataset::read_blobs(column).with_row_ids(..).preserve_order(true)` call, and consumes `try_into_stream`, never `execute`. Materializing rewrites (`TableStore::rebuild_blob_column`, schema apply's `rebuild_blob_column`) take one batch's values; export and the change-feed baseline (`BlobRowReader`) open one call per Blob column per batch of rows and take its values row by row as they render; change-feed images, the change comparator's payload tie-break and entity reads open a one-row reader. Callers pass managed row ids only: given an external row, `read_blobs` opens and reads the referenced object. Every call sets `with_io_buffer_size_bytes(BLOB_READ_IO_BUFFER_BYTES)` (8 MiB); Lance's default is `SchedulerConfig::max_bandwidth`, 32 MiB times the store's I/O parallelism (2 GiB at the cloud default of 64). Lance plans the reads in request-order batches no larger than that buffer (a larger value alone) and reads a batch only when the stream reaches it, so a reader that holds a stream across rows keeps one batch per column resident. The stream yields one `ReadBlob` per requested id in order, with duplicates, `None` for null and empty bytes for a valid empty value; the engine checks each length against the descriptor. Only the single-cell read facade keeps `take_blobs`, for bounded ranges of one value. | `lance_surface_guards.rs::compact_files_succeeds_on_blob_columns` (Guard 10: every managed placement, small-buffer stream equals `execute`, a taken value leaves the next batch unread, external rows are read), `forbidden_apis.rs::lance_batched_blob_read_call_counts_are_pinned` (one production `read_blobs` and one `with_io_buffer_size_bytes` call, both in `table_store.rs`; the helper's callers in `table_store.rs`, `schema_apply.rs` and `export.rs`; `take_blobs` in `blob.rs` alone; no `read_blob_ranges`), `export.rs::export_reads_each_batch_of_managed_blobs_in_one_read_per_column` | +| Blob batch reads | Every complete managed Blob read goes through `TableStore::managed_blob_payloads`, the one `Dataset::read_blobs(column).with_row_ids(..).preserve_order(true)` call, and consumes `try_into_stream`, never `execute`. Materializing rewrites (`TableStore::rebuild_blob_column`) take one batch's values; export and the change-feed baseline (`BlobRowReader`) open one call per Blob column per batch of rows and take its values row by row as they render; change-feed images, the change comparator's payload tie-break and entity reads open a one-row reader. Callers pass managed row ids only: given an external row, `read_blobs` opens and reads the referenced object. Every call sets `with_io_buffer_size_bytes(BLOB_READ_IO_BUFFER_BYTES)` (8 MiB); Lance's default is `SchedulerConfig::max_bandwidth`, 32 MiB times the store's I/O parallelism (2 GiB at the cloud default of 64). Lance plans the reads in request-order batches no larger than that buffer (a larger value alone) and reads a batch only when the stream reaches it, so a reader that holds a stream across rows keeps one batch per column resident. The stream yields one `ReadBlob` per requested id in order, with duplicates, `None` for null and empty bytes for a valid empty value; the engine checks each length against the descriptor. Only the single-cell read facade keeps `take_blobs`, for bounded ranges of one value. | `lance_surface_guards.rs::compact_files_succeeds_on_blob_columns` (Guard 10: every managed placement, small-buffer stream equals `execute`, a taken value leaves the next batch unread, external rows are read), `forbidden_apis.rs::lance_batched_blob_read_call_counts_are_pinned` (one production `read_blobs` and one `with_io_buffer_size_bytes` call, both in `table_store.rs`; the helper's callers in `table_store.rs` and `export.rs`; `take_blobs` in `blob.rs` alone; no `read_blob_ranges`), `export.rs::export_reads_each_batch_of_managed_blobs_in_one_read_per_column` | +| Schema evolution | Schema apply changes an existing table's columns without writing a data file (`TableStore::stage_schema_evolution`). Renames and drops commit as `Operation::Project` over the same field ids (Lance's own `alter_columns` rename and `drop_columns`); additions commit as `Operation::Merge` over the unchanged fragments with nullable fields whose ids exceed `Manifest::max_field_id` (Lance's own all-null `add_columns`). Lance 11 validates detached commits too, and its `merge_schema_valid` refuses a Merge that moves a field id to another path, so a table with renames or drops and additions chains the Merge on the Project. Lance 11 reads a field that no data file of a fragment holds as null (`NullReader`) and skips a data file holding no projected field; Project drops a data file whose every field is dropped; `retain_relevant_indices` keeps each index whose fields survive, with its fragment bitmap, and removes an index on a dropped field. Merge stamps no row version for fragments whose files are unchanged. Keyed writes send batches in catalog column order, and Lance 11's merge-insert matches a source to the full table schema only in the same field order (otherwise it takes its subschema path), so the evolved schema keeps the catalog order. A dropped column's values stay in the data files until optimize rewrites the fragment (dropped-column erasure, next row). | `lance_surface_guards.rs::metadata_only_schema_evolution_keeps_blob_files_ids_and_index_coverage`, `schema_apply.rs`, the deferred `benchmarks/deferred/crates/omnigraph/tests/compaction_memory.rs.disabled` schema-apply instrument (not an active test) | +| Dropped-column erasure | A drop's `Operation::Project` keeps every data file that also holds a surviving column, and that file's `DataFile::fields` still lists the dropped field id. A Blob-v2 column is listed by its own id, never by its descriptor children's, so on a table without drops every listed id is a schema field id. Lance's default planner selects fragments by size, deletions and overlay count, never by dropped columns, and leaves a lone fragment without deletions alone. `TableStore::plan_table_compaction`, the one plan behind optimize's planning gate and `stage_compaction`, appends a single-fragment `TaskData` (public, as is `CompactionPlan::tasks`) for every other fragment whose data or overlay files list a non-negative id outside the schema. Lance's planner applies its per-run source budgets (`max_source_fragments`, `max_source_rows`, `max_source_bytes`, in `limit_tasks_to_source_budget`) to the tasks it returns, so the appended tasks would fall outside them; the engine sets no budget, and `plan_table_compaction` refuses options that set one. A single-fragment group is the shape Lance plans for deletion materialization, and `Rewrite` replaces it in place. Compaction re-encodes by default (`CompactionMode::Reencode`; binary copy would carry pages) and scans the current schema, so the new files list only current field ids and only the kept Blob values get new sidecars; the task is sized like any other, from the kept Blob columns alone. `cleanup` then sweeps the old files with their sidecars once no retained version names them. | `lance_surface_guards.rs::compacting_a_chosen_fragment_drops_the_bytes_of_dropped_columns`, `maintenance.rs::optimize_then_cleanup_erases_dropped_property_values`, `staged_tests.rs::table_compaction_planning_refuses_source_budgets`, the deferred `benchmarks/deferred/crates/omnigraph/tests/compaction_memory.rs.disabled` erasure instrument (not an active test) | | Compaction of Blob tables | Lance 11 compaction cannot binary-copy a Blob table; it rewrites through a scanner and materializes every managed Blob-v2 payload of one scanner batch (external descriptors are carried unread). A batch never crosses a fragment, so at the default batch size one batch can hold a whole fragment's payloads. `TableStore::stage_compaction` executes the plan's tasks itself and sets each task's `CompactionTask::options.batch_size` from a descriptor-only scan of that task's fragments: the largest row's managed Blob bytes, summed over its Blob columns, with external descriptors counting nothing (`compaction_blob_batch_rows`: a 32 MiB budget, 1 to 8192 rows, so a row over 16 MiB puts its whole task on one-row batches; a nested or non-Blob-v2 Blob field fails closed to one row). Every task of a Blob table gets the derived size explicitly, capped by a caller's smaller one, so it takes precedence over `LANCE_DEFAULT_BATCH_SIZE`, including when the variable is smaller; all tasks are sized before any executes, so when a fragment being compacted holds a descriptor the decoder refuses, sizing can refuse the compaction with a Blob integrity error before that table is rewritten, and nothing is committed; a table with nothing to compact is not scanned. Lance reads a task's options into its scanner and ignores the batch size when planning. A set batch size beats `LANCE_DEFAULT_BATCH_SIZE` on the V2.x read path, where the variable is only the fallback. The fence is the scanner shape: a scanner built as compaction builds it honors an explicit batch size over the variable. No CI-run test observes Lance passing `CompactionOptions::batch_size` into that scanner; the historical `compaction_memory.rs` instrument was the only observer and is now deferred under `benchmarks/deferred/` pending GQ optimization support. Drop the derivation at the bump whose compaction streams Blob payloads. | `lance_surface_guards.rs::explicit_scanner_batch_size_beats_lance_default_batch_size_env` (Guard 10b: the compaction scanner shape honors an explicit batch size over the variable), `maintenance.rs::optimize_compacts_large_blob_rows_in_bounded_batches`, `maintenance.rs::optimize_sizes_a_blob_batch_by_the_sum_of_a_rows_blob_columns`, `maintenance.rs::optimize_sizes_each_compaction_task_from_its_own_fragments`, `maintenance.rs::optimize_does_not_size_a_blob_batch_by_external_references`, `maintenance.rs::optimize_compacts_blob_fragments_with_deleted_rows`, `staged_tests.rs::compaction_blob_batch_rows_bounds_one_batch`, the deferred `benchmarks/deferred/crates/omnigraph/tests/compaction_memory.rs.disabled` instrument (not an active test) | | Index coverage | Indexes are derived. Rewrites and compaction may leave an uncovered tail; reads must combine indexed and scan paths until explicit reconciliation. | `scalar_indexes.rs`, `search.rs`, `maintenance.rs` | | Inherited index files | An engine `CommitHandler` adapter repairs stock Lance's nested-clone index origins within the existing clone commit. Exact source metadata preserves inherited `Some(base_id)` values and assigns the immediate-source base only to source-local indexes. External fragment-reuse details are read from their original base and converted to validated inline details. Invalid details fail closed before native creation. | `storage_layer::lance_clone` tests; `lance_surface_guards.rs::stock_lance_nested_clone_overwrites_inherited_index_base_issue_7840` | diff --git a/docs/dev/writes.md b/docs/dev/writes.md index da820d2ca..91b45bc50 100644 --- a/docs/dev/writes.md +++ b/docs/dev/writes.md @@ -144,10 +144,10 @@ physical-effect proofs: | Writer | Physical adapter | Publication | |---|---|---| | Mutation / Load | One exact staged keyed, overwrite, or delete transaction per touched table | One graph commit | -| SchemaApply | Existing-table detached rewrites; original added-type Create or qualified reuse/detached replacement; complete table delta and replacement schema-contract row | One main-branch graph commit | +| SchemaApply | Existing-table metadata-only schema evolution (a detached `Project` for renamed and dropped columns, then a detached `Merge` adding nullable columns over the unchanged fragments; no data file written, no row read); original added-type Create or qualified reuse/detached replacement; complete table delta and replacement schema-contract row | One main-branch graph commit | | BranchMerge | Onto main: a pointer switch, main's registration taking the source's pin. Into a named branch: a chain of detached chunk commits (proven insertion chain or bounded ordered diff) published as one pin per table (`exec/merge.rs`) | One target-branch graph commit | | EnsureIndices / full-text rebuild | One detached `CreateIndex` batch per productive table, published as a pin like a mutation's effect (RFC 0067); ordinary ensure leaves untrainable vector work pending, explicit FTS rebuild replaces postings from rows | One graph publication when work lands | -| Optimize | One detached compaction `Rewrite` per productive table, chained with a detached whole rebuild of each index whose coverage lags and a detached build of each declared-but-unbuilt index, published as pins (RFC 0067) | One main-branch graph commit with an exact CAS on the pins the batch was planned from | +| Optimize | One detached compaction `Rewrite` per productive table (Lance's plan plus every fragment still holding a dropped column), chained with a detached whole rebuild of each index whose coverage lags and a detached build of each declared-but-unbuilt index, published as pins (RFC 0067) | One main-branch graph commit with an exact CAS on the pins the batch was planned from | Native graph-branch create/delete is a control exception. `BranchContents` is the logical authority; clone/delete residue is derived physical state and is @@ -208,17 +208,22 @@ still does, or when no `__manifest` version ever named it and the authority its transaction properties record is provably gone. Nothing defers a table's collection; see [Maintenance](../user/operations/maintenance.md#cleanup). -Optimize plans each table's compaction from its pin and stages the rewrite -detached with fragment ids above the base's high-water mark, so it needs no -`ReserveFragments`; a lagging scalar or vector index is rebuilt whole as a -detached commit chained on the rewrite under its name (Lance 11 folds only -through a linear commit), keeping a vector index's partition count; the batch -publishes once with an exact CAS on every planned pin, and a pin a concurrent -writer moved fails the run with a read-set conflict so the next run re-plans. -A failure before publication leaves unpublished staging the collector -reclaims once it is dead. A strict mutation prepared before Optimize's -publication reports the same read-set conflict as it would after any other -writer. +Optimize plans each table's compaction from its pin +(`TableStore::plan_table_compaction`): Lance's plan, plus a task of its own +for every other fragment whose data or overlay files still list a field id the +schema lacks, the values of a dropped column. The rewrite scans the current +schema only, so its files hold no dropped value; this is how a drop is erased, +deterministically, whatever Lance's size and deletion heuristics select. It +stages the rewrite detached with fragment ids above the base's high-water +mark, so it needs no `ReserveFragments`; a lagging scalar or vector index is +rebuilt whole as a detached commit chained on the rewrite under its name +(Lance 11 folds only through a linear commit), keeping a vector index's +partition count; the batch publishes once with an exact CAS on every planned +pin, and a pin a concurrent writer moved fails the run with a read-set +conflict so the next run re-plans. A failure before publication leaves +unpublished staging the collector reclaims once it is dead. A strict mutation +prepared before Optimize's publication reports the same read-set conflict as +it would after any other writer. The index writer (`ensure_indices` and the explicit full-text rebuild) follows the same protocol: it opens each productive table at its pin, stages @@ -228,9 +233,33 @@ publication leaves no graph-visible residue; after publication reads, including full-text search through the batch's certificate, serve from the staged version. -Schema apply stages each existing-table rewrite as a detached Overwrite of -the table's pin and publishes it one logical version past the published -version. An added type uses its identity-derived path, which a retry derives +Schema apply changes an existing table's columns by metadata-only Lance +commits (`TableStore::stage_schema_evolution`). Renamed and dropped columns +commit as one detached `Operation::Project` of the table's pin, the surviving +columns keeping their field ids, metadata and indexes; added nullable columns +commit as a detached `Operation::Merge` over the unchanged fragments, with +field ids above every id the data files reference, which fragments that lack +them read as null. Lance refuses a Merge that renames a field, so a table with +both kinds chains the Merge on the Project, and the pin names the tip; either +way the table publishes one logical version past its published version. No +data file is written and no row or Blob payload is read, so the apply's memory +and I/O do not grow with the rows or payload bytes the table stores; they +still grow with its fragment, schema and index metadata, because planning +walks the fragment inventory, the Merge carries it and each detached commit +serializes it. Every surviving index keeps its coverage, and a stored external +Blob descriptor, ranged or not, is never rebuilt. A dropped column's values +stay in the data files until the next optimize rewrites every fragment holding +them (above); `cleanup` then deletes the old files once no retained version +references them. The collector retains every live branch's head and creation +point and every tagged version, so a branch created before that optimize keeps +the old files, merged or not, until it is deleted. Every evolved table's +commits are planned once, from its manifest, before the first effect, and the +effects stage exactly that plan, each step checking that its base has the +columns the previous step left. A column whose type or nullability would change refuses at +planning; the schema planner emits no such step. The physical column order +follows the catalog. + +An added type uses its identity-derived path, which a retry derives again from the accepted allocator. Retry preserves that path and every existing version. It can reuse an original empty, stable-row-ID version-one Create with the desired physical schema. If that original Create has another diff --git a/docs/rfcs/0033-blob-management.md b/docs/rfcs/0033-blob-management.md index deeb86548..6398eb3dd 100644 --- a/docs/rfcs/0033-blob-management.md +++ b/docs/rfcs/0033-blob-management.md @@ -486,8 +486,8 @@ An implementation may wrap Lance `read_blob_ranges`, `read_blobs`, or `take_blobs`, but Lance types do not appear in public signatures. Every complete-payload read uses Lance's batched `read_blobs` API on managed rows with an explicit I/O buffer, through one engine helper: a carried update or -merge cell, the bounded rewrite stream and the schema-apply rewrite each pass -one batch's managed stable row ids to one streamed call. Export and the +merge cell and the bounded rewrite stream each pass one batch's managed stable +row ids to one streamed call. Export and the change-feed baseline do the same for each batch of rows they walk, one call per Blob column, and consume the stream row by row, so one row's values and each call's bounded I/O buffer are resident however many rows the batch holds. @@ -1040,19 +1040,16 @@ Every path that carries an existing Blob cell uses the central descriptor decoder and accounts `BlobReader::len()` before payload allocation. Zero-length managed values participate as values. A nullable cell alone is skipped. -Schema apply carries a whole-object external descriptor without probing or -reading its target. Lance's current logical Blob input cannot express an -existing external offset/length range, so schema apply refuses such a valid -ranged descriptor before recovery arm or table movement; it never silently -widens the cell to the whole object. Supporting descriptor-preserving ranged -schema rewrites remains part of the future ownership-proof optimization below. - -The current materializing rewrite is a V1 implementation, not an ideal -physical plan, and the schema-apply rewrite is not bounded: it scans the whole -table into one batch and holds every managed payload of a rewritten Blob column -in memory before staging. Mutation and merge carries stay under their operation -budget (§10). A bounded, streamed schema-apply rewrite is required future work. -A future optimization may carry immutable +Schema apply never rewrites a Blob cell. Adding, renaming or dropping a +property is a metadata-only Lance commit that keeps every data file, so managed +values of every placement, null and valid-empty cells and external descriptors, +whole-object or ranged, stay exactly as stored, and an added Blob property reads +as null in every existing row. Schema apply reads no payload and probes no +external target, so it needs no Blob byte budget and cannot widen a range. + +The materializing rewrite of mutation and merge carries is a V1 +implementation, not an ideal physical plan, and stays under its operation +budget (§10). A future optimization may carry immutable prepared descriptors for unchanged cells only after a Lance surface guard proves that references cannot escape their source dataset/incarnation and recovery can account for every file. Correctness and ownership proof come before avoiding the @@ -1133,7 +1130,7 @@ Physical row addresses never become public stable identity. | URI parser amplification | Reject a raw configured or input URI above 64 KiB before trimming, parsing, decoding, or filesystem resolution | | External SSRF during read | Descriptor-first classification; redirect only; no proxy or validation on GET/HEAD | | Oversize upload | Route and engine 32 MiB inclusive limits; refusal before effect | -| Rewrite amplification | New logical input and row-writing branch merge pre-size all carried Blob payloads under one 32 MiB operation budget before read; predicate mutation carry applies the same cumulative byte ceiling while materializing bounded scan batches. The schema-apply rewrite has no byte ceiling yet (§8.4) | +| Rewrite amplification | New logical input and row-writing branch merge pre-size all carried Blob payloads under one 32 MiB operation budget before read; predicate mutation carry applies the same cumulative byte ceiling while materializing bounded scan batches. Schema apply reads no Blob payload: its column changes are metadata-only (§8.4) | | Compaction memory | Optimize sets the compaction scanner batch from the planned fragments' largest row, summing that row's Blob columns: as many rows (1 to 8,192) as fit 32 MiB of managed payload at that row's size, so one batch materializes at most 32 MiB of managed payload. A row whose Blob values together exceed 32 MiB is compacted in a batch of its own and materialized whole. This bounds payload per batch, not heap: Lance's writer copies inline payloads into its prepared arrays while it holds the batch (see the operator guide's optimize section). External descriptors are carried unread | | External-source planning | Row-writing branch merge admits at most 8,192 external-reference cells and 32 MiB of retained URI metadata before HEAD; probes are bounded and normalized aliases deduplicate within the applicable operation or scan-batch envelope | | Engine read memory | `BlobReader::read_range` returns at most `BLOB_READ_RANGE_MAX_BYTES` (4 MiB); larger values require consecutive calls and there is no unbounded full-read method | @@ -1492,8 +1489,8 @@ correctness gate. ### Phase 4 — measured optimization - Batched reads (`read_blobs`) landed for materializing rewrites, export, the - change-feed baseline, change images and entity reads (§4). Remaining: make the - schema-apply rewrite bounded and streamed (§8.4), then benchmark. Tuning is + change-feed baseline, change images and entity reads (§4), and schema apply + stopped rewriting Blob columns (§8.4). Remaining: benchmark. Tuning is optional and may not weaken the batched contract or change logical behavior. - Retain the exact empty/null/neighbor compaction guard across every future Lance dependency bump. @@ -1725,6 +1722,35 @@ publisher architecture. ## Decision log +- 2026-10-04: Schema apply's column changes became metadata-only Lance + commits: renames and drops as a detached `Project`, additions as a detached + `Merge` over the unchanged fragments, keeping every data file. The rewrite + had held every managed payload of a table in memory inside the server's + deployment path. Superseded sentences: §4's "a carried update or merge cell, + the bounded rewrite stream and the schema-apply rewrite each pass one + batch's managed stable row ids to one streamed call" (the schema-apply + rewrite is gone); §8.4's "Lance's current logical Blob input cannot express + an existing external offset/length range, so schema apply refuses such a + valid ranged descriptor before recovery arm or table movement; it never + silently widens the cell to the whole object.", "Supporting + descriptor-preserving ranged schema rewrites remains part of the future + ownership-proof optimization below.", "The current materializing rewrite is + a V1 implementation, not an ideal physical plan, and the schema-apply + rewrite is not bounded: it scans the whole table into one batch and holds + every managed payload of a rewritten Blob column in memory before staging." + and "A bounded, streamed schema-apply rewrite is required future work." + (a ranged descriptor is now kept as stored, and no rewrite remains to + bound); §10's rewrite-amplification sentence "The schema-apply rewrite has + no byte ceiling yet (§8.4)"; and Phase 4's "make the schema-apply rewrite + bounded and streamed (§8.4)". A dropped Blob property's managed bytes now + stay with their data files until the next `optimize`, which rewrites every + fragment still holding a dropped column without reading it (the + `compaction_memory.rs` erasure instrument measured 34.1 MiB at its peak on 64 + rows of a kept and a dropped 1 MiB value, under the 42 MiB compaction bound), + and `cleanup` then deletes them once no retained version names them. The + `lance_surface_guards.rs` evolution guard pins the Lance facts; the + `compaction_memory.rs` instrument measured schema apply's peak at 72.1 and + 136.1 MiB on 64 and 128 MiB Blob tables before, and 5.1 MiB on both after. - 2026-10-08: Export, the change-feed baseline, change-feed images and entity reads read managed payloads through the batched `read_blobs` helper the materializing rewrites use, closing that Phase 4 item. Export and the diff --git a/docs/user/operations/maintenance.md b/docs/user/operations/maintenance.md index 9474fa08e..66c9b6fc2 100644 --- a/docs/user/operations/maintenance.md +++ b/docs/user/operations/maintenance.md @@ -27,10 +27,22 @@ omnigraph optimize ./graph.omni omnigraph optimize ./graph.omni --json ``` -Optimize rewrites small fragments into fewer larger fragments, rebuilds each -scalar or vector index whose coverage lags behind appended rows, and builds -missing declared indexes that are ready to build. It does not delete old -versions or collect unused table forks. Use `cleanup` for storage reclamation. +Optimize rewrites small fragments into fewer larger fragments, rewrites every +fragment that still stores a dropped property's values, rebuilds each scalar or +vector index whose coverage lags behind appended rows, and builds missing +declared indexes that are ready to build. It does not delete old versions or +collect unused table forks. Use `cleanup` for storage reclamation. + +Dropping a property removes it from the schema without rewriting its table +(see [schema changes](../schema/index.md#schema-changes)), so its values stay +in the table's data files. Every optimize rewrites each fragment that still +holds them, whatever its size, copying only the remaining properties; the +dropped values are not read. A `cleanup` then deletes the old files, Blob +values included, once nothing it retains references them: no commit its +retention keeps, no live branch created from a commit before that optimize, +and no tag naming such a commit. Merging a branch does not release its files; +deleting it does. A table holding no dropped values and nothing else to +compact is left alone. Each table's work is staged as detached Lance versions of the table's current pin and published in one graph commit, like any other write. A run diff --git a/docs/user/schema/index.md b/docs/user/schema/index.md index 839e721ba..90a17ac3c 100644 --- a/docs/user/schema/index.md +++ b/docs/user/schema/index.md @@ -154,6 +154,15 @@ nodes, edges, or properties with `@rename_from`, adding index declarations, widening an enum with new values, updating descriptions or instructions, and dropping node, edge, or property declarations. +None of these changes rewrites existing rows. Adding a nullable property, +renaming a property, or dropping a property changes only the table's metadata: +existing rows read the new property as null, no stored value or Blob is read, +and existing indexes keep covering the rows they covered. The time and memory +the apply takes therefore do not grow with the number of rows or the size of +their values. They still grow with the table's metadata: its columns, its +indexes and its fragments, of which a table written by many small commits has +more until `omnigraph optimize` compacts them. + Applying changed schema text, including comments or formatting, records the new source in one graph commit even when no table changes are needed. Applying the same accepted source again leaves the graph unchanged. @@ -165,10 +174,22 @@ rejected. The plan reports the exact unsupported step before anything changes. A drop removes the declaration from the current schema and reclaims no storage at apply. Older commits still read the dropped data until `omnigraph cleanup` -stops retaining them; after that, the dropped data cannot be recovered. Review -the plan carefully, and treat the cleanup that follows as the step that cannot -be undone. To reclaim the space, run cleanup with a retention that excludes -the commits before the drop. See [cleanup](../operations/maintenance.md#cleanup). +stops retaining them. A dropped type's data is reclaimed by that cleanup. A +dropped property's values also stay in the table's current data files, beside +the properties that remain, until the next `omnigraph optimize`: every +optimize rewrites each fragment of a table that still stores a dropped +property's values, copying only the remaining properties. Optimize rewrites +main only, and cleanup deletes the old files only once nothing it retains +references them: a live branch keeps the commit it was created from and its +head, a tag keeps the snapshot it names, and the retention policy keeps older +commits. Merging a branch releases none of its files; only deleting the branch +does. To erase dropped data, run optimize after the drop, delete every branch +created from a commit before that optimize and every tag naming such a commit, +then run cleanup with a retention that excludes the commits before the +optimize, for example `--keep 1`; after that, the dropped data cannot be +recovered. Review the plan carefully, and treat that cleanup as the step that +cannot be undone. See [optimize](../operations/maintenance.md#optimize) and +[cleanup](../operations/maintenance.md#cleanup). Cluster-managed graphs change schema through `omnigraph cluster apply`. Direct schema apply and the server schema-apply endpoint refuse cluster-managed graphs.