diff --git a/docs/src/format/table/transaction.md b/docs/src/format/table/transaction.md index f7de5e2d54a..96f0d3134a1 100644 --- a/docs/src/format/table/transaction.md +++ b/docs/src/format/table/transaction.md @@ -582,6 +582,11 @@ same id, name, or path. opt-in for exactly this reason, and a table that has one anywhere in its history raises the minimum reader version for that table to v12.0.0. + Writing is gated twice while the feature is experimental: the calling code + opts in per commit, and the deployment sets + `LANCE_ENABLE_UNSTABLE_TRANSACTION_V2` in the environment. Release builds + refuse without both. + Every operation above is a single named verb. A transaction may instead carry a `CompositeOperation`: an ordered list of granular *actions* that apply atomically as one manifest change. This lets one commit express a change no diff --git a/java/lance-jni/src/transaction.rs b/java/lance-jni/src/transaction.rs index eb06d599f2d..f4df38c7703 100644 --- a/java/lance-jni/src/transaction.rs +++ b/java/lance-jni/src/transaction.rs @@ -1217,6 +1217,11 @@ fn convert_to_java_operation_inner<'local>( &[JValue::Object(&new_bases)], )?) } + // Transaction V2 is a pre-vote draft with no Java surface yet; a + // composite operation has no Java class to map onto. + Operation::CompositeOperation(_) => Err(Error::unsupported_error( + "CompositeOperation transactions are not exposed to the Java bindings".to_string(), + )), } } diff --git a/protos/transaction/actions.proto b/protos/transaction/actions.proto index bb7b5ff4b87..f4d6d2881d5 100644 --- a/protos/transaction/actions.proto +++ b/protos/transaction/actions.proto @@ -18,8 +18,9 @@ package lance.table; * atomically as one manifest change. This is an experimental feature under * the process in docs/src/community/voting.md: the field numbers are not a * stable contract, may change without a separate vote, and are removed if - * the stabilization vote does not pass. Support here is read-side - * fail-closed — rejected on load, with no write path. + * the stabilization vote does not pass. Reading is fail-closed: an action + * this build does not recognize aborts the load rather than being skipped. + * Writing is opt-in, for the reason below. * * COMPATIBILITY WARNING. A table version carrying one of these cannot be * opened at all by Lance before v12.0.0, and the fix is not backported diff --git a/rust/lance-table/src/feature_flags.rs b/rust/lance-table/src/feature_flags.rs index d5a7ec6e31c..7c0b3a90589 100644 --- a/rust/lance-table/src/feature_flags.rs +++ b/rust/lance-table/src/feature_flags.rs @@ -70,6 +70,24 @@ pub(crate) const STICKY_PAIRED_FLAGS: u64 = FLAG_MIXED_DATA_FILE_VERSIONS; /// overlay files before the feature is generally released. pub const ENABLE_UNSTABLE_DATA_OVERLAY_FILES_ENV: &str = "LANCE_ENABLE_UNSTABLE_DATA_OVERLAY_FILES"; +/// Environment variable that opts a release build into committing Transaction +/// V2 (an action-based [`CompositeOperation`]) before the feature is generally +/// released. +/// +/// This gates the writer only. It lives here so every +/// `LANCE_ENABLE_UNSTABLE_*` switch is discoverable in one place, even though +/// Transaction V2 has no manifest flag bit of its own. +/// +/// [`CompositeOperation`]: crate::transaction::action::CompositeOperation +pub const ENABLE_UNSTABLE_TRANSACTION_V2_ENV: &str = "LANCE_ENABLE_UNSTABLE_TRANSACTION_V2"; + +/// Whether this build may commit Transaction V2: always in debug builds, so +/// tests exercise the path, and in release builds only when +/// [`ENABLE_UNSTABLE_TRANSACTION_V2_ENV`] is set. +pub fn transaction_v2_enabled() -> bool { + cfg!(debug_assertions) || std::env::var_os(ENABLE_UNSTABLE_TRANSACTION_V2_ENV).is_some() +} + /// Set the reader and writer feature flags in the manifest based on the contents of the manifest. pub fn apply_feature_flags( manifest: &mut Manifest, diff --git a/rust/lance-table/src/format/fragment.rs b/rust/lance-table/src/format/fragment.rs index 82afee22c1d..be8dfce5cc8 100644 --- a/rust/lance-table/src/format/fragment.rs +++ b/rust/lance-table/src/format/fragment.rs @@ -455,6 +455,22 @@ impl TryFrom for DeletionFile { } } +impl From<&DeletionFile> for pb::DeletionFile { + fn from(value: &DeletionFile) -> Self { + let file_type = match value.file_type { + DeletionFileType::Array => pb::deletion_file::DeletionFileType::ArrowArray, + DeletionFileType::Bitmap => pb::deletion_file::DeletionFileType::Bitmap, + }; + Self { + read_version: value.read_version, + id: value.id, + file_type: file_type.into(), + num_deleted_rows: value.num_deleted_rows.unwrap_or_default() as u64, + base_id: value.base_id, + } + } +} + /// Data fragment. /// /// A fragment is a set of files which represent the different columns of the same rows. @@ -717,19 +733,7 @@ impl TryFrom for Fragment { impl From<&Fragment> for pb::DataFragment { fn from(f: &Fragment) -> Self { - let deletion_file = f.deletion_file.as_ref().map(|f| { - let file_type = match f.file_type { - DeletionFileType::Array => pb::deletion_file::DeletionFileType::ArrowArray, - DeletionFileType::Bitmap => pb::deletion_file::DeletionFileType::Bitmap, - }; - pb::DeletionFile { - read_version: f.read_version, - id: f.id, - file_type: file_type.into(), - num_deleted_rows: f.num_deleted_rows.unwrap_or_default() as u64, - base_id: f.base_id, - } - }); + let deletion_file = f.deletion_file.as_ref().map(pb::DeletionFile::from); let row_id_sequence = f.row_id_meta.as_ref().map(|m| match m { RowIdMeta::Inline(data) => { diff --git a/rust/lance-table/src/transaction.rs b/rust/lance-table/src/transaction.rs index 74fba1b33ba..cc9b9151420 100644 --- a/rust/lance-table/src/transaction.rs +++ b/rust/lance-table/src/transaction.rs @@ -17,6 +17,7 @@ //! ```text //! builder Transaction: an operation plus the version it was based on //! operation the vocabulary of changes an operation can describe +//! action the finer-grained Transaction V2 vocabulary (experimental) //! update_map incremental edits to the manifest's string maps //! validate pre-commit checks against the manifest being replaced //! manifest_build applying an operation to produce the next manifest @@ -26,6 +27,7 @@ //! proto the persisted protobuf encoding of all of the above //! ``` +pub mod action; mod builder; mod conflicts; mod index_maintenance; diff --git a/rust/lance-table/src/transaction/action.rs b/rust/lance-table/src/transaction/action.rs new file mode 100644 index 00000000000..836d5b6a92d --- /dev/null +++ b/rust/lance-table/src/transaction/action.rs @@ -0,0 +1,371 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! The action vocabulary of Transaction V2. +//! +//! Where an [`Operation`](super::Operation) names one whole change and carries a +//! post-image of the parts of the manifest it touches, a [`CompositeOperation`] is an +//! ordered list of [`Action`]s, each recording a single *delta*. Composing +//! several changes into one atomic commit and replaying a change onto a +//! different version both fall out of that, with no per-operation logic. +//! +//! The wire format and the reasoning behind it live in +//! `protos/transaction/actions.proto`; the two definitions must stay in step. +//! Only the subset of the specified vocabulary that is implemented appears here -- +//! an action this build does not know is rejected on load rather than skipped. +//! +//! Each action lives in its own module and owns everything about itself: its +//! definition, how it is applied, which coordinates it writes, and its wire +//! encoding. This module holds the shared vocabulary and dispatches to them. +//! +//! ```text +//! action the vocabulary and the dispatch (this module) +//! action:: one action, end to end +//! action::apply the working state an action set is applied against +//! action::footprint comparing two action sets for conflicts +//! action::proto the envelope around the per-action encodings +//! ``` +//! +//! # Stability +//! +//! Transaction V2 is an experimental format feature: breaking changes may be +//! made to it without a separate vote, and it is removed outright if its +//! stabilization vote does not pass. Nothing in this module is a +//! compatibility contract. +//! +//! Committing a [`CompositeOperation`] is a durable compatibility break, not +//! merely a feature an older reader ignores: a table version carrying one +//! cannot be *opened* by Lance before v12.0.0. Those releases decode the +//! manifest's inline transaction while opening the table, and an operation +//! they do not know decodes to no operation at all, which fails the open. The +//! fix shipped in v12.0.0 and is not expected to be backported, so writing one +//! raises that table's minimum reader version. See +//! . + +/// The action vocabulary, as one list. +/// +/// Every per-variant `match` over an [`Action`] -- the enum itself, its +/// forwarding methods, and both directions of its wire encoding -- is generated +/// from this, so an action is added by writing its module and adding one name +/// here. A module that does not supply the full set of methods fails to +/// compile. +/// +/// The variant name doubles as the protobuf `oneof` variant name and as the +/// action's name in errors and logs, so the three cannot drift apart. +/// +/// This is defined ahead of the modules below so it is in scope for all of +/// them; `macro_rules!` visibility runs from the definition to the end of the +/// enclosing module, children included. +macro_rules! for_each_action { + ($emit:ident) => { + $emit! { + AddFragment, + AddDataFile, + AddField, + AddBase, + TombstoneFieldData, + RemoveFragment, + SetDeletionFile, + AlterField, + DropField, + ReserveFragmentIds, + ReserveRowIds, + ResetTable, + ConfigUpdate, + } + }; +} + +mod add_base; +mod add_data_file; +mod add_field; +mod add_fragment; +mod alter_field; +mod apply; +mod config_update; +mod drop_field; +mod footprint; +mod proto; +mod remove_fragment; +mod reserve_fragment_ids; +mod reserve_row_ids; +mod reset_table; +mod set_deletion_file; +mod tombstone_field_data; + +#[cfg(test)] +mod test_support; + +pub use add_base::AddBase; +pub use add_data_file::AddDataFile; +pub use add_field::AddField; +pub use add_fragment::AddFragment; +pub use alter_field::AlterField; +pub use config_update::{ConfigUpdate, FieldMetadataUpdate}; +pub use drop_field::DropField; +pub use footprint::{ConfigMap, Coordinate, Footprint}; +pub use remove_fragment::RemoveFragment; +pub use reserve_fragment_ids::ReserveFragmentIds; +pub use reserve_row_ids::ReserveRowIds; +pub use reset_table::ResetTable; +pub use set_deletion_file::SetDeletionFile; +pub use tombstone_field_data::TombstoneFieldData; + +use apply::ApplyState; +use lance_core::Result; +use lance_core::deepsize::DeepSizeOf; + +/// A reference to a counter-allocated identifier -- a field id, fragment id, or +/// base id -- that may not have been assigned yet. +/// +/// [`Ref::Committed`] is a concrete id that already exists in the manifest. +/// [`Ref::Local`] is a placeholder token minted by an `Add*` action earlier in +/// the same [`CompositeOperation`]; it resolves to a freshly-allocated id at apply, +/// and re-resolves against the target's counters when the operation is replayed +/// onto a newer version. That re-resolution is what lets two independent +/// `AddField`s on divergent branches become two distinct fields rather than a +/// collision. +/// +/// Local tokens are scoped to one [`CompositeOperation`] and must be distinct within +/// it. The three id spaces do not share a token namespace: a fragment token 0 +/// and a field token 0 are unrelated. +/// +/// ``` +/// # use lance_table::transaction::action::Ref; +/// // A fragment the dataset already has. +/// let existing = Ref::Committed(3); +/// assert_eq!(existing.committed(), Some(3)); +/// +/// // A fragment this operation is about to mint. It has no id until apply +/// // resolves it against whichever version the commit lands on. +/// let minted = Ref::Local(0); +/// assert_eq!(minted.committed(), None); +/// ``` +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, DeepSizeOf)] +pub enum Ref { + Committed(u64), + Local(u32), +} + +impl Ref { + /// The committed id, or `None` if this is an unresolved local token. + pub fn committed(&self) -> Option { + match self { + Self::Committed(id) => Some(*id), + Self::Local(_) => None, + } + } + + /// The local token, or `None` if this reference is already committed. + pub fn local(&self) -> Option { + match self { + Self::Local(token) => Some(*token), + Self::Committed(_) => None, + } + } +} + +/// A composable transaction: an ordered list of user actions that commit +/// atomically as a single manifest change. +/// +/// The `uuid` and `read_version` carried on the wire mirror the enclosing +/// [`Transaction`](super::Transaction) and are filled in from it, so they are +/// not repeated here. +/// +/// The steps are applied in order, and a step may name what an earlier one +/// minted: below, the data file is attached to the fragment added in the step +/// before it, which has no committed id yet. Doing that as two named +/// operations would take two commits, and a reader could observe the empty +/// fragment in between. +/// +/// ``` +/// # use lance_file::version::ConcreteFileVersion; +/// # use lance_table::format::DataFile; +/// use lance_table::transaction::action::{ +/// Action, AddDataFile, AddFragment, CompositeOperation, Ref, UserAction, +/// }; +/// +/// let operation = CompositeOperation::new(vec![ +/// UserAction::new( +/// "add a fragment", +/// vec![Action::AddFragment(AddFragment { +/// id: Ref::Local(0), +/// physical_rows: 10, +/// row_id_meta: None, +/// last_updated_at_version_meta: None, +/// created_at_version_meta: None, +/// data_change: true, +/// })], +/// ), +/// UserAction::new( +/// "back its one column", +/// vec![Action::AddDataFile(AddDataFile { +/// fragment: Ref::Local(0), +/// file: DataFile::new_unstarted("data/new.lance", ConcreteFileVersion::V2_0), +/// field_ids: vec![Ref::Committed(0)], +/// data_change: true, +/// })], +/// ), +/// ]); +/// +/// // The steps are what a user did; the actions are the deltas they became. +/// assert_eq!(operation.actions.len(), 2); +/// assert_eq!(operation.iter_actions().count(), 2); +/// ``` +/// +/// Committing one goes through +/// `CommitBuilder::with_experimental_composite_operations`, which is the +/// caller's acknowledgement that this is an experimental format (see +/// [Stability](self#stability)). +#[derive(Debug, Clone, PartialEq, DeepSizeOf, Default)] +pub struct CompositeOperation { + /// The ordered steps this operation applies. + pub actions: Vec, +} + +impl CompositeOperation { + pub fn new(actions: Vec) -> Self { + Self { actions } + } + + /// Every action in every step, in application order. + pub fn iter_actions(&self) -> impl Iterator { + self.actions.iter().flat_map(|step| step.actions.iter()) + } +} + +/// A single user-recognizable step within a [`CompositeOperation`], e.g. "append +/// batch" or "rebuild index". +/// +/// The description keeps transaction history readable: when a range of versions +/// is squashed, each original operation collapses into one step, so the sequence +/// a user performed survives even though the deltas are flattened when applied. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct UserAction { + pub description: String, + pub actions: Vec, +} + +impl UserAction { + pub fn new(description: impl Into, actions: Vec) -> Self { + Self { + description: description.into(), + actions, + } + } +} + +macro_rules! define_action { + ($($variant:ident,)*) => { + /// A single granular change to the manifest. + /// + /// The specified vocabulary is larger than this; the variants here are the + /// ones this build implements end to end. Each one is defined, applied, + /// and encoded in the module named after it, and appears here only + /// because it is listed in `for_each_action!`. + #[derive(Debug, Clone, PartialEq, DeepSizeOf)] + pub enum Action { + $($variant($variant),)* + } + + impl Action { + pub fn name(&self) -> &'static str { + match self { + $(Self::$variant(_) => stringify!($variant),)* + } + } + + /// Whether this action changes the data a reader would see, as + /// opposed to rearranging how it is stored (compaction, a segment + /// rebuild). + /// + /// CDC and streaming consumers use this to skip commits that cannot + /// have changed any row's value. + pub fn is_data_change(&self) -> bool { + match self { + $(Self::$variant(action) => action.is_data_change(),)* + } + } + + /// Fold this action into the state the next manifest is built from. + fn apply(&self, state: &mut ApplyState) -> Result<()> { + match self { + $(Self::$variant(action) => action.apply(state),)* + } + } + + /// Record the coordinates this action writes. + fn footprint(&self, footprint: &mut Footprint) { + match self { + $(Self::$variant(action) => action.footprint(footprint),)* + } + } + } + }; +} + +for_each_action!(define_action); + +impl std::fmt::Display for Action { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.name()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_ref_accessors() { + assert_eq!(Ref::Committed(7).committed(), Some(7)); + assert_eq!(Ref::Committed(7).local(), None); + assert_eq!(Ref::Local(2).local(), Some(2)); + assert_eq!(Ref::Local(2).committed(), None); + } + + #[test] + fn test_data_change_defaults_by_action_kind() { + let alter = Action::AlterField(AlterField { + field: Ref::Committed(1), + name: Some("renamed".into()), + logical_type: None, + nullable: None, + }); + assert!(!alter.is_data_change(), "a rename changes no row values"); + + let remove = Action::RemoveFragment(RemoveFragment { + fragment: Ref::Committed(3), + data_change: true, + }); + assert!(remove.is_data_change()); + } + + #[test] + fn test_iter_actions_flattens_steps_in_order() { + let operation = CompositeOperation::new(vec![ + UserAction::new( + "first", + vec![Action::RemoveFragment(RemoveFragment { + fragment: Ref::Committed(1), + data_change: true, + })], + ), + UserAction::new( + "second", + vec![Action::RemoveFragment(RemoveFragment { + fragment: Ref::Committed(2), + data_change: true, + })], + ), + ]); + let fragments = operation + .iter_actions() + .map(|action| match action { + Action::RemoveFragment(remove) => remove.fragment, + other => panic!("unexpected action {other}"), + }) + .collect::>(); + assert_eq!(fragments, vec![Ref::Committed(1), Ref::Committed(2)]); + } +} diff --git a/rust/lance-table/src/transaction/action/add_base.rs b/rust/lance-table/src/transaction/action/add_base.rs new file mode 100644 index 00000000000..06dc88a6b6e --- /dev/null +++ b/rust/lance-table/src/transaction/action/add_base.rs @@ -0,0 +1,177 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Mint a new base path. + +use super::apply::ApplyState; +use super::proto::required; +use super::{Coordinate, Footprint}; +use crate::format::{BasePath, pb}; +use lance_core::deepsize::DeepSizeOf; +use lance_core::{Error, Result}; + +/// Mint a new base path. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct AddBase { + /// Token standing in for the base id until it is allocated at apply. + pub local: u32, + /// The base path. Its `id` is ignored and stamped in at apply. + pub base: BasePath, +} + +impl AddBase { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + let id = state.mint_base(self.local)?; + + // Two separate uniqueness rules, reported separately: a caller that + // trips one needs to know which, and the two are fixed differently. + if let Some(name) = &self.base.name { + // Only a name that is actually set: the name is an optional alias, + // and the format puts no uniqueness rule on its absence. + if let Some(conflicting) = state.bases().find(|base| base.name.as_ref() == Some(name)) { + return Err(Error::invalid_input(format!( + "a base path named {:?} already exists (id {}, at '{}'); base names are \ + unique within a dataset", + name, conflicting.id, conflicting.path + ))); + } + } + if let Some(conflicting) = state.bases().find(|base| base.path == self.base.path) { + return Err(Error::invalid_input(format!( + "a base path at '{}' already exists (id {}, named {:?}); base locations \ + are unique within a dataset", + self.base.path, conflicting.id, conflicting.name + ))); + } + + let mut base = self.base.clone(); + base.id = id; + state.push_base(base); + Ok(()) + } + + /// A base path is a location, not data. + pub(super) fn is_data_change(&self) -> bool { + false + } + + /// The base id is minted, but the name and location are not: both must be + /// unique, so two operations claiming either collide. An unset name claims + /// nothing -- it is an optional alias, not a name to share. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + if let Some(name) = &self.base.name { + footprint.write(Coordinate::BaseName(name.clone())); + } + footprint.write(Coordinate::BaseLocation(self.base.path.clone())); + } +} + +impl From<&AddBase> for pb::AddBase { + fn from(value: &AddBase) -> Self { + Self { + local: value.local, + base: Some(pb::BasePath::from(value.base.clone())), + } + } +} + +impl TryFrom for AddBase { + type Error = Error; + + fn try_from(message: pb::AddBase) -> Result { + Ok(Self { + local: message.local, + base: BasePath::from(required(message.base, "AddBase.base")?), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::transaction::action::Action; + use crate::transaction::action::test_support::apply; + use crate::transaction::test_support::sample_manifest; + + #[test] + fn test_add_base_mints_an_id_and_rejects_duplicates() { + let manifest = sample_manifest(); + let next = apply( + &manifest, + vec![Action::AddBase(AddBase { + local: 0, + base: BasePath::new(0, "s3://bucket/a".into(), Some("a".into()), false), + })], + ) + .unwrap(); + assert_eq!(next.base_paths.len(), 1); + assert_eq!(next.base_paths[&1].path, "s3://bucket/a"); + + // A fresh name, but the location is taken. + let error = apply( + &next, + vec![Action::AddBase(AddBase { + local: 0, + base: BasePath::new(0, "s3://bucket/a".into(), Some("other".into()), false), + })], + ) + .unwrap_err(); + assert!( + error + .to_string() + .contains("base path at 's3://bucket/a' already exists"), + "unexpected error: {error}" + ); + } + + /// The name is an optional alias, so leaving it unset is not a name two + /// bases can share. `Operation::UpdateBases` guards its name comparison + /// with `is_some()` for the same reason. + #[test] + fn test_unnamed_bases_do_not_collide_with_each_other() { + let manifest = sample_manifest(); + let next = apply( + &manifest, + vec![ + Action::AddBase(AddBase { + local: 0, + base: BasePath::new(0, "s3://bucket/a".into(), None, false), + }), + Action::AddBase(AddBase { + local: 1, + base: BasePath::new(0, "s3://bucket/b".into(), None, false), + }), + ], + ) + .unwrap(); + assert_eq!(next.base_paths.len(), 2); + } + + /// The second base takes a different id and a different location, but the + /// same name, and the first is only visible to it because both land in one + /// operation. + #[test] + fn test_two_add_bases_in_one_operation_see_each_other() { + let manifest = sample_manifest(); + let error = apply( + &manifest, + vec![ + Action::AddBase(AddBase { + local: 0, + base: BasePath::new(0, "s3://bucket/a".into(), Some("a".into()), false), + }), + Action::AddBase(AddBase { + local: 1, + base: BasePath::new(0, "s3://bucket/b".into(), Some("a".into()), false), + }), + ], + ) + .unwrap_err(); + assert!( + error + .to_string() + .contains("base path named \"a\" already exists"), + "unexpected error: {error}" + ); + } +} diff --git a/rust/lance-table/src/transaction/action/add_data_file.rs b/rust/lance-table/src/transaction/action/add_data_file.rs new file mode 100644 index 00000000000..99d1aa7cc7e --- /dev/null +++ b/rust/lance-table/src/transaction/action/add_data_file.rs @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Add a data file to a fragment. + +use super::apply::ApplyState; +use super::proto::{data_change_from_wire, data_change_to_wire, required}; +use super::{Footprint, Ref}; +use crate::format::{DataFile, pb}; +use lance_core::deepsize::DeepSizeOf; +use lance_core::{Error, Result}; + +/// Add a data file to a fragment. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct AddDataFile { + /// The fragment to add the file to: committed, or a fragment minted earlier + /// in the same operation. + pub fragment: Ref, + /// The file. Its `fields` are placeholders and are stamped in at apply from + /// `field_ids`, which is the authority for the column -> field mapping. + pub file: DataFile, + /// One entry per column in `file`, in column order. + pub field_ids: Vec, + /// See [`AddFragment::data_change`](super::AddFragment::data_change). + pub data_change: bool, +} + +impl AddDataFile { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + let fragment_id = state.resolve_fragment(self.fragment)?; + let field_ids = self + .field_ids + .iter() + .map(|field| state.resolve_field(*field)) + .collect::>>()?; + + let mut file = self.file.clone(); + if !file.column_indices.is_empty() && file.column_indices.len() != field_ids.len() { + return Err(Error::invalid_input(format!( + "AddDataFile for fragment {fragment_id} lists {} field ids but the file has {} \ + columns", + field_ids.len(), + file.column_indices.len() + ))); + } + file.fields = field_ids.into(); + + state + .fragment_mut(fragment_id, "AddDataFile")? + .files + .push(file); + Ok(()) + } + + pub(super) fn is_data_change(&self) -> bool { + self.data_change + } + + /// The data of every committed field the file backs, in the fragment it is + /// attached to. A file backing only minted fields writes nothing. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + footprint.write_field_data(self.fragment, self.field_ids.iter().copied()); + } +} + +impl From<&AddDataFile> for pb::AddDataFile { + fn from(value: &AddDataFile) -> Self { + Self { + fragment: Some(value.fragment.into()), + file: Some(pb::DataFile::from(&value.file)), + field_ids: value.field_ids.iter().map(|id| (*id).into()).collect(), + data_change: data_change_to_wire(value.data_change), + } + } +} + +impl TryFrom for AddDataFile { + type Error = Error; + + fn try_from(message: pb::AddDataFile) -> Result { + Ok(Self { + fragment: required(message.fragment, "AddDataFile.fragment")?.try_into()?, + file: DataFile::try_from(required(message.file, "AddDataFile.file")?)?, + field_ids: message + .field_ids + .into_iter() + .map(Ref::try_from) + .collect::>>()?, + data_change: data_change_from_wire(message.data_change), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::transaction::action::Action; + use crate::transaction::action::test_support::apply; + use crate::transaction::test_support::sample_manifest; + use lance_file::version::ConcreteFileVersion; + + #[test] + fn test_add_data_file_rejects_an_unbound_local_token() { + let manifest = sample_manifest(); + let error = apply( + &manifest, + vec![Action::AddDataFile(AddDataFile { + fragment: Ref::Local(3), + file: DataFile::new_unstarted("data/x.lance", ConcreteFileVersion::V2_0), + field_ids: vec![Ref::Committed(0)], + data_change: true, + })], + ) + .unwrap_err(); + assert!( + error.to_string().contains("local fragment token 3"), + "unexpected error: {error}" + ); + } + + #[test] + fn test_add_data_file_rejects_a_column_count_mismatch() { + let manifest = sample_manifest(); + let mut file = DataFile::new_unstarted("data/x.lance", ConcreteFileVersion::V2_0); + file.column_indices = vec![0, 1].into(); + + let error = apply( + &manifest, + vec![Action::AddDataFile(AddDataFile { + fragment: Ref::Committed(0), + file, + field_ids: vec![Ref::Committed(0)], + data_change: true, + })], + ) + .unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + assert!( + error.to_string().contains("1 field ids"), + "unexpected error: {error}" + ); + } + + #[test] + fn test_add_data_file_rejects_a_missing_fragment() { + let manifest = sample_manifest(); + let error = apply( + &manifest, + vec![Action::AddDataFile(AddDataFile { + fragment: Ref::Committed(7), + file: DataFile::new_unstarted("data/x.lance", ConcreteFileVersion::V2_0), + field_ids: vec![Ref::Committed(0)], + data_change: true, + })], + ) + .unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + assert!(error.to_string().contains("fragment 7"), "{error}"); + } +} diff --git a/rust/lance-table/src/transaction/action/add_field.rs b/rust/lance-table/src/transaction/action/add_field.rs new file mode 100644 index 00000000000..c9c81f566da --- /dev/null +++ b/rust/lance-table/src/transaction/action/add_field.rs @@ -0,0 +1,320 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Mint a new schema field. + +use super::apply::ApplyState; +use super::proto::required; +use super::{Coordinate, Footprint, Ref}; +use crate::format::pb; +use lance_core::datatypes::Field; +use lance_core::deepsize::DeepSizeOf; +use lance_core::{Error, Result}; + +/// Mint a new schema field. +/// +/// A nested column that introduces several fields is several ordered +/// `AddField`s -- parent first, each child naming its parent's local token. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct AddField { + /// Token standing in for the field id until it is allocated at apply. + pub local: u32, + /// The parent field, or `None` for a top-level column. + pub parent: Option, + /// The field definition. Its `id`, `parent_id`, and `children` are ignored: + /// `local` and `parent` carry that structure, and each child is its own + /// action. + pub def: Field, +} + +impl AddField { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + let id = state.mint_field(self.local)?; + let parent_id = self + .parent + .map(|parent| state.resolve_field(parent)) + .transpose()?; + state.reject_duplicate_sibling_name(parent_id, &self.def.name, "AddField")?; + + // The definition's own id, parent id, and children are ignored: the + // minted id and the parent reference carry that structure, and each + // child column arrives as its own action. + let field = Field { + id, + parent_id: parent_id.unwrap_or(-1), + children: Vec::new(), + ..self.def.clone() + }; + + match parent_id { + None => state.schema_mut().fields.push(field), + Some(parent_id) => { + let parent = state + .schema_mut() + .field_by_id_mut(parent_id) + .ok_or_else(|| { + Error::invalid_input(format!( + "AddField names parent field {parent_id}, which does not exist" + )) + })?; + parent.children.push(field); + } + } + Ok(()) + } + + /// A new field starts empty; an + /// [`AddDataFile`](super::AddDataFile) is what gives it values. + pub(super) fn is_data_change(&self) -> bool { + false + } + + /// The field does not exist in the read version, so its definition is not + /// a coordinate. Its name is: two writers adding a column by the same name + /// would otherwise both land. Attaching it under a committed parent does + /// not rewrite the parent's definition, but it does need the parent to + /// still be there. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + footprint.write(Coordinate::FieldName(self.def.name.clone())); + if let Some(parent) = self.parent { + footprint.require_field_definition(parent); + } + } +} + +impl From<&AddField> for pb::AddField { + fn from(value: &AddField) -> Self { + Self { + local: value.local, + parent: value.parent.map(Into::into), + def: Some(lance_file::format::pb::Field::from(&value.def)), + } + } +} + +impl TryFrom for AddField { + type Error = Error; + + fn try_from(message: pb::AddField) -> Result { + Ok(Self { + local: message.local, + parent: message.parent.map(Ref::try_from).transpose()?, + def: Field::from(&required(message.def, "AddField.def")?), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::DataFile; + use crate::transaction::action::test_support::{added_field, apply}; + use crate::transaction::action::{Action, AddDataFile}; + use crate::transaction::test_support::sample_manifest; + use arrow_schema::{DataType, Field as ArrowField}; + use lance_file::version::ConcreteFileVersion; + + #[test] + fn test_two_add_fields_mint_distinct_ids() { + let manifest = sample_manifest(); + let next = apply( + &manifest, + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("a"), + }), + Action::AddField(AddField { + local: 1, + parent: None, + def: added_field("b"), + }), + ], + ) + .unwrap(); + + let ids = next + .schema + .fields + .iter() + .map(|f| (f.name.as_str(), f.id)) + .collect::>(); + assert_eq!(ids, vec![("id", 0), ("a", 1), ("b", 2)]); + } + + #[test] + fn test_add_field_then_add_its_data_file() { + // The add-column shape: mint the field, then write the file that backs + // it, naming the field by the token the mint has not resolved yet. + let manifest = sample_manifest(); + let next = apply( + &manifest, + vec![ + Action::AddField(AddField { + local: 7, + parent: None, + def: added_field("added"), + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Committed(0), + file: DataFile::new_unstarted("data/added.lance", ConcreteFileVersion::V2_0), + field_ids: vec![Ref::Local(7)], + data_change: true, + }), + ], + ) + .unwrap(); + + let field_id = next.schema.field("added").unwrap().id; + assert_eq!(field_id, 1); + let fragment = next.fragments.iter().find(|f| f.id == 0).unwrap(); + assert_eq!(fragment.files.last().unwrap().fields.as_ref(), &[field_id]); + } + + #[test] + fn test_add_field_under_a_parent() { + let manifest = sample_manifest(); + let next = apply( + &manifest, + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: Field::try_from(ArrowField::new( + "nested", + DataType::Struct(Default::default()), + true, + )) + .unwrap(), + }), + Action::AddField(AddField { + local: 1, + parent: Some(Ref::Local(0)), + def: added_field("child"), + }), + ], + ) + .unwrap(); + + let parent = next.schema.field("nested").unwrap(); + assert_eq!(parent.children.len(), 1); + assert_eq!(parent.children[0].name, "child"); + assert_eq!(parent.children[0].parent_id, parent.id); + } + + #[test] + fn test_add_field_rejects_a_missing_parent() { + let manifest = sample_manifest(); + let error = apply( + &manifest, + vec![Action::AddField(AddField { + local: 0, + parent: Some(Ref::Committed(7)), + def: added_field("orphan"), + })], + ) + .unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + assert!(error.to_string().contains("parent field 7"), "{error}"); + } + + #[test] + fn test_add_field_rejects_a_name_a_sibling_already_has() { + // `sample_manifest` already has a top-level field called "id". + let manifest = sample_manifest(); + let error = apply( + &manifest, + vec![Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("id"), + })], + ) + .unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + assert!( + error.to_string().contains("unique among siblings"), + "{error}" + ); + + // Under a parent, only that parent's children are siblings, so the + // top-level name is free again. + let nested = Field::try_from(ArrowField::new( + "nested", + DataType::Struct(Default::default()), + true, + )) + .unwrap(); + let next = apply( + &manifest, + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: nested.clone(), + }), + Action::AddField(AddField { + local: 1, + parent: Some(Ref::Local(0)), + def: added_field("id"), + }), + ], + ) + .unwrap(); + assert_eq!(next.schema.field("nested").unwrap().children[0].name, "id"); + + let error = apply( + &manifest, + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: nested, + }), + Action::AddField(AddField { + local: 1, + parent: Some(Ref::Local(0)), + def: added_field("child"), + }), + Action::AddField(AddField { + local: 2, + parent: Some(Ref::Local(0)), + def: added_field("child"), + }), + ], + ) + .unwrap_err(); + assert!( + error.to_string().contains("unique among siblings"), + "{error}" + ); + } + + #[test] + fn test_duplicate_local_token_is_rejected() { + let manifest = sample_manifest(); + let error = apply( + &manifest, + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("a"), + }), + Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("b"), + }), + ], + ) + .unwrap_err(); + assert!( + error.to_string().contains("minted more than once"), + "unexpected error: {error}" + ); + } +} diff --git a/rust/lance-table/src/transaction/action/add_fragment.rs b/rust/lance-table/src/transaction/action/add_fragment.rs new file mode 100644 index 00000000000..d896325a5b8 --- /dev/null +++ b/rust/lance-table/src/transaction/action/add_fragment.rs @@ -0,0 +1,336 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Add a new, empty fragment. + +use super::apply::ApplyState; +use super::proto::{data_change_from_wire, data_change_to_wire, required}; +use super::{Coordinate, Footprint, Ref}; +use crate::format::{ExternalFile, Fragment, RowIdMeta, pb}; +use crate::rowids::version::RowDatasetVersionMeta; +use lance_core::Result; +use lance_core::deepsize::DeepSizeOf; + +/// Add a new, empty fragment. +/// +/// Its data files arrive via [`AddDataFile`](super::AddDataFile) actions naming +/// this fragment by the same [`Ref`]. A newly added fragment has no deletion +/// vector: it has no committed rows to delete yet. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct AddFragment { + /// The id to add the fragment at. + /// + /// [`Ref::Local`] is the usual form: a token standing in for an id + /// allocated at apply, which re-allocates when the operation is replayed + /// onto a newer version. + /// + /// [`Ref::Committed`] names an id out of a range an earlier commit reserved + /// with [`ReserveFragmentIds`](super::ReserveFragmentIds). It exists for a + /// writer that has to know the id before it commits, because it bakes row + /// addresses into an index it writes in the same commit. Such an id is the + /// writer's alone and stays valid at every later version, so it does not + /// move under replay -- but it is also not the writer's to choose freely: + /// apply rejects an id that is not reserved or is already in use. + pub id: Ref, + /// Physical rows in the fragment, including rows later tombstoned. + pub physical_rows: u64, + /// Stable row id sequence. `None` on datasets without stable row ids, and + /// on datasets that have them but where the ids are assigned at apply. + pub row_id_meta: Option, + /// Per-row version metadata, carried exactly as on + /// [`Fragment`](crate::format::Fragment). + /// + /// `None` means "stamp at apply", which fills both with a uniform sequence + /// at the commit version. That is right for an append, but not for the two + /// producers whose rows carry versions from before this commit, so they set + /// the fields explicitly: an update resolves each row's `created_at` from + /// the fragment the row came from, and a compaction rechunks both sequences + /// off the fragments it merged, since moving a row does not update it. + pub last_updated_at_version_meta: Option, + pub created_at_version_meta: Option, + /// `false` marks a pure rearrangement, e.g. a compaction rewrite. + pub data_change: bool, +} + +impl AddFragment { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + let id = state.add_fragment_id(self.id)?; + state.push_fragment(Fragment { + id, + files: Vec::new(), + overlays: Vec::new(), + deletion_file: None, + row_id_meta: self.row_id_meta.clone(), + physical_rows: Some(self.physical_rows as usize), + last_updated_at_version_meta: self.last_updated_at_version_meta.clone(), + created_at_version_meta: self.created_at_version_meta.clone(), + }); + Ok(()) + } + + pub(super) fn is_data_change(&self) -> bool { + self.data_change + } + + /// Nothing for a local token: the fragment does not exist in the read + /// version, so no concurrent writer can be naming it. + /// + /// A committed id does write one coordinate. A reservation is meant to be + /// one writer's alone, but nothing in the format enforces that, so two + /// operations handed the same range would otherwise each add a fragment at + /// the same id and the second commit would silently win. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + if let Some(id) = self.id.committed() { + footprint.write(Coordinate::FragmentExistence(id)); + } + } +} + +fn external_file_to_wire(file: &ExternalFile) -> pb::ExternalFile { + pb::ExternalFile { + path: file.path.clone(), + offset: file.offset, + size: file.size, + } +} + +fn external_file_from_wire(file: pb::ExternalFile) -> ExternalFile { + ExternalFile { + path: file.path, + offset: file.offset, + size: file.size, + } +} + +impl From<&AddFragment> for pb::AddFragment { + fn from(value: &AddFragment) -> Self { + Self { + id: Some(value.id.into()), + physical_rows: value.physical_rows, + row_id_sequence: value.row_id_meta.as_ref().map(|meta| match meta { + RowIdMeta::Inline(data) => { + pb::add_fragment::RowIdSequence::InlineRowIds(data.to_vec()) + } + RowIdMeta::External(file) => { + pb::add_fragment::RowIdSequence::ExternalRowIds(external_file_to_wire(file)) + } + }), + last_updated_at_version_sequence: value + .last_updated_at_version_meta + .as_ref() + .map(|meta| match meta { + RowDatasetVersionMeta::Inline(data) => { + pb::add_fragment::LastUpdatedAtVersionSequence::InlineLastUpdatedAtVersions( + data.to_vec(), + ) + } + RowDatasetVersionMeta::External(file) => { + pb::add_fragment::LastUpdatedAtVersionSequence::ExternalLastUpdatedAtVersions( + external_file_to_wire(file), + ) + } + }), + created_at_version_sequence: value + .created_at_version_meta + .as_ref() + .map(|meta| match meta { + RowDatasetVersionMeta::Inline(data) => { + pb::add_fragment::CreatedAtVersionSequence::InlineCreatedAtVersions( + data.to_vec(), + ) + } + RowDatasetVersionMeta::External(file) => { + pb::add_fragment::CreatedAtVersionSequence::ExternalCreatedAtVersions( + external_file_to_wire(file), + ) + } + }), + data_change: data_change_to_wire(value.data_change), + } + } +} + +impl TryFrom for AddFragment { + type Error = lance_core::Error; + + fn try_from(message: pb::AddFragment) -> Result { + Ok(Self { + id: required(message.id, "AddFragment.id")?.try_into()?, + physical_rows: message.physical_rows, + row_id_meta: message.row_id_sequence.map(|sequence| match sequence { + pb::add_fragment::RowIdSequence::InlineRowIds(data) => { + RowIdMeta::Inline(data.into()) + } + pb::add_fragment::RowIdSequence::ExternalRowIds(file) => { + RowIdMeta::External(external_file_from_wire(file)) + } + }), + last_updated_at_version_meta: message.last_updated_at_version_sequence.map( + |sequence| { + match sequence { + pb::add_fragment::LastUpdatedAtVersionSequence::InlineLastUpdatedAtVersions( + data, + ) => RowDatasetVersionMeta::Inline(data.into()), + pb::add_fragment::LastUpdatedAtVersionSequence::ExternalLastUpdatedAtVersions( + file, + ) => RowDatasetVersionMeta::External(external_file_from_wire(file)), + } + }, + ), + created_at_version_meta: message.created_at_version_sequence.map(|sequence| { + match sequence { + pb::add_fragment::CreatedAtVersionSequence::InlineCreatedAtVersions(data) => { + RowDatasetVersionMeta::Inline(data.into()) + } + pb::add_fragment::CreatedAtVersionSequence::ExternalCreatedAtVersions(file) => { + RowDatasetVersionMeta::External(external_file_from_wire(file)) + } + } + }), + data_change: data_change_from_wire(message.data_change), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::DataFile; + use crate::transaction::action::test_support::apply; + use crate::transaction::action::{Action, AddDataFile, ReserveFragmentIds}; + use crate::transaction::test_support::sample_manifest; + use lance_file::version::ConcreteFileVersion; + + fn add_fragment(id: Ref) -> Action { + Action::AddFragment(AddFragment { + id, + physical_rows: 10, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }) + } + + #[test] + fn test_add_fragment_and_data_file_mint_ids() { + let manifest = sample_manifest(); + let next = apply( + &manifest, + vec![ + Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 10, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Local(0), + file: DataFile::new_unstarted("data/new.lance", ConcreteFileVersion::V2_0), + field_ids: vec![Ref::Committed(0)], + data_change: true, + }), + ], + ) + .unwrap(); + + // sample_manifest already holds fragment 0, so the mint lands on 1. + assert_eq!( + next.fragments.iter().map(|f| f.id).collect::>(), + vec![0, 1] + ); + let minted = next.fragments.iter().find(|f| f.id == 1).unwrap(); + assert_eq!(minted.physical_rows, Some(10)); + assert_eq!(minted.files.len(), 1); + // The file's field list is stamped in from the action's refs. + assert_eq!(minted.files[0].fields.as_ref(), &[0]); + assert_eq!(next.max_fragment_id(), Some(1)); + } + + #[test] + fn test_a_fragment_can_be_added_at_a_reserved_id() { + // What a writer that needs the id up front does: reserve in one commit, + // read the high-water mark back, then name the id in the next. + let reserved = apply( + &sample_manifest(), + vec![Action::ReserveFragmentIds(ReserveFragmentIds { count: 2 })], + ) + .unwrap(); + assert_eq!(reserved.max_fragment_id(), Some(2)); + + let next = apply(&reserved, vec![add_fragment(Ref::Committed(1))]).unwrap(); + + assert_eq!( + next.fragments.iter().map(|f| f.id).collect::>(), + vec![0, 1] + ); + // The rest of the reservation stays reserved: the high-water mark does + // not fall back to the ids actually in use. + assert_eq!(next.max_fragment_id(), Some(2)); + } + + #[test] + fn test_an_unreserved_id_is_rejected() { + // sample_manifest holds fragment 0 and nothing is reserved, so 1 is the + // next id a mint would hand out -- claiming it would collide. + let error = apply(&sample_manifest(), vec![add_fragment(Ref::Committed(1))]).unwrap_err(); + + assert!(matches!(error, lance_core::Error::InvalidInput { .. })); + assert!( + error.to_string().contains("no commit has reserved"), + "{error}" + ); + } + + #[test] + fn test_an_id_already_in_use_is_rejected() { + let reserved = apply( + &sample_manifest(), + vec![Action::ReserveFragmentIds(ReserveFragmentIds { count: 2 })], + ) + .unwrap(); + + // Reserved, but claimed twice in the one operation. + let error = apply( + &reserved, + vec![ + add_fragment(Ref::Committed(1)), + add_fragment(Ref::Committed(1)), + ], + ) + .unwrap_err(); + + assert!(error.to_string().contains("already exists"), "{error}"); + } + + #[test] + fn test_a_reserved_id_can_back_a_data_file_in_the_same_operation() { + let reserved = apply( + &sample_manifest(), + vec![Action::ReserveFragmentIds(ReserveFragmentIds { count: 1 })], + ) + .unwrap(); + + let next = apply( + &reserved, + vec![ + add_fragment(Ref::Committed(1)), + Action::AddDataFile(AddDataFile { + // The same reference the AddFragment carried, so a writer + // does not need a local token alongside the reserved id. + fragment: Ref::Committed(1), + file: DataFile::new_unstarted("data/new.lance", ConcreteFileVersion::V2_0), + field_ids: vec![Ref::Committed(0)], + data_change: true, + }), + ], + ) + .unwrap(); + + let added = next.fragments.iter().find(|f| f.id == 1).unwrap(); + assert_eq!(added.files.len(), 1); + assert_eq!(added.files[0].fields.as_ref(), &[0]); + } +} diff --git a/rust/lance-table/src/transaction/action/alter_field.rs b/rust/lance-table/src/transaction/action/alter_field.rs new file mode 100644 index 00000000000..a9426e08426 --- /dev/null +++ b/rust/lance-table/src/transaction/action/alter_field.rs @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Alter facets of an existing field in place. + +use super::apply::ApplyState; +use super::proto::required; +use super::{Coordinate, Footprint, Ref}; +use crate::format::pb; +use lance_core::datatypes::LogicalType; +use lance_core::deepsize::DeepSizeOf; +use lance_core::{Error, Result}; + +/// Alter facets of an existing field in place, preserving its id. +/// +/// Each facet is independently optional -- present means "change this", absent +/// means "leave it alone" -- so a widening cast and a nullability relaxation on +/// the same field commute. A cast additionally needs a +/// [`TombstoneFieldData`](super::TombstoneFieldData) plus a fresh +/// [`AddDataFile`](super::AddDataFile) to rewrite the data. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct AlterField { + pub field: Ref, + pub name: Option, + /// The new type. The cast. + /// + /// A [`LogicalType`], Lance's own string encoding of an Arrow type and + /// what [`Field::logical_type`](lance_core::datatypes::Field::logical_type) + /// holds, not an [`arrow_schema::DataType`]. The schema persists the string + /// form, so the action names the same thing the manifest stores rather than + /// a type that would have to be converted at both ends. + pub logical_type: Option, + pub nullable: Option, +} + +impl AlterField { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + let field_id = state.resolve_field(self.field)?; + let missing = || { + Error::invalid_input(format!( + "AlterField names field {field_id}, which does not exist" + )) + }; + if let Some(name) = &self.name { + let field = state.schema().field_by_id(field_id).ok_or_else(missing)?; + if *name != field.name { + let parent_id = (field.parent_id >= 0).then_some(field.parent_id); + state.reject_duplicate_sibling_name(parent_id, name, "AlterField")?; + } + } + let field = state + .schema_mut() + .field_by_id_mut(field_id) + .ok_or_else(missing)?; + if let Some(name) = &self.name { + field.name.clone_from(name); + } + if let Some(nullable) = self.nullable { + field.nullable = nullable; + } + if let Some(logical_type) = &self.logical_type { + field.logical_type = logical_type.clone(); + // The cast leaves any index on the field describing the old type. + // The data rewrite itself is separate actions; this only records + // that every fragment's view of the field changed. + state.rebind_field_everywhere(field_id); + } + Ok(()) + } + + /// Renaming or relaxing a field leaves the values alone, and the + /// rewrite a cast needs is separate actions that answer for themselves. + pub(super) fn is_data_change(&self) -> bool { + false + } + + /// The field's definition, and the name a rename takes -- a name is a + /// coordinate whether a field is added under it or moved to it. The data + /// rewrite a cast needs is separate actions, which record their own + /// coordinates. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + footprint.write_field_definition(self.field); + if let Some(name) = &self.name { + footprint.write(Coordinate::FieldName(name.clone())); + } + } +} + +impl From<&AlterField> for pb::AlterField { + fn from(value: &AlterField) -> Self { + Self { + field: Some(value.field.into()), + name: value.name.clone(), + logical_type: value + .logical_type + .as_ref() + .map(|logical_type| logical_type.to_string()), + nullable: value.nullable, + } + } +} + +impl TryFrom for AlterField { + type Error = Error; + + fn try_from(message: pb::AlterField) -> Result { + Ok(Self { + field: required(message.field, "AlterField.field")?.try_into()?, + name: message.name, + logical_type: message.logical_type.as_deref().map(LogicalType::from), + nullable: message.nullable, + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::transaction::action::Action; + use crate::transaction::action::test_support::{apply, apply_with_indices, backed_manifest}; + use crate::transaction::test_support::sample_index_metadata; + + #[test] + fn test_alter_field_renames_without_touching_indices() { + let (next, indices) = apply_with_indices( + &backed_manifest(), + vec![Action::AlterField(AlterField { + field: Ref::Committed(0), + name: Some("renamed".into()), + logical_type: None, + nullable: Some(true), + })], + vec![sample_index_metadata("idx")], + ) + .unwrap(); + + let field = next.schema.field_by_id(0).unwrap(); + assert_eq!(field.name, "renamed"); + assert!(field.nullable); + // A rename does not change the values the index recorded. + assert!(indices[0].fragment_bitmap.as_ref().unwrap().contains(0)); + } + + #[test] + fn test_alter_field_retype_prunes_covering_indices() { + let (next, indices) = apply_with_indices( + &backed_manifest(), + vec![Action::AlterField(AlterField { + field: Ref::Committed(0), + name: None, + logical_type: Some("int64".into()), + nullable: None, + })], + vec![sample_index_metadata("idx")], + ) + .unwrap(); + + assert_eq!( + next.schema.field_by_id(0).unwrap().logical_type.to_string(), + "int64" + ); + assert!(indices[0].fragment_bitmap.as_ref().unwrap().is_empty()); + } + + #[test] + fn test_alter_field_rejects_a_missing_field() { + let error = apply( + &backed_manifest(), + vec![Action::AlterField(AlterField { + field: Ref::Committed(7), + name: Some("nope".into()), + logical_type: None, + nullable: None, + })], + ) + .unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + assert!(error.to_string().contains("field 7"), "{error}"); + } + + #[test] + fn test_alter_field_rejects_renaming_onto_a_sibling() { + use crate::transaction::action::AddField; + use crate::transaction::action::test_support::added_field; + + let taken = Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("taken"), + }); + let rename = |name: &str| { + Action::AlterField(AlterField { + field: Ref::Committed(0), + name: Some(name.into()), + logical_type: None, + nullable: None, + }) + }; + + let error = apply(&backed_manifest(), vec![taken.clone(), rename("taken")]).unwrap_err(); + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + assert!( + error.to_string().contains("unique among siblings"), + "{error}" + ); + + // Restating a field's own name is not a collision with itself. + let next = apply(&backed_manifest(), vec![taken, rename("id")]).unwrap(); + assert_eq!(next.schema.field_by_id(0).unwrap().name, "id"); + } + + #[test] + fn test_alter_field_can_name_a_field_minted_in_the_same_operation() { + // A squash of "add column" and "rename column" lowers to exactly this, + // and has no committed id to name the field by. + use crate::transaction::action::AddField; + use crate::transaction::action::test_support::added_field; + + let next = apply( + &backed_manifest(), + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("before"), + }), + Action::AlterField(AlterField { + field: Ref::Local(0), + name: Some("after".into()), + logical_type: None, + nullable: None, + }), + ], + ) + .unwrap(); + + assert!(next.schema.field("before").is_none()); + assert!(next.schema.field("after").is_some()); + } +} diff --git a/rust/lance-table/src/transaction/action/apply.rs b/rust/lance-table/src/transaction/action/apply.rs new file mode 100644 index 00000000000..c6f40b94aeb --- /dev/null +++ b/rust/lance-table/src/transaction/action/apply.rs @@ -0,0 +1,839 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Applying an action set to produce the next manifest. +//! +//! Each action is applied in order against a working copy of the read-version +//! state. Minting actions allocate an id from the target's counters as they are +//! reached and record it against their local token, so every later reference to +//! that token -- in this same operation -- resolves to the id this apply chose. +//! Replaying the same action set against a different version therefore produces +//! different ids without any of the actions changing. +//! +//! An action naming an id an earlier commit reserved is the exception: that id +//! is already allocated, so apply checks the claim rather than making one, and +//! replay leaves it alone. +//! +//! [`ApplyState`] is that working copy, and its methods are the API the action +//! modules program against. What each action does with it lives in that action's +//! own module. + +use super::{CompositeOperation, Ref}; +use crate::format::{BasePath, Fragment, IndexMetadata, Manifest, ManifestBuildConfig, RowIdMeta}; +use crate::rowids::read_row_ids; +use crate::rowids::version::build_version_meta; +use crate::system_index::mem_wal::MEM_WAL_INDEX_NAME; +use crate::transaction::{LogicalIndexSegments, ReadVersionState, Transaction}; +use lance_core::datatypes::Schema; +use lance_core::{Error, Result}; +use std::collections::{HashMap, HashSet}; + +/// The field id written into a data file's field list once the file no longer +/// backs that field. A file whose every slot is tombstoned is dropped. +pub(super) const TOMBSTONED_FIELD: i32 = -2; + +impl Transaction { + /// Build the next manifest by applying an action set. + /// + /// Takes the current manifest by reference, unlike the legacy path: an + /// action set is a delta, so there is nothing for it to be a delta against + /// when the dataset does not exist yet. The caller turns the absent case + /// into an error, which is where the `Option` it holds actually lives. + pub(in crate::transaction) fn build_manifest_from_actions( + &self, + composite_operation: &CompositeOperation, + current_manifest: &Manifest, + current_indices: Vec, + transaction_file_path: &str, + config: &ManifestBuildConfig, + read_version_state: Option>, + ) -> Result<(Manifest, Vec)> { + // No `migration_next_row_id` escape, unlike the legacy guard in + // `build_manifest_with_read_version`: `migrate_to_stable_row_ids` + // activates the flag with an `Operation::Merge`, so an action set + // never carries that watermark. + if config.use_stable_row_ids && !current_manifest.uses_stable_row_ids() { + return Err(Error::not_supported_source( + "This dataset was not created with the stable row ids feature. Please run \ + `migrate_to_stable_row_ids` before attempting to use stable row ids" + .into(), + )); + } + + // Snapshot taken before the actions rewrite the list, so coverage can + // be compared against what each logical index looked like going in -- + // the same point the legacy path snapshots at, before its operation + // arm runs. Only tables with a MemWAL index maintain coverage, so + // every other commit pays nothing. + let mem_wal_segments_before = current_indices + .iter() + .any(|index| index.name == MEM_WAL_INDEX_NAME) + .then(|| Self::logical_index_segments(¤t_indices)); + + let mut state = ApplyState::new(current_manifest); + for action in composite_operation.iter_actions() { + action.apply(&mut state)?; + } + + state.into_manifest( + self, + current_indices, + transaction_file_path, + config, + mem_wal_segments_before.as_ref(), + read_version_state, + ) + } +} + +/// The read-version state an action set is applied against, plus the id +/// allocations made so far. +pub(super) struct ApplyState<'a> { + /// The read version this delta applies to. + current_manifest: &'a Manifest, + schema: Schema, + fragments: Vec, + /// Base paths minted by this operation. Kept apart from the manifest's own + /// base paths, which the manifest assembly inherits from the read version. + new_bases: Vec, + existing_base_paths: HashMap, + /// The manifest's string maps. Unlike the schema and the fragment list, + /// these are inherited wholesale by the manifest assembly, so an edit has + /// to be written back over the assembled manifest. + config: HashMap, + table_metadata: HashMap, + + next_fragment_id: u64, + next_field_id: i32, + next_base_id: u32, + + /// Local token -> the id minted for it, one map per id space. + fragment_tokens: HashMap, + field_tokens: HashMap, + base_tokens: HashMap, + + /// Ids of the fragments this operation added, whether minted from the + /// counter or claimed out of a reservation. + new_fragments: HashSet, + + /// The highest fragment id this operation reserved for a later writer, if + /// it reserved any. No fragment backs it, so the manifest assembly cannot + /// infer it from the fragment list. + reserved_fragment_ids: Option, + + /// How many row ids this operation reserved for a later writer. Taken off + /// the counter after this operation's own fragments are numbered, so the + /// reserved range is always the top `count` ids of the resulting watermark. + reserved_row_ids: u64, + + /// Fields whose backing data changed, per fragment. An index covering such + /// a field no longer describes that fragment's contents. + rebound_fields: HashMap>, + + /// Whether the table was reset, which discards every index outright rather + /// than pruning fragments out of them. + reset: bool, +} + +impl<'a> ApplyState<'a> { + fn new(manifest: &'a Manifest) -> Self { + Self { + current_manifest: manifest, + schema: manifest.schema.clone(), + fragments: manifest.fragments.as_ref().clone(), + new_bases: Vec::new(), + existing_base_paths: manifest.base_paths.clone(), + config: manifest.config.clone(), + table_metadata: manifest.table_metadata.clone(), + next_fragment_id: manifest.max_fragment_id().map(|id| id + 1).unwrap_or(0), + next_field_id: manifest.max_field_id() + 1, + next_base_id: manifest + .base_paths + .keys() + .max() + .map(|id| id + 1) + .unwrap_or(1), + fragment_tokens: HashMap::new(), + field_tokens: HashMap::new(), + base_tokens: HashMap::new(), + new_fragments: HashSet::new(), + reserved_fragment_ids: None, + reserved_row_ids: 0, + rebound_fields: HashMap::new(), + reset: false, + } + } + + /// Assemble the next manifest from the state the actions left behind. + fn into_manifest( + mut self, + transaction: &Transaction, + current_indices: Vec, + transaction_file_path: &str, + config: &ManifestBuildConfig, + mem_wal_segments_before: Option<&LogicalIndexSegments>, + read_version_state: Option>, + ) -> Result<(Manifest, Vec)> { + let current_manifest = self.current_manifest; + let new_version = current_manifest.version + 1; + + let mut next_row_id = current_manifest + .uses_stable_row_ids() + .then_some(current_manifest.next_row_id); + self.assign_row_ids_to_new_fragments(&mut next_row_id, new_version)?; + self.take_row_id_reservation(&mut next_row_id)?; + + let ApplyState { + schema, + mut fragments, + new_bases, + rebound_fields, + reserved_fragment_ids, + reset, + config: dataset_config, + table_metadata, + .. + } = self; + + let mut indices = current_indices; + if reset { + indices.clear(); + } + prune_rebound_fields_from_indices(&mut indices, &rebound_fields); + Transaction::retain_relevant_indices(&mut indices, &schema, &fragments); + + Transaction::normalize_fragments(&mut fragments)?; + + // Applied once the final index list is known, so it sees exactly the + // indices this commit publishes rather than what any one action + // intended. Same position as on the legacy path: after the index list + // has been pruned, before the manifest is assembled. + if let Some(segments_before) = mem_wal_segments_before { + Transaction::apply_mem_wal_index_coverage( + &mut indices, + segments_before, + read_version_state, + new_version, + )?; + } + + let mut manifest = transaction.assemble_manifest( + Some(current_manifest), + schema, + fragments, + &indices, + HashMap::new(), + false, + config, + )?; + + for base in new_bases { + manifest.base_paths.insert(base.id, base); + } + + manifest.config = dataset_config; + manifest.table_metadata = table_metadata; + + // A reserved id backs no fragment, so the manifest assembly cannot + // derive it from the fragment list; raise the high-water mark to cover + // the range so a later writer's ids are not handed out twice. + if let Some(high_water) = reserved_fragment_ids { + let high_water = u32::try_from(high_water).map_err(|_| { + Error::invalid_input(format!( + "reserving fragment ids up to {high_water} exceeds the maximum fragment id \ + ({})", + u32::MAX + )) + })?; + manifest.max_fragment_id = Some( + manifest + .max_fragment_id + .map_or(high_water, |current| current.max(high_water)), + ); + } + + manifest.transaction_file = Some(transaction_file_path.to_string()); + if let Some(next_row_id) = next_row_id { + manifest.next_row_id = next_row_id; + } + + Ok((manifest, indices)) + } + + pub(super) fn schema(&self) -> &Schema { + &self.schema + } + + pub(super) fn schema_mut(&mut self) -> &mut Schema { + &mut self.schema + } + + /// Reject `name` if a field with that name already sits under `parent_id` + /// (or at the top level, for `None`). Field names are unique among + /// siblings, and this is the one place the schema is at hand to check it: + /// the footprint keeps two concurrent writers from taking one name, but a + /// name already committed is only visible here. + pub(super) fn reject_duplicate_sibling_name( + &self, + parent_id: Option, + name: &str, + action: &str, + ) -> Result<()> { + let siblings = match parent_id { + None => &self.schema.fields, + Some(parent_id) => { + &self + .schema + .field_by_id(parent_id) + .ok_or_else(|| { + Error::invalid_input(format!( + "{action} names parent field {parent_id}, which does not exist" + )) + })? + .children + } + }; + if let Some(existing) = siblings.iter().find(|field| field.name == name) { + return Err(Error::invalid_input(format!( + "{action} names a field {name:?}, but field {} already has that name at the same \ + level; field names are unique among siblings", + existing.id + ))); + } + Ok(()) + } + + pub(super) fn config_mut(&mut self) -> &mut HashMap { + &mut self.config + } + + pub(super) fn table_metadata_mut(&mut self) -> &mut HashMap { + &mut self.table_metadata + } + + pub(super) fn fragments_mut(&mut self) -> &mut [Fragment] { + &mut self.fragments + } + + pub(super) fn fragment_mut(&mut self, fragment_id: u64, action: &str) -> Result<&mut Fragment> { + self.fragments + .iter_mut() + .find(|fragment| fragment.id == fragment_id) + .ok_or_else(|| { + Error::invalid_input(format!( + "{action} targets fragment {fragment_id}, which does not exist" + )) + }) + } + + pub(super) fn push_fragment(&mut self, fragment: Fragment) { + self.fragments.push(fragment); + } + + /// Drop a fragment and forget everything recorded about it. `false` if no + /// such fragment was present. + pub(super) fn remove_fragment(&mut self, fragment_id: u64) -> bool { + let before = self.fragments.len(); + self.fragments.retain(|fragment| fragment.id != fragment_id); + if self.fragments.len() == before { + return false; + } + self.new_fragments.remove(&fragment_id); + self.rebound_fields.remove(&fragment_id); + true + } + + /// Empty the table: no schema, no fragments, no indices. The id counters + /// keep going, so a field or fragment added afterwards never reuses an id an + /// old file might still name. + pub(super) fn reset(&mut self) { + self.schema.fields.clear(); + self.schema.metadata.clear(); + self.fragments.clear(); + self.new_fragments.clear(); + self.rebound_fields.clear(); + self.reset = true; + } + + /// The base paths this apply can see: the read version's, plus the ones + /// earlier actions in this operation minted. + pub(super) fn bases(&self) -> impl Iterator { + self.existing_base_paths + .values() + .chain(self.new_bases.iter()) + } + + pub(super) fn push_base(&mut self, base: BasePath) { + self.new_bases.push(base); + } + + /// The id to add a new fragment at: minted off the counter for a local + /// token, or claimed out of a reservation for a committed id. + pub(super) fn add_fragment_id(&mut self, reference: Ref) -> Result { + match reference { + Ref::Local(token) => self.mint_fragment(token), + Ref::Committed(id) => self.claim_reserved_fragment_id(id), + } + } + + fn mint_fragment(&mut self, token: u32) -> Result { + if self.fragment_tokens.contains_key(&token) { + return Err(duplicate_token_err("fragment", token)); + } + let id = self.next_fragment_id; + self.next_fragment_id += 1; + self.fragment_tokens.insert(token, id); + self.new_fragments.insert(id); + Ok(id) + } + + /// Claim an id an earlier commit set aside with + /// [`ReserveFragmentIds`](super::ReserveFragmentIds). + /// + /// Only ids already below the counter can be claimed. Anything at or above + /// it is an id a later mint would hand out again, so a writer that guessed + /// rather than reserved is rejected here instead of colliding later. + fn claim_reserved_fragment_id(&mut self, id: u64) -> Result { + if id >= self.next_fragment_id { + return Err(Error::invalid_input(format!( + "AddFragment adds a fragment at id {id}, which no commit has reserved: only ids \ + below {} are reserved. Reserve a range with ReserveFragmentIds and read the \ + committed manifest's high-water mark back before naming an id.", + self.next_fragment_id + ))); + } + if self.fragments.iter().any(|fragment| fragment.id == id) { + return Err(Error::invalid_input(format!( + "AddFragment adds a fragment at id {id}, which already exists" + ))); + } + self.new_fragments.insert(id); + Ok(id) + } + + /// Take `count` ids off the fragment counter without minting fragments for + /// them. The reserved ids are `[next, next + count)`; a later writer names + /// them as [`Ref::Committed`]. + pub(super) fn reserve_fragment_ids(&mut self, count: u32) { + if count == 0 { + return; + } + self.next_fragment_id += u64::from(count); + self.reserved_fragment_ids = Some(self.next_fragment_id - 1); + } + + /// Set `count` row ids aside for a later writer, on top of any this + /// operation already reserved. + pub(super) fn reserve_row_ids(&mut self, count: u64) { + self.reserved_row_ids += count; + } + + pub(super) fn mint_field(&mut self, token: u32) -> Result { + if self.field_tokens.contains_key(&token) { + return Err(duplicate_token_err("field", token)); + } + let id = self.next_field_id; + self.next_field_id += 1; + self.field_tokens.insert(token, id); + Ok(id) + } + + pub(super) fn mint_base(&mut self, token: u32) -> Result { + if self.base_tokens.contains_key(&token) { + return Err(duplicate_token_err("base", token)); + } + let id = self.next_base_id; + self.next_base_id += 1; + self.base_tokens.insert(token, id); + Ok(id) + } + + pub(super) fn resolve_fragment(&self, reference: Ref) -> Result { + match reference { + Ref::Committed(id) => Ok(id), + Ref::Local(token) => self + .fragment_tokens + .get(&token) + .copied() + .ok_or_else(|| unbound_token_err("fragment", token)), + } + } + + pub(super) fn resolve_field(&self, reference: Ref) -> Result { + match reference { + Ref::Committed(id) => i32::try_from(id).map_err(|_| { + Error::invalid_input(format!("field id {id} in an action is out of range")) + }), + Ref::Local(token) => self + .field_tokens + .get(&token) + .copied() + .ok_or_else(|| unbound_token_err("field", token)), + } + } + + /// Record that these fields' data in this fragment no longer matches what + /// an index built over them recorded. + pub(super) fn rebind_fields( + &mut self, + fragment_id: u64, + fields: impl IntoIterator, + ) { + self.rebound_fields + .entry(fragment_id) + .or_default() + .extend(fields); + } + + /// As [`Self::rebind_fields`], for a change that invalidates the field + /// across every fragment at once. + pub(super) fn rebind_field_everywhere(&mut self, field: i32) { + let fragment_ids = self + .fragments + .iter() + .map(|fragment| fragment.id) + .collect::>(); + for fragment_id in fragment_ids { + self.rebound_fields + .entry(fragment_id) + .or_default() + .insert(field); + } + } + + /// Stamp row ids and version metadata onto the fragments this operation + /// minted, matching what an Append does for its new fragments. + fn assign_row_ids_to_new_fragments( + &mut self, + next_row_id: &mut Option, + new_version: u64, + ) -> Result<()> { + let Some(next_row_id) = next_row_id.as_mut() else { + return Ok(()); + }; + self.validate_supplied_row_ids(*next_row_id)?; + let minted_ids = std::mem::take(&mut self.new_fragments); + // The manifest assembly sorts fragments by id, so partitioning them here + // does not disturb the final order. + let (mut minted, existing): (Vec, Vec) = self + .fragments + .drain(..) + .partition(|fragment| minted_ids.contains(&fragment.id)); + + Transaction::assign_row_ids(next_row_id, minted.as_mut_slice())?; + for fragment in minted.iter_mut() { + // An action may carry its own sequences (a squashed operation + // does); only stamp the ones it left for apply to fill. + let version_meta = build_version_meta(fragment, new_version); + if fragment.last_updated_at_version_meta.is_none() { + fragment.last_updated_at_version_meta = version_meta.clone(); + } + if fragment.created_at_version_meta.is_none() { + fragment.created_at_version_meta = version_meta; + } + } + + self.fragments = existing; + self.fragments.extend(minted); + self.new_fragments = minted_ids; + Ok(()) + } + + /// Reject row ids a new fragment arrived with that no commit reserved. + /// + /// [`Transaction::assign_row_ids`] honors a sequence a fragment carries and + /// leaves the counter where it found it, which is right for a compaction: + /// its rows keep ids that are already below the watermark. A writer + /// supplying *fresh* ids must have set them aside with + /// [`ReserveRowIds`](super::ReserveRowIds), which moved the watermark past + /// them. An id at or above the watermark was never reserved, and the next + /// append would hand it out a second time. + fn validate_supplied_row_ids(&self, next_row_id: u64) -> Result<()> { + for fragment in &self.fragments { + if !self.new_fragments.contains(&fragment.id) { + continue; + } + // An external sequence lives in a file, which this synchronous path + // has no way to read; it goes unchecked. + let Some(RowIdMeta::Inline(data)) = &fragment.row_id_meta else { + continue; + }; + let Some(range) = read_row_ids(data)?.row_id_range() else { + continue; + }; + if *range.end() >= next_row_id { + return Err(Error::invalid_input(format!( + "fragment {} is added carrying row id {}, which no commit has reserved: \ + only ids below {next_row_id} are reserved. Reserve a range with \ + ReserveRowIds and read the committed manifest's next_row_id back before \ + supplying row ids.", + fragment.id, + range.end() + ))); + } + } + Ok(()) + } + + /// Take this operation's row id reservation off the counter, after its own + /// fragments have been numbered, so the reserved range is always the top + /// `count` ids of the committed watermark. + fn take_row_id_reservation(&self, next_row_id: &mut Option) -> Result<()> { + if self.reserved_row_ids == 0 { + return Ok(()); + } + let Some(next_row_id) = next_row_id.as_mut() else { + return Err(Error::invalid_input( + "ReserveRowIds needs a row id counter to reserve from, and this dataset does \ + not use stable row ids", + )); + }; + let current = *next_row_id; + *next_row_id = current.checked_add(self.reserved_row_ids).ok_or_else(|| { + Error::invalid_input(format!( + "reserving {} row ids overflows the row id counter at {current}", + self.reserved_row_ids + )) + })?; + Ok(()) + } +} + +/// Drop the fragments whose data no longer matches what an index recorded. +/// +/// An index built over a field describes the values that were in that field +/// when it was built. Rebinding the field's data in a fragment invalidates the +/// index for that fragment only, so the fragment leaves the bitmap rather than +/// the whole index being discarded. +fn prune_rebound_fields_from_indices( + indices: &mut [IndexMetadata], + rebound: &HashMap>, +) { + if rebound.is_empty() { + return; + } + for index in indices.iter_mut() { + let Some(bitmap) = index.fragment_bitmap.as_mut() else { + continue; + }; + for (fragment_id, fields) in rebound { + if index.fields.iter().any(|field| fields.contains(field)) + && let Ok(fragment_id) = u32::try_from(*fragment_id) + { + bitmap.remove(fragment_id); + } + } + } +} + +fn unbound_token_err(space: &str, token: u32) -> Error { + Error::invalid_input(format!( + "an action references local {space} token {token}, which no earlier action in this \ + operation minted" + )) +} + +fn duplicate_token_err(space: &str, token: u32) -> Error { + Error::invalid_input(format!( + "local {space} token {token} is minted more than once in this operation; tokens must be \ + distinct within an operation" + )) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::DataFile; + use crate::system_index::mem_wal::{ + CompactedSsTable, IndexCatchupProgress, MemWalIndexDetails, load_mem_wal_index_details, + new_mem_wal_index_meta, + }; + use crate::transaction::action::test_support::{added_field, apply, backed_manifest}; + use crate::transaction::action::{ + Action, AddDataFile, AddField, AddFragment, CompositeOperation, TombstoneFieldData, + UserAction, + }; + use crate::transaction::test_support::{default_build_config, sample_index_metadata}; + use crate::transaction::{DataReplacementGroup, Operation}; + use lance_file::version::ConcreteFileVersion; + use roaring::RoaringBitmap; + use rstest::rstest; + use std::sync::Arc; + use uuid::Uuid; + + /// MemWAL coverage invalidation is derived while the manifest is built, so + /// it has to run on the action path as well as the legacy one. A commit + /// that narrows an index below the version it read must lose that index's + /// catch-up entry, or the WAL pod goes on retiring SSTables against a + /// position the index no longer holds. + #[rstest] + #[case::legacy(false)] + #[case::actions(true)] + fn test_narrowing_an_index_invalidates_mem_wal_catchup_on_both_paths( + #[case] via_actions: bool, + ) { + let mut manifest = backed_manifest(); + let mut second = manifest.fragments[0].clone(); + second.id = 1; + second.files[0].path = "data/1.lance".into(); + manifest.fragments = Arc::new(vec![manifest.fragments[0].clone(), second]); + + let shard = Uuid::from_u128(7); + let existing = vec![ + IndexMetadata { + fragment_bitmap: Some(RoaringBitmap::from_iter([0u32, 1])), + ..sample_index_metadata("idx") + }, + new_mem_wal_index_meta( + manifest.version, + MemWalIndexDetails { + compacted_sstables: vec![CompactedSsTable::new(shard, 5)], + index_catchup: vec![IndexCatchupProgress::new( + "idx".to_string(), + vec![CompactedSsTable::new(shard, 5)], + )], + ..Default::default() + }, + ) + .unwrap(), + ]; + + // Replacing field 0's data in fragment 0 prunes that fragment from the + // index, which then no longer spans the version this commit read. The + // two forms below are the same change said twice. + let replacement = DataFile::new( + "data/0-new.lance", + vec![0], + vec![0], + ConcreteFileVersion::V2_0, + None, + None, + ); + let operation = if via_actions { + Operation::CompositeOperation(CompositeOperation::new(vec![UserAction::new( + "replace field 0", + vec![ + Action::TombstoneFieldData(TombstoneFieldData { + fragment: Ref::Committed(0), + field_ids: vec![Ref::Committed(0)], + data_change: true, + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Committed(0), + file: replacement, + field_ids: vec![Ref::Committed(0)], + data_change: true, + }), + ], + )])) + } else { + Operation::DataReplacement { + replacements: vec![DataReplacementGroup(0, replacement)], + } + }; + + let (_, indices) = Transaction::new(manifest.version, operation, None) + .build_manifest_with_read_version( + Some(&manifest), + existing.clone(), + "tx.txn", + &default_build_config(), + Some(ReadVersionState { + manifest: &manifest, + indices: &existing, + }), + ) + .unwrap(); + + let narrowed = indices.iter().find(|index| index.name == "idx").unwrap(); + assert_eq!( + narrowed.fragment_bitmap, + Some(RoaringBitmap::from_iter([1u32])), + "the replacement should have pruned fragment 0 from the index" + ); + + let details = load_mem_wal_index_details( + indices + .iter() + .find(|index| index.name == MEM_WAL_INDEX_NAME) + .expect("the MemWAL index should still be there") + .clone(), + ) + .unwrap(); + assert!( + details.index_catchup.is_empty(), + "an index that no longer covers the read version must lose its catch-up entry, \ + but it kept {:?}", + details.index_catchup + ); + } + + #[test] + fn test_an_action_set_relocates_onto_a_newer_version() { + let actions = vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("added"), + }), + Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 10, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Local(0), + file: DataFile::new_unstarted("data/new.lance", ConcreteFileVersion::V2_0), + field_ids: vec![Ref::Local(0)], + data_change: true, + }), + ]; + + // Replaying the same actions against the manifest the first run + // produced re-resolves both local tokens against the newer counters. + // Only the field's name differs, since the first run took "added". + let first = apply(&backed_manifest(), actions.clone()).unwrap(); + let mut replayed = actions; + let Action::AddField(add_field) = &mut replayed[0] else { + unreachable!("the first action is the AddField built above"); + }; + add_field.def.name = "added_again".into(); + let second = apply(&first, replayed).unwrap(); + + assert_eq!( + second.fragments.iter().map(|f| f.id).collect::>(), + vec![0, 1, 2] + ); + assert_eq!( + second + .schema + .fields_pre_order() + .map(|field| field.id) + .collect::>(), + vec![0, 1, 2] + ); + // The second run's data file points at the field the second run minted, + // not at the one the first run did. + let relocated = second.fragments.iter().find(|f| f.id == 2).unwrap(); + assert_eq!(relocated.files[0].fields.as_ref(), &[2]); + } + + #[test] + fn test_action_set_cannot_create_a_dataset() { + let transaction = Transaction::new( + 0, + Operation::CompositeOperation(CompositeOperation::new(vec![])), + None, + ); + let error = transaction + .build_manifest(None, Vec::new(), "tx.txn", &default_build_config()) + .unwrap_err(); + assert!( + error.to_string().contains("cannot create one"), + "unexpected error: {error}" + ); + } +} diff --git a/rust/lance-table/src/transaction/action/config_update.rs b/rust/lance-table/src/transaction/action/config_update.rs new file mode 100644 index 00000000000..97804ec0063 --- /dev/null +++ b/rust/lance-table/src/transaction/action/config_update.rs @@ -0,0 +1,544 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Apply config and metadata updates. + +use super::apply::ApplyState; +use super::footprint::UnenforcedKey; +use super::proto::required; +use super::{ConfigMap, Coordinate, Footprint, Ref}; +use crate::format::pb; +use crate::transaction::UpdateMap; +use crate::transaction::update_map::apply_update_map; +use lance_core::datatypes::{ + Field, LANCE_UNENFORCED_CLUSTERING_KEY_POSITION, LANCE_UNENFORCED_PRIMARY_KEY, + LANCE_UNENFORCED_PRIMARY_KEY_POSITION, +}; +use lance_core::deepsize::DeepSizeOf; +use lance_core::{Error, Result}; + +/// Apply config and metadata updates. +/// +/// Each of the four string maps a manifest carries is edited by an +/// [`UpdateMap`]: a list of keys to set or delete, or a wholesale replacement. +/// Absent means "leave this map alone", which is why every field is optional. +/// +/// This is reference-stable rather than a delta -- config keys and field ids are +/// stable coordinates -- so two operations editing different keys commute. +#[derive(Debug, Clone, PartialEq, DeepSizeOf, Default)] +pub struct ConfigUpdate { + /// Dataset config. + pub config: Option, + /// Table metadata. + pub table_metadata: Option, + /// Schema-level metadata. + pub schema_metadata: Option, + /// Per-field metadata, in application order. Keyed by [`Ref`] so a field + /// minted earlier in the same operation can be given metadata. + pub field_metadata: Vec, +} + +/// One field's metadata within a [`ConfigUpdate`]. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct FieldMetadataUpdate { + pub field: Ref, + pub updates: UpdateMap, +} + +impl ConfigUpdate { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + if let Some(updates) = &self.config { + apply_update_map(state.config_mut(), updates); + } + if let Some(updates) = &self.table_metadata { + apply_update_map(state.table_metadata_mut(), updates); + } + if let Some(updates) = &self.schema_metadata { + apply_update_map(&mut state.schema_mut().metadata, updates); + } + if self.field_metadata.is_empty() { + return Ok(()); + } + + // The unenforced primary and clustering keys are reserved schema + // properties: each is immutable once set, and its reserved keys cannot + // be written with an invalid value. Capture what they were before the + // updates land so a violation can be rejected below. This runs on every + // apply, including a conflict rebase, so it also catches the + // concurrent-writer race. + let primary_key_before = unenforced_primary_key(state); + let clustering_key_before = unenforced_clustering_key(state); + let writes_primary_key = self.writes_any(&[ + LANCE_UNENFORCED_PRIMARY_KEY, + LANCE_UNENFORCED_PRIMARY_KEY_POSITION, + ]); + let writes_clustering_key = self.writes_any(&[LANCE_UNENFORCED_CLUSTERING_KEY_POSITION]); + + for update in &self.field_metadata { + let field_id = state.resolve_field(update.field)?; + let field = state + .schema_mut() + .field_by_id_mut(field_id) + .ok_or_else(|| { + Error::invalid_input(format!( + "ConfigUpdate names field {field_id}, which does not exist" + )) + })?; + apply_update_map(&mut field.metadata, &update.updates); + refresh_reserved_key_positions(field); + } + + reject_reserved_key_change( + "primary", + &primary_key_before, + &unenforced_primary_key(state), + writes_primary_key, + )?; + reject_reserved_key_change( + "clustering", + &clustering_key_before, + &unenforced_clustering_key(state), + writes_clustering_key, + ) + } + + /// The config and metadata maps hold no row values. + pub(super) fn is_data_change(&self) -> bool { + false + } + + /// The keys this update names, or the whole map when it replaces one. A + /// field's metadata belongs to the field, so an update to it needs the + /// field to still be there when it lands. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + for (map, update) in [ + (ConfigMap::Config, &self.config), + (ConfigMap::TableMetadata, &self.table_metadata), + (ConfigMap::SchemaMetadata, &self.schema_metadata), + ] { + if let Some(update) = update { + footprint.write_map_update(map, update); + } + } + for update in &self.field_metadata { + // A field minted in this operation has no committed id, so no + // concurrent writer can be naming it. A committed field's metadata + // goes with the field: writing it needs the field to still be + // there, and is lost with it if the field is dropped afterwards. + if let Some(id) = update.field.committed() + && let Ok(id) = i32::try_from(id) + { + footprint.write_map_update(ConfigMap::Field(id), &update.updates); + footprint.require_field_definition(update.field); + } + } + // The reserved keys are declared per field but held once per table, so + // two writers declaring one on different fields collide on the key + // itself, not on either field's metadata. + if self.writes_any(&[ + LANCE_UNENFORCED_PRIMARY_KEY, + LANCE_UNENFORCED_PRIMARY_KEY_POSITION, + ]) { + footprint.write(Coordinate::UnenforcedKey(UnenforcedKey::Primary)); + } + if self.writes_any(&[LANCE_UNENFORCED_CLUSTERING_KEY_POSITION]) { + footprint.write(Coordinate::UnenforcedKey(UnenforcedKey::Clustering)); + } + } + + fn writes_any(&self, keys: &[&str]) -> bool { + self.field_metadata.iter().any(|update| { + update + .updates + .update_entries + .iter() + .any(|entry| keys.contains(&entry.key.as_str())) + }) + } +} + +fn unenforced_primary_key(state: &ApplyState) -> Vec { + state + .schema() + .unenforced_primary_key() + .iter() + .map(|field| field.id) + .collect() +} + +fn unenforced_clustering_key(state: &ApplyState) -> Vec { + state + .schema() + .unenforced_clustering_key() + .iter() + .map(|field| field.id) + .collect() +} + +/// A field caches its reserved-key positions alongside the metadata they are +/// parsed from, so the cache has to be rebuilt whenever the metadata changes. +fn refresh_reserved_key_positions(field: &mut Field) { + field.unenforced_primary_key_position = field + .metadata + .get(LANCE_UNENFORCED_PRIMARY_KEY_POSITION) + .and_then(|value| value.parse::().ok()) + .or_else(|| { + field + .metadata + .get(LANCE_UNENFORCED_PRIMARY_KEY) + .filter(|value| matches!(value.to_lowercase().as_str(), "true" | "1" | "yes")) + .map(|_| 0) + }); + field.unenforced_clustering_key_position = field + .metadata + .get(LANCE_UNENFORCED_CLUSTERING_KEY_POSITION) + .and_then(|value| value.parse::().ok()); +} + +fn reject_reserved_key_change( + which: &str, + before: &[i32], + after: &[i32], + writes_reserved_key: bool, +) -> Result<()> { + if !before.is_empty() { + if writes_reserved_key || after != before { + return Err(Error::invalid_input(format!( + "the unenforced {which} key is a reserved key and cannot be changed once set" + ))); + } + } else if writes_reserved_key && after.is_empty() { + // A reserved key was written but installed no valid key, e.g. a + // non-marker flag value or a non-numeric position. + return Err(Error::invalid_input(format!( + "the unenforced {which} key is a reserved key and cannot be set to an invalid value" + ))); + } + Ok(()) +} + +impl From<&ConfigUpdate> for pb::ConfigUpdate { + fn from(value: &ConfigUpdate) -> Self { + Self { + config: value.config.as_ref().map(pb::UpdateMap::from), + table_metadata: value.table_metadata.as_ref().map(pb::UpdateMap::from), + schema_metadata: value.schema_metadata.as_ref().map(pb::UpdateMap::from), + field_metadata: value + .field_metadata + .iter() + .map(|update| pb::config_update::FieldMetadata { + field: Some(update.field.into()), + updates: Some(pb::UpdateMap::from(&update.updates)), + }) + .collect(), + } + } +} + +impl TryFrom for ConfigUpdate { + type Error = Error; + + fn try_from(message: pb::ConfigUpdate) -> Result { + Ok(Self { + config: message.config.as_ref().map(UpdateMap::from), + table_metadata: message.table_metadata.as_ref().map(UpdateMap::from), + schema_metadata: message.schema_metadata.as_ref().map(UpdateMap::from), + field_metadata: message + .field_metadata + .into_iter() + .map(|update| { + Ok(FieldMetadataUpdate { + field: required(update.field, "ConfigUpdate.field_metadata.field")? + .try_into()?, + updates: UpdateMap::from(&required( + update.updates, + "ConfigUpdate.field_metadata.updates", + )?), + }) + }) + .collect::>>()?, + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::transaction::action::test_support::{ + added_field, apply, backed_manifest, footprint, + }; + use crate::transaction::action::{Action, AddField}; + use crate::transaction::update_map::UpdateMapEntry; + + fn merge(entries: &[(&str, Option<&str>)]) -> UpdateMap { + UpdateMap { + update_entries: entries.iter().map(|entry| (*entry).into()).collect(), + replace: false, + } + } + + fn replace(entries: &[(&str, &str)]) -> UpdateMap { + UpdateMap { + update_entries: entries + .iter() + .map(|(key, value)| UpdateMapEntry { + key: (*key).into(), + value: Some((*value).into()), + }) + .collect(), + replace: true, + } + } + + #[test] + fn test_config_update_merges_and_deletes() { + let mut manifest = backed_manifest(); + manifest.config.insert("keep".into(), "yes".into()); + manifest.config.insert("drop".into(), "yes".into()); + + let next = apply( + &manifest, + vec![Action::ConfigUpdate(ConfigUpdate { + config: Some(merge(&[("drop", None), ("added", Some("1"))])), + ..Default::default() + })], + ) + .unwrap(); + + assert_eq!(next.config.get("keep"), Some(&"yes".to_string())); + assert_eq!(next.config.get("added"), Some(&"1".to_string())); + assert!(!next.config.contains_key("drop")); + } + + #[test] + fn test_config_update_replaces_a_whole_map() { + let mut manifest = backed_manifest(); + manifest.table_metadata.insert("old".into(), "yes".into()); + + let next = apply( + &manifest, + vec![Action::ConfigUpdate(ConfigUpdate { + table_metadata: Some(replace(&[("new", "1")])), + ..Default::default() + })], + ) + .unwrap(); + + assert!(!next.table_metadata.contains_key("old")); + assert_eq!(next.table_metadata.get("new"), Some(&"1".to_string())); + } + + #[test] + fn test_config_update_edits_schema_and_field_metadata() { + let next = apply( + &backed_manifest(), + vec![Action::ConfigUpdate(ConfigUpdate { + schema_metadata: Some(merge(&[("schema", Some("yes"))])), + field_metadata: vec![FieldMetadataUpdate { + field: Ref::Committed(0), + updates: merge(&[("comment", Some("the id"))]), + }], + ..Default::default() + })], + ) + .unwrap(); + + assert_eq!(next.schema.metadata.get("schema"), Some(&"yes".to_string())); + let field = next.schema.field_by_id(0).unwrap(); + assert_eq!(field.metadata.get("comment"), Some(&"the id".to_string())); + } + + #[test] + fn test_config_update_can_name_a_field_minted_in_the_same_operation() { + let next = apply( + &backed_manifest(), + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("fresh"), + }), + Action::ConfigUpdate(ConfigUpdate { + field_metadata: vec![FieldMetadataUpdate { + field: Ref::Local(0), + updates: merge(&[("comment", Some("brand new"))]), + }], + ..Default::default() + }), + ], + ) + .unwrap(); + + let field = next.schema.field("fresh").unwrap(); + assert_eq!( + field.metadata.get("comment"), + Some(&"brand new".to_string()) + ); + } + + #[test] + fn test_config_update_rejects_a_missing_field() { + let error = apply( + &backed_manifest(), + vec![Action::ConfigUpdate(ConfigUpdate { + field_metadata: vec![FieldMetadataUpdate { + field: Ref::Committed(7), + updates: merge(&[("comment", Some("nope"))]), + }], + ..Default::default() + })], + ) + .unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + assert!(error.to_string().contains("field 7"), "{error}"); + } + + #[test] + fn test_config_update_sets_then_refuses_to_change_the_primary_key() { + let set_key = |field: u64| { + Action::ConfigUpdate(ConfigUpdate { + field_metadata: vec![FieldMetadataUpdate { + field: Ref::Committed(field), + updates: merge(&[(LANCE_UNENFORCED_PRIMARY_KEY, Some("true"))]), + }], + ..Default::default() + }) + }; + + let next = apply(&backed_manifest(), vec![set_key(0)]).unwrap(); + assert_eq!( + next.schema + .unenforced_primary_key() + .iter() + .map(|field| field.id) + .collect::>(), + vec![0] + ); + + let error = apply(&next, vec![set_key(0)]).unwrap_err(); + assert!( + error.to_string().contains("cannot be changed once set"), + "unexpected error: {error}" + ); + } + + #[test] + fn test_config_update_rejects_an_invalid_primary_key() { + let error = apply( + &backed_manifest(), + vec![Action::ConfigUpdate(ConfigUpdate { + field_metadata: vec![FieldMetadataUpdate { + field: Ref::Committed(0), + updates: merge(&[( + LANCE_UNENFORCED_PRIMARY_KEY_POSITION, + Some("not a number"), + )]), + }], + ..Default::default() + })], + ) + .unwrap_err(); + + assert!( + error + .to_string() + .contains("cannot be set to an invalid value"), + "unexpected error: {error}" + ); + } + + #[test] + fn test_edits_to_different_keys_commute() { + let update = |key: &str| { + vec![Action::ConfigUpdate(ConfigUpdate { + config: Some(merge(&[(key, Some("1"))])), + ..Default::default() + })] + }; + + assert!(!footprint(update("a")).conflicts_with(&footprint(update("b")))); + assert!(footprint(update("a")).conflicts_with(&footprint(update("a")))); + } + + #[test] + fn test_a_replacement_collides_with_any_edit_to_the_same_map() { + let replaced = footprint(vec![Action::ConfigUpdate(ConfigUpdate { + config: Some(replace(&[("a", "1")])), + ..Default::default() + })]); + let merged = footprint(vec![Action::ConfigUpdate(ConfigUpdate { + config: Some(merge(&[("untouched-by-name", Some("1"))])), + ..Default::default() + })]); + let other_map = footprint(vec![Action::ConfigUpdate(ConfigUpdate { + table_metadata: Some(merge(&[("a", Some("1"))])), + ..Default::default() + })]); + + assert!(replaced.conflicts_with(&merged)); + // Two clears of the same map name no key at all, and still collide. + assert!(replaced.conflicts_with(&replaced.clone())); + // A different map is a different coordinate space. + assert!(!replaced.conflicts_with(&other_map)); + } + + #[test] + fn test_dropping_a_field_collides_with_updating_its_metadata() { + use crate::transaction::action::DropField; + + let metadata = footprint(vec![Action::ConfigUpdate(ConfigUpdate { + field_metadata: vec![FieldMetadataUpdate { + field: Ref::Committed(1), + updates: merge(&[("comment", Some("x"))]), + }], + ..Default::default() + })]); + let dropped = footprint(vec![Action::DropField(DropField { + field: Ref::Committed(1), + })]); + let other = footprint(vec![Action::DropField(DropField { + field: Ref::Committed(2), + })]); + + // Metadata landing after the drop names a field that is gone; the + // drop landing after the metadata takes the metadata with the field. + assert!(metadata.conflicts_with(&dropped)); + assert!(!dropped.conflicts_with(&metadata)); + assert!(!other.conflicts_with(&metadata)); + assert!(!metadata.conflicts_with(&other)); + } + + #[test] + fn test_declaring_the_primary_key_on_different_fields_collides() { + let primary_key = |field: u64| { + footprint(vec![Action::ConfigUpdate(ConfigUpdate { + field_metadata: vec![FieldMetadataUpdate { + field: Ref::Committed(field), + updates: merge(&[(LANCE_UNENFORCED_PRIMARY_KEY, Some("true"))]), + }], + ..Default::default() + })]) + }; + let clustering_key = footprint(vec![Action::ConfigUpdate(ConfigUpdate { + field_metadata: vec![FieldMetadataUpdate { + field: Ref::Committed(3), + updates: merge(&[(LANCE_UNENFORCED_CLUSTERING_KEY_POSITION, Some("0"))]), + }], + ..Default::default() + })]); + let comment = footprint(vec![Action::ConfigUpdate(ConfigUpdate { + field_metadata: vec![FieldMetadataUpdate { + field: Ref::Committed(3), + updates: merge(&[("comment", Some("x"))]), + }], + ..Default::default() + })]); + + // The key is one per table, so the fields being different is no help. + assert!(primary_key(1).conflicts_with(&primary_key(2))); + // A different reserved key, or plain metadata, is a different thing. + assert!(!primary_key(1).conflicts_with(&clustering_key)); + assert!(!primary_key(1).conflicts_with(&comment)); + } +} diff --git a/rust/lance-table/src/transaction/action/drop_field.rs b/rust/lance-table/src/transaction/action/drop_field.rs new file mode 100644 index 00000000000..218d027cfd4 --- /dev/null +++ b/rust/lance-table/src/transaction/action/drop_field.rs @@ -0,0 +1,291 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Remove a field from the schema. + +use super::apply::{ApplyState, TOMBSTONED_FIELD}; +use super::proto::required; +use super::{Footprint, Ref}; +use crate::format::pb; +use lance_core::datatypes::Field; +use lance_core::deepsize::DeepSizeOf; +use lance_core::{Error, Result}; +use std::collections::HashSet; + +/// Remove a field from the schema. +/// +/// The field's descendants go with it, since a struct's children cannot outlive +/// it. At apply, any data file left backing no live field is dropped, and any +/// index over a removed field is discarded. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct DropField { + pub field: Ref, +} + +impl DropField { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + let field_id = state.resolve_field(self.field)?; + let field = state.schema().field_by_id(field_id).ok_or_else(|| { + Error::invalid_input(format!( + "DropField names field {field_id}, which does not exist" + )) + })?; + // A struct's children cannot outlive it, so the whole subtree goes. + let mut dropped = HashSet::new(); + collect_subtree_ids(field, &mut dropped); + remove_field(&mut state.schema_mut().fields, field_id); + + // The fields are gone from the schema, so the slots that backed them in + // each data file are dead. Tombstoning rather than rewriting the field + // list keeps a file's remaining columns at the positions they were + // written at; a file left with nothing live is pruned during + // normalization. + for fragment in state.fragments_mut() { + for file in fragment.files.iter_mut() { + if !file.fields.iter().any(|id| dropped.contains(id)) { + continue; + } + let fields = file + .fields + .iter() + .map(|id| { + if dropped.contains(id) { + TOMBSTONED_FIELD + } else { + *id + } + }) + .collect::>(); + file.fields = fields.into(); + } + + let overlaid: Vec = dropped + .iter() + .filter_map(|id| u32::try_from(*id).ok()) + .collect(); + crate::format::overlay::tombstone_overlay_fields(&mut fragment.overlays, &overlaid); + } + + // Indices over a field that no longer exists are discarded wholesale by + // `retain_relevant_indices`, so there is nothing to record here. + Ok(()) + } + + /// Dropping a field discards every value it held. + pub(super) fn is_data_change(&self) -> bool { + true + } + + /// The field's definition and all of its data, which cannot be enumerated, + /// so the removal is recorded as such and matched by field id. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + footprint.remove_field(self.field); + } +} + +fn collect_subtree_ids(field: &Field, out: &mut HashSet) { + out.insert(field.id); + for child in &field.children { + collect_subtree_ids(child, out); + } +} + +/// Remove the field with `field_id` from `fields`, at whatever depth it sits. +fn remove_field(fields: &mut Vec, field_id: i32) { + let before = fields.len(); + fields.retain(|field| field.id != field_id); + if fields.len() != before { + return; + } + for field in fields.iter_mut() { + remove_field(&mut field.children, field_id); + } +} + +impl From<&DropField> for pb::DropField { + fn from(value: &DropField) -> Self { + Self { + field: Some(value.field.into()), + } + } +} + +impl TryFrom for DropField { + type Error = Error; + + fn try_from(message: pb::DropField) -> Result { + Ok(Self { + field: required(message.field, "DropField.field")?.try_into()?, + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::DataFile; + use crate::transaction::action::test_support::{ + added_field, apply, apply_with_indices, backed_manifest, + }; + use crate::transaction::action::{Action, AddField}; + use crate::transaction::test_support::sample_index_metadata; + use arrow_schema::{DataType, Field as ArrowField}; + use lance_file::version::ConcreteFileVersion; + use std::sync::Arc; + + #[test] + fn test_drop_field_removes_it_and_its_data() { + let (next, indices) = apply_with_indices( + &backed_manifest(), + vec![Action::DropField(DropField { + field: Ref::Committed(0), + })], + vec![sample_index_metadata("idx")], + ) + .unwrap(); + + assert!(next.schema.field_by_id(0).is_none()); + // The file backed only the dropped field, so nothing is left of it. + assert!(next.fragments[0].files.is_empty()); + // An index over a field that no longer exists is discarded outright. + assert!(indices.is_empty()); + } + + #[test] + fn test_drop_field_keeps_a_file_with_a_surviving_field() { + let mut manifest = backed_manifest(); + let mut schema_field = added_field("keep"); + schema_field.id = 1; + manifest.schema.fields.push(schema_field); + let mut fragment = manifest.fragments[0].clone(); + fragment.files[0] = DataFile::new( + "data/0.lance", + vec![0, 1], + vec![0, 1], + ConcreteFileVersion::V2_0, + None, + None, + ); + manifest.fragments = Arc::new(vec![fragment]); + + let next = apply( + &manifest, + vec![Action::DropField(DropField { + field: Ref::Committed(0), + })], + ) + .unwrap(); + + assert!(next.schema.field_by_id(0).is_none()); + assert!(next.schema.field_by_id(1).is_some()); + // The surviving field stays at the position it was written at. + assert_eq!( + next.fragments[0].files[0].fields.as_ref(), + &[TOMBSTONED_FIELD, 1] + ); + } + + #[test] + fn test_drop_field_takes_the_whole_subtree() { + let mut manifest = backed_manifest(); + let mut parent = Field::try_from(ArrowField::new("parent", DataType::Int32, true)).unwrap(); + parent.id = 1; + let mut child = added_field("child"); + child.id = 2; + child.parent_id = 1; + parent.children.push(child); + manifest.schema.fields.push(parent); + + let next = apply( + &manifest, + vec![Action::DropField(DropField { + field: Ref::Committed(1), + })], + ) + .unwrap(); + + assert!(next.schema.field_by_id(1).is_none()); + assert!( + next.schema.field_by_id(2).is_none(), + "a struct's children cannot outlive it" + ); + } + + #[test] + fn test_drop_field_rejects_a_missing_field() { + let error = apply( + &backed_manifest(), + vec![Action::DropField(DropField { + field: Ref::Committed(7), + })], + ) + .unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + assert!(error.to_string().contains("field 7"), "{error}"); + } + + #[test] + fn test_drop_field_then_add_a_field_reuses_no_id() { + let next = apply( + &backed_manifest(), + vec![ + Action::DropField(DropField { + field: Ref::Committed(0), + }), + Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("replacement"), + }), + ], + ) + .unwrap(); + + // Field ids come from a monotonic counter, never from the freed id -- + // an old data file naming id 0 must not be read as the new field. + let ids = next + .schema + .fields_pre_order() + .map(|field| field.id) + .collect::>(); + assert_eq!(ids, vec![1]); + } + + #[test] + fn test_drop_field_rejects_a_field_id_out_of_range() { + let error = apply( + &backed_manifest(), + vec![Action::DropField(DropField { + field: Ref::Committed(u64::from(u32::MAX) + 1), + })], + ) + .unwrap_err(); + assert!( + error.to_string().contains("out of range"), + "unexpected message: {error}" + ); + } + + #[test] + fn test_drop_field_can_name_a_field_minted_in_the_same_operation() { + // A squash of "add column" and "drop column" lowers to exactly this, + // and has no committed id to name the field by. + let next = apply( + &backed_manifest(), + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("transient"), + }), + Action::DropField(DropField { + field: Ref::Local(0), + }), + ], + ) + .unwrap(); + + assert!(next.schema.field("transient").is_none()); + } +} diff --git a/rust/lance-table/src/transaction/action/footprint.rs b/rust/lance-table/src/transaction/action/footprint.rs new file mode 100644 index 00000000000..eae41e4d601 --- /dev/null +++ b/rust/lance-table/src/transaction/action/footprint.rs @@ -0,0 +1,799 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! The set of coordinates an action set writes. +//! +//! Two concurrent action sets can both commit when neither writes anything the +//! other writes. This is a structural test over coordinates rather than a +//! matrix over operation pairs, so it stays a single rule as the vocabulary +//! grows -- adding an action means saying which coordinates it writes, not +//! extending an N-by-N table. +//! +//! Footprints are derived from the actions at conflict time and never +//! serialized. A writer cannot pin down what a reader considers a conflict, and +//! the rule can be tightened in a later release without a format change. +//! +//! Which coordinates an action writes is decided by that action, in its own +//! module. This module holds the coordinate space and the comparison. + +use super::{CompositeOperation, Ref}; +use crate::transaction::UpdateMap; +use std::collections::{HashMap, HashSet}; + +/// One thing an action set writes. +/// +/// Only committed coordinates appear. A minted fragment, field, or base has no +/// id in the read version, so no concurrent writer can be naming the same +/// thing; relocation re-resolves it against whatever version wins. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum Coordinate { + /// Whether a committed fragment is still part of the dataset. + FragmentExistence(u64), + /// A committed fragment's deletion file. + FragmentDeletions(u64), + /// The data backing one field within one committed fragment. + FieldData { fragment: u64, field: i32 }, + /// A field's definition in the schema. + FieldDefinition(i32), + /// A field's name. Names only have to be unique among siblings, but a + /// footprint has no schema to place a field among its siblings, so a name + /// coordinates table-wide: two writers adding a `c` under different structs + /// collide as if they had added it at the top level. The cost is a spurious + /// conflict in that case; the alternative is two fields called `c` side by + /// side, which no reader can tell apart. + FieldName(String), + /// A base path's name, where it has one. Names must be unique; an unset + /// name is an absent alias rather than a name shared with every other + /// unnamed base, so it coordinates nothing. + BaseName(String), + /// A base path's location, which the manifest requires to be unique. + BaseLocation(String), + /// One key in one of the manifest's string maps. + ConfigEntry { map: ConfigMap, key: String }, + /// One of the table-wide keys declared through field metadata. Each is set + /// once, on one set of fields, so two writers declaring it on fields of + /// their own collide even though they name different fields' metadata. + UnenforcedKey(UnenforcedKey), +} + +/// One of the string maps a manifest carries. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum ConfigMap { + /// Dataset config. + Config, + /// Table metadata. + TableMetadata, + /// Schema-level metadata. + SchemaMetadata, + /// One field's metadata, by field id. + Field(i32), +} + +/// A key the table declares once, across its fields, through reserved field +/// metadata entries. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum UnenforcedKey { + Primary, + Clustering, +} + +impl Coordinate { + /// The fragment this coordinate lives in, if it is fragment-scoped. + fn fragment(&self) -> Option { + match self { + Self::FragmentExistence(id) | Self::FragmentDeletions(id) => Some(*id), + Self::FieldData { fragment, .. } => Some(*fragment), + Self::FieldDefinition(_) + | Self::FieldName(_) + | Self::BaseName(_) + | Self::BaseLocation(_) + | Self::ConfigEntry { .. } + | Self::UnenforcedKey(_) => None, + } + } + + /// The string map this coordinate is a key in, if it is one. + fn config_map(&self) -> Option<&ConfigMap> { + match self { + Self::ConfigEntry { map, .. } => Some(map), + _ => None, + } + } +} + +/// How an action set touches a coordinate. Ordered weakest to strongest, so +/// that a set touching one coordinate two ways is summarized by the maximum. +/// +/// The whole conflict rule between two sets is the table in +/// [`pair_conflicts`]: one row per mode of the set being committed, one column +/// per mode of the set that committed before it. A new way of touching a +/// coordinate is a new variant here and a new row and column there. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +enum Mode { + /// Reads the coordinate and needs it to still hold what it read. Data + /// written for a committed field requires the field's definition, since + /// the values are encoded in the type it names. Unlike a write, two sets + /// may require the same coordinate -- two readers of one column do not + /// collide. + Requires, + /// Replaces the coordinate. Two of these on one coordinate never commute. + Writes, +} + +/// A part of the manifest an action set touches wholesale, without being able +/// to enumerate the coordinates inside it. Compared against the other set by +/// containment: a region write is a write of every coordinate the region holds. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +enum Region { + /// A committed fragment: its existence, its deletions, and the data of + /// every field in it. + Fragment(u64), + /// One of the manifest's string maps, every key included. + Map(ConfigMap), +} + +impl Region { + fn contains(&self, coordinate: &Coordinate) -> bool { + match self { + Self::Fragment(id) => coordinate.fragment() == Some(*id), + Self::Map(map) => coordinate.config_map() == Some(map), + } + } +} + +/// Whether a claim of mode `committing` collides with a claim of mode +/// `committed` on the same coordinate, where the `committed` set landed after +/// the `committing` set read. +/// +/// | committing \ committed | Writes | Requires | +/// |------------------------|----------|----------| +/// | Writes | conflict | ok | +/// | Requires | conflict | ok | +/// +/// The `Requires` column is all "ok": what the committed set required held when +/// it committed, and it serialized first, so nothing arriving later can +/// retroactively break it. The `Requires` row is the open question, because +/// the committing set read before the other landed. +/// +/// The table is monotone along [`Mode`]'s order in every row and column, which +/// is what lets one coordinate touched two ways by one set be summarized by +/// its strongest mode. +fn pair_conflicts(committing: Mode, committed: Mode) -> bool { + match (committing, committed) { + (Mode::Writes | Mode::Requires, Mode::Writes) => true, + (_, Mode::Requires) => false, + } +} + +/// Everything an action set writes, in one set. +#[derive(Debug, Clone, Default, PartialEq)] +pub struct Footprint { + /// Every coordinate this set touches, with the strongest [`Mode`] it + /// touches it in. + /// + /// There is no region for a dropped field, the way there is for a removed + /// fragment. Dropping a field writes its definition, and anything that + /// depends on the field -- data written for it, its metadata, a child added + /// under it -- requires that definition, so the drop-then-write order is a + /// `Requires`-vs-`Writes` collision. The write-then-drop order is safe, + /// because [`DropField`](super::DropField) applies to every fragment the + /// manifest holds by then, including one a concurrent set just added, and + /// tombstones the field out of each. + claims: HashMap, + /// Regions this set writes wholesale: fragments it removes, string maps it + /// replaces rather than merges into. A region write counts as a write of + /// every coordinate inside the region, and of the region itself -- two sets + /// replacing one map name no key in common, and still collide. + regions: HashMap, + /// Fragments this set needs to still be there, without writing anything a + /// concurrent set could name inside them. Data for a field this set mints, + /// written into a committed fragment, is the case this exists for: the + /// field id is invisible to a concurrent writer, so the cells are not a + /// coordinate, but they are gone if the fragment is. + /// + /// Not a `Requires` claim on the region, because the check is symmetric: a + /// removal that lands second destroys the cells just as surely as one that + /// lands first. And not a write, because it must not collide with a + /// concurrent write to some other coordinate in the same fragment. + required_fragments: HashSet, + /// Whether this set rewrites the table wholesale. Such a set collides with + /// every concurrent set, including one that records no claim at all -- an + /// append's rows would vanish or survive the reset depending on which + /// commit landed first -- so it is a flag rather than a region. + exclusive: bool, +} + +impl Footprint { + /// Whether this set can still commit on top of `committed`, a set that + /// landed after this one read. + /// + /// Coordinates and regions are compared through [`pair_conflicts`], with + /// this set in the committing role and `committed` in the committed role. + /// That is where the one asymmetry lives: a requirement of this set is + /// tested against what `committed` wrote, never the reverse, because + /// `committed` serialized first and nothing arriving later can break what + /// it required. The distinction is not academic. Data written for a field + /// requires the field's definition; a cast of that field writes it. + /// Checking both directions would also reject the cast that arrives + /// *after* the data -- an order [`AlterField`](super::AlterField) already + /// handles, by rebinding the field in every fragment the manifest has by + /// then. + pub fn conflicts_with(&self, committed: &Self) -> bool { + // A wholesale rewrite leaves nothing for a concurrent set to land on -- + // not even an append, whose rows the reset would discard or resurrect + // depending on which commit won. + if self.exclusive || committed.exclusive { + return true; + } + if self.claims_conflict_with(committed) { + return true; + } + self.anchors_removed_by(committed) || committed.anchors_removed_by(self) + } + + /// Whether any coordinate touched by both sets, directly or through a + /// region, collides under [`pair_conflicts`] with this set committing after + /// `committed`. + /// + /// Every pair is enumerated from this side's point of view: our claims + /// against their claims and their regions, their claims against our + /// regions, and our regions against theirs. A region only matches a region + /// that is equal to it; containment is between a region and a coordinate. + fn claims_conflict_with(&self, committed: &Self) -> bool { + let ours_against_theirs = self.claims.iter().flat_map(|(coordinate, ours)| { + let same_coordinate = committed.claims.get(coordinate).copied(); + let containing_regions = committed + .regions + .iter() + .filter(|(region, _)| region.contains(coordinate)) + .map(|(_, theirs)| *theirs); + same_coordinate + .into_iter() + .chain(containing_regions) + .map(move |theirs| (*ours, theirs)) + }); + let theirs_inside_our_regions = committed.claims.iter().flat_map(|(coordinate, theirs)| { + self.regions + .iter() + .filter(|(region, _)| region.contains(coordinate)) + .map(move |(_, ours)| (*ours, *theirs)) + }); + let same_region = self.regions.iter().filter_map(|(region, ours)| { + committed.regions.get(region).map(|theirs| (*ours, *theirs)) + }); + ours_against_theirs + .chain(theirs_inside_our_regions) + .chain(same_region) + .any(|(ours, theirs)| pair_conflicts(ours, theirs)) + } + + /// Whether `other` removes a fragment this set needs to still be there. + fn anchors_removed_by(&self, other: &Self) -> bool { + self.required_fragments + .iter() + .any(|fragment| other.regions.contains_key(&Region::Fragment(*fragment))) + } + + /// Record a claim on `coordinate`, keeping the strongest mode if the set + /// already touches it another way. See [`pair_conflicts`] for why the + /// strongest mode is the right summary. + fn claim(&mut self, coordinate: Coordinate, mode: Mode) { + self.claims + .entry(coordinate) + .and_modify(|current| *current = (*current).max(mode)) + .or_insert(mode); + } + + /// Record that this set writes `coordinate`. See [`Mode::Writes`]. + pub(super) fn write(&mut self, coordinate: Coordinate) { + self.claim(coordinate, Mode::Writes); + } + + /// Record that this set reads `coordinate` and needs it to still hold what + /// it read. See [`Mode::Requires`]. + pub(super) fn require(&mut self, coordinate: Coordinate) { + self.claim(coordinate, Mode::Requires); + } + + /// The data of each field within `fragment`. + /// + /// A field this operation mints writes no coordinate -- no concurrent + /// writer can be naming one -- and neither does a fragment this operation + /// mints. But data written into a committed fragment lives or dies with + /// that fragment whichever kind of field it is for, so the fragment is + /// required to still be there: a concurrent compaction that removed it + /// would otherwise discard the new column's cells for those rows, and the + /// replacement fragment would read them back as null. + /// + /// Every committed field's definition is required. Values are written in + /// the type the schema names, so a concurrent + /// [`AlterField`](super::AlterField) casting it would leave the manifest + /// describing these values as something they are not, and a concurrent + /// [`DropField`](super::DropField) would leave a file carrying data for a + /// field the manifest no longer has. Both write the definition, so both are + /// caught when they land first. Landing second, a cast rebinds the field in + /// every fragment the manifest has by then and a drop tombstones it out of + /// every fragment's files, so neither order needs a symmetric check. + /// + /// This requires the whole definition, not the type alone, so it also + /// rejects a concurrent rename or nullability change that would not have + /// invalidated the data. That matches the granularity of the coordinate: + /// `AlterField` writes one `FieldDefinition` whichever facet it sets. + pub(super) fn write_field_data( + &mut self, + fragment: Ref, + fields: impl IntoIterator, + ) { + let fields: Vec = fields.into_iter().filter_map(committed_field).collect(); + for field in fields.iter().copied() { + self.require(Coordinate::FieldDefinition(field)); + } + let Some(fragment) = fragment.committed() else { + return; + }; + self.require_fragment(fragment); + for field in fields { + self.write(Coordinate::FieldData { fragment, field }); + } + } + + /// A field's entry in the schema. + pub(super) fn write_field_definition(&mut self, field: Ref) { + if let Some(field) = committed_field(field) { + self.write(Coordinate::FieldDefinition(field)); + } + } + + /// Record that this set depends on `field` still being defined as it was, + /// without redefining it: a child added under it, metadata written on it. + pub(super) fn require_field_definition(&mut self, field: Ref) { + if let Some(field) = committed_field(field) { + self.require(Coordinate::FieldDefinition(field)); + } + } + + /// Note that this set only works if `fragment` is still part of the dataset, + /// without claiming anything inside it. + pub(super) fn require_fragment(&mut self, fragment: u64) { + self.required_fragments.insert(fragment); + } + + /// Record that this set removes `fragment` outright: its existence, and + /// with it every coordinate inside it. + pub(super) fn remove_fragment(&mut self, fragment: u64) { + self.write(Coordinate::FragmentExistence(fragment)); + self.regions + .insert(Region::Fragment(fragment), Mode::Writes); + } + + /// Record an edit to one of the manifest's string maps: the keys it names, + /// or the whole map when it replaces rather than merges. + pub(super) fn write_map_update(&mut self, map: ConfigMap, update: &UpdateMap) { + if update.replace { + self.regions.insert(Region::Map(map), Mode::Writes); + return; + } + for entry in &update.update_entries { + self.write(Coordinate::ConfigEntry { + map: map.clone(), + key: entry.key.clone(), + }); + } + } + + /// Mark this set as rewriting the whole table, conflicting with any + /// concurrent set whatsoever. + pub(super) fn take_exclusive(&mut self) { + self.exclusive = true; + } + + /// Record that this set drops `field` from the schema. + /// + /// Only the definition is written, and only the named field's: a footprint + /// has no schema to expand a struct with. A concurrent write to a child of + /// a dropped struct requires the child's definition, which this does not + /// write, so it is not caught here and fails when it is applied against the + /// version where the child no longer exists. + pub(super) fn remove_field(&mut self, field: Ref) { + self.write_field_definition(field); + } +} + +/// A field id a concurrent writer could also be naming, or `None` for a field +/// this operation mints, which no one else can see yet. +fn committed_field(reference: Ref) -> Option { + i32::try_from(reference.committed()?).ok() +} + +impl From<&CompositeOperation> for Footprint { + fn from(composite_operation: &CompositeOperation) -> Self { + let mut footprint = Self::default(); + for action in composite_operation.iter_actions() { + action.footprint(&mut footprint); + } + footprint + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::{BasePath, DataFile}; + use crate::transaction::action::test_support::footprint; + use crate::transaction::action::{ + Action, AddBase, AddDataFile, AddField, AddFragment, AlterField, DropField, RemoveFragment, + SetDeletionFile, TombstoneFieldData, + }; + use arrow_schema::{DataType, Field as ArrowField}; + use lance_core::datatypes::{Field, LogicalType}; + use lance_file::version::ConcreteFileVersion; + use rstest::rstest; + + fn add_fragment(id: Ref) -> Action { + Action::AddFragment(AddFragment { + id, + physical_rows: 10, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }) + } + + fn add_data_file(fragment: Ref, fields: &[i32]) -> Action { + add_data_file_for( + fragment, + fields + .iter() + .map(|field| Ref::Committed(*field as u64)) + .collect(), + ) + } + + fn add_data_file_for(fragment: Ref, field_ids: Vec) -> Action { + Action::AddDataFile(AddDataFile { + fragment, + file: DataFile::new_unstarted("data/x.lance", ConcreteFileVersion::V2_0), + field_ids, + data_change: true, + }) + } + + fn add_field(local: u32, name: &str, parent: Option) -> Action { + Action::AddField(AddField { + local, + parent, + def: Field::try_from(ArrowField::new(name, DataType::Int32, true)).unwrap(), + }) + } + + /// The add-column shape: mint a field, then back it in a committed fragment. + fn add_column(name: &str, fragment: u64) -> Vec { + vec![ + add_field(1, name, None), + add_data_file_for(Ref::Committed(fragment), vec![Ref::Local(1)]), + ] + } + + fn rename_field(field: i32, name: &str) -> Action { + Action::AlterField(AlterField { + field: Ref::Committed(field as u64), + name: Some(name.into()), + logical_type: None, + nullable: None, + }) + } + + fn cast_field(field: i32) -> Action { + Action::AlterField(AlterField { + field: Ref::Committed(field as u64), + name: None, + logical_type: Some(LogicalType::from("int64")), + nullable: None, + }) + } + + fn drop_field(field: i32) -> Action { + Action::DropField(DropField { + field: Ref::Committed(field as u64), + }) + } + + fn tombstone(fragment: u64, fields: &[i32]) -> Action { + Action::TombstoneFieldData(TombstoneFieldData { + fragment: Ref::Committed(fragment), + field_ids: fields.iter().map(|id| Ref::Committed(*id as u64)).collect(), + data_change: true, + }) + } + + fn remove_fragment(fragment: u64) -> Action { + Action::RemoveFragment(RemoveFragment { + fragment: Ref::Committed(fragment), + data_change: true, + }) + } + + fn set_deletion_file(fragment: u64) -> Action { + Action::SetDeletionFile(SetDeletionFile { + fragment, + deletion_file: None, + data_change: true, + }) + } + + fn add_base(local: u32, name: &str, path: &str) -> Action { + Action::AddBase(AddBase { + local, + base: BasePath::new(0, path.into(), Some(name.into()), false), + }) + } + + fn add_unnamed_base(local: u32, path: &str) -> Action { + Action::AddBase(AddBase { + local, + base: BasePath::new(0, path.into(), None, false), + }) + } + + #[test] + fn test_minting_actions_write_nothing() { + let minting = footprint(vec![ + add_fragment(Ref::Local(0)), + add_data_file(Ref::Local(0), &[0]), + ]); + + // Two writers appending at the same time never collide. + assert!(!minting.conflicts_with(&minting.clone())); + } + + #[rstest] + #[case::same_field_in_same_fragment( + vec![tombstone(0, &[1])], + vec![add_data_file(Ref::Committed(0), &[1])], + true, + )] + #[case::different_fields_in_same_fragment( + vec![tombstone(0, &[1])], + vec![add_data_file(Ref::Committed(0), &[2])], + false, + )] + #[case::same_field_in_different_fragments( + vec![tombstone(0, &[1])], + vec![tombstone(1, &[1])], + false, + )] + #[case::deletions_do_not_collide_with_field_data( + vec![set_deletion_file(0)], + vec![tombstone(0, &[1])], + false, + )] + #[case::concurrent_deletes_of_one_fragment( + vec![set_deletion_file(0)], + vec![set_deletion_file(0)], + true, + )] + #[case::removal_swallows_the_whole_fragment( + vec![remove_fragment(0)], + vec![tombstone(0, &[1])], + true, + )] + #[case::removal_leaves_other_fragments_alone( + vec![remove_fragment(0)], + vec![tombstone(1, &[1])], + false, + )] + // Two compactions of one fragment: the second would remove something the + // first already took away. + #[case::removing_the_same_fragment_twice( + vec![remove_fragment(0)], + vec![remove_fragment(0)], + true, + )] + // A new column's cells in a committed fragment are not a coordinate -- the + // field is minted -- but they are gone if the fragment is, whichever order + // the two land in. + #[case::adding_a_column_into_a_fragment_a_concurrent_set_removes( + add_column("c", 7), + vec![remove_fragment(7)], + true, + )] + #[case::adding_a_column_leaves_other_writes_to_the_fragment_alone( + add_column("c", 7), + vec![tombstone(7, &[2]), set_deletion_file(7)], + false, + )] + #[case::same_field_definition( + vec![rename_field(1, "a")], + vec![Action::AlterField(AlterField { field: Ref::Committed(1), name: None, logical_type: None, nullable: Some(true) })], + true, + )] + #[case::different_field_definitions( + vec![Action::AlterField(AlterField { field: Ref::Committed(1), name: None, logical_type: None, nullable: None })], + vec![Action::AlterField(AlterField { field: Ref::Committed(2), name: None, logical_type: None, nullable: None })], + false, + )] + #[case::dropping_a_field_collides_with_altering_it( + vec![drop_field(1)], + vec![Action::AlterField(AlterField { field: Ref::Committed(1), name: None, logical_type: None, nullable: Some(true) })], + true, + )] + #[case::dropping_a_field_leaves_other_fields_alone( + vec![drop_field(1)], + vec![tombstone(0, &[2])], + false, + )] + #[case::dropping_a_field_leaves_deletions_alone( + vec![drop_field(1)], + vec![set_deletion_file(0)], + false, + )] + #[case::dropping_a_field_a_concurrent_append_does_not_write( + vec![drop_field(1)], + vec![add_fragment(Ref::Local(0)), add_data_file(Ref::Local(0), &[2])], + false, + )] + // A cast and data for an unrelated field never interact, in either order. + #[case::casting_a_field_a_concurrent_append_does_not_write( + vec![cast_field(1)], + vec![add_fragment(Ref::Local(0)), add_data_file(Ref::Local(0), &[2])], + false, + )] + // Names coordinate table-wide, whether a writer takes one by adding a + // field or by renaming one to it. + #[case::adding_two_fields_with_the_same_name( + vec![add_field(0, "c", None)], + vec![add_field(0, "c", None)], + true, + )] + #[case::adding_two_fields_with_different_names( + vec![add_field(0, "c", None)], + vec![add_field(0, "d", None)], + false, + )] + #[case::adding_a_field_named_like_a_concurrent_rename( + vec![add_field(0, "c", None)], + vec![rename_field(1, "c")], + true, + )] + #[case::renaming_two_fields_to_the_same_name( + vec![rename_field(1, "c")], + vec![rename_field(2, "c")], + true, + )] + #[case::adding_the_same_name_under_different_parents( + vec![add_field(0, "c", Some(Ref::Committed(1)))], + vec![add_field(0, "c", Some(Ref::Committed(2)))], + true, + )] + #[case::bases_with_the_same_name( + vec![add_base(0, "a", "s3://bucket/one")], + vec![add_base(0, "a", "s3://bucket/two")], + true, + )] + #[case::bases_with_the_same_location( + vec![add_base(0, "a", "s3://bucket/one")], + vec![add_base(0, "b", "s3://bucket/one")], + true, + )] + #[case::unrelated_bases( + vec![add_base(0, "a", "s3://bucket/one")], + vec![add_base(0, "b", "s3://bucket/two")], + false, + )] + // The name is an optional alias. Two writers who both decline to set one + // have not picked the same name, so only their locations coordinate. + #[case::unnamed_bases_at_different_locations( + vec![add_unnamed_base(0, "s3://bucket/one")], + vec![add_unnamed_base(0, "s3://bucket/two")], + false, + )] + #[case::unnamed_bases_at_the_same_location( + vec![add_unnamed_base(0, "s3://bucket/one")], + vec![add_unnamed_base(0, "s3://bucket/one")], + true, + )] + // A reservation is meant to be one writer's alone, but nothing in the + // format enforces that, so claiming a reserved id has to be a write. + #[case::the_same_reserved_fragment_id( + vec![add_fragment(Ref::Committed(1))], + vec![add_fragment(Ref::Committed(1))], + true, + )] + #[case::different_reserved_fragment_ids( + vec![add_fragment(Ref::Committed(1))], + vec![add_fragment(Ref::Committed(2))], + false, + )] + #[case::claiming_a_reserved_id_a_concurrent_set_removes( + vec![add_fragment(Ref::Committed(1))], + vec![remove_fragment(1)], + true, + )] + fn test_conflicts( + #[case] ours: Vec, + #[case] theirs: Vec, + #[case] expected: bool, + ) { + let ours = footprint(ours); + let theirs = footprint(theirs); + assert_eq!(ours.conflicts_with(&theirs), expected); + // The relation has to hold whichever side is asking. + assert_eq!(theirs.conflicts_with(&ours), expected); + } + + /// Pairs whose answer depends on which set committed first, so each order + /// is stated rather than assumed to match the other. + /// + /// Anything that depends on a field -- data written for it, a child added + /// under it -- requires that field's definition, and a requirement is only + /// checked against what committed *after* the set was planned. A cast or a + /// drop that arrives second is the unsafe order: the data was encoded + /// against a definition the manifest no longer carries. Arriving first they + /// are not, because applying a cast rebinds the field in every fragment the + /// manifest has by then, and applying a drop tombstones the field out of + /// every fragment's files -- including one a concurrent set just added. + #[rstest] + #[case::an_append_landing_after_a_cast( + vec![add_fragment(Ref::Local(0)), add_data_file(Ref::Local(0), &[1])], + vec![cast_field(1)], + true, + )] + #[case::a_cast_landing_after_an_append( + vec![cast_field(1)], + vec![add_fragment(Ref::Local(0)), add_data_file(Ref::Local(0), &[1])], + false, + )] + #[case::a_rewrite_landing_after_a_cast( + vec![add_data_file(Ref::Committed(7), &[1])], + vec![cast_field(1)], + true, + )] + #[case::a_cast_landing_after_a_rewrite( + vec![cast_field(1)], + vec![add_data_file(Ref::Committed(7), &[1])], + false, + )] + #[case::an_append_landing_after_a_drop( + vec![add_fragment(Ref::Local(0)), add_data_file(Ref::Local(0), &[1])], + vec![drop_field(1)], + true, + )] + #[case::a_drop_landing_after_an_append( + vec![drop_field(1)], + vec![add_fragment(Ref::Local(0)), add_data_file(Ref::Local(0), &[1])], + false, + )] + #[case::a_rewrite_landing_after_a_drop( + vec![tombstone(0, &[1])], + vec![drop_field(1)], + true, + )] + #[case::a_drop_landing_after_a_rewrite( + vec![drop_field(1)], + vec![tombstone(0, &[1])], + false, + )] + // A child added under a struct needs the struct to still be there; the + // struct dropped afterwards takes the child with it, as intended. + #[case::a_child_landing_after_its_parent_was_dropped( + vec![add_field(0, "child", Some(Ref::Committed(1)))], + vec![drop_field(1)], + true, + )] + #[case::a_parent_dropped_after_a_child_was_added( + vec![drop_field(1)], + vec![add_field(0, "child", Some(Ref::Committed(1)))], + false, + )] + fn test_directional_conflicts( + #[case] ours: Vec, + #[case] committed: Vec, + #[case] expected: bool, + ) { + assert_eq!( + footprint(ours).conflicts_with(&footprint(committed)), + expected + ); + } +} diff --git a/rust/lance-table/src/transaction/action/proto.rs b/rust/lance-table/src/transaction/action/proto.rs new file mode 100644 index 00000000000..a3059657aa2 --- /dev/null +++ b/rust/lance-table/src/transaction/action/proto.rs @@ -0,0 +1,304 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! The envelope around the per-action protobuf encodings. +//! +//! Each action encodes itself, in its own module; this module carries the +//! [`Ref`], [`CompositeOperation`], and [`UserAction`] wrappers, the dispatch over +//! the `oneof`, and the helpers the per-action conversions share. +//! +//! Reading is fail-closed: an action this build does not implement is an error, +//! never a silently skipped element. The commit path collects concurrent +//! transactions with `try_collect`, so a transaction carrying an unknown action +//! must abort the commit rather than be treated as a no-op. + +use super::{Action, CompositeOperation, Ref, UserAction}; +use crate::format::pb; +use lance_core::{Error, Result}; + +/// `data_change` is absent-means-true on the wire, so only the `false` case is +/// written out. +pub(super) fn data_change_to_wire(data_change: bool) -> Option { + (!data_change).then_some(false) +} + +pub(super) fn data_change_from_wire(data_change: Option) -> bool { + data_change.unwrap_or(true) +} + +pub(super) fn required(value: Option, what: &str) -> Result { + value.ok_or_else(|| Error::invalid_input(format!("{what} is required but was not set"))) +} + +impl From for pb::Ref { + fn from(value: Ref) -> Self { + let kind = match value { + Ref::Committed(id) => pb::r#ref::Kind::Committed(id), + Ref::Local(token) => pb::r#ref::Kind::Local(token), + }; + Self { kind: Some(kind) } + } +} + +impl TryFrom for Ref { + type Error = Error; + + fn try_from(message: pb::Ref) -> Result { + match message.kind { + Some(pb::r#ref::Kind::Committed(id)) => Ok(Self::Committed(id)), + Some(pb::r#ref::Kind::Local(token)) => Ok(Self::Local(token)), + None => Err(Error::invalid_input( + "a Ref in an action was empty; it must be either committed or local", + )), + } + } +} + +impl From<&CompositeOperation> for pb::CompositeOperation { + fn from(value: &CompositeOperation) -> Self { + Self { + actions: value.actions.iter().map(pb::UserAction::from).collect(), + } + } +} + +impl TryFrom for CompositeOperation { + type Error = Error; + + fn try_from(message: pb::CompositeOperation) -> Result { + Ok(Self { + actions: message + .actions + .into_iter() + .map(UserAction::try_from) + .collect::>>()?, + }) + } +} + +impl From<&UserAction> for pb::UserAction { + fn from(value: &UserAction) -> Self { + Self { + description: value.description.clone(), + actions: value.actions.iter().map(pb::Action::from).collect(), + } + } +} + +impl TryFrom for UserAction { + type Error = Error; + + fn try_from(message: pb::UserAction) -> Result { + Ok(Self { + description: message.description, + actions: message + .actions + .into_iter() + .map(Action::try_from) + .collect::>>()?, + }) + } +} + +macro_rules! define_action_proto { + ($($variant:ident,)*) => { + impl From<&Action> for pb::Action { + fn from(value: &Action) -> Self { + let action = match value { + $(Action::$variant(action) => pb::action::Action::$variant(action.into()),)* + }; + Self { + action: Some(action), + } + } + } + + impl TryFrom for Action { + type Error = Error; + + fn try_from(message: pb::Action) -> Result { + match message.action { + $(Some(pb::action::Action::$variant(action)) => { + Ok(Self::$variant(action.try_into()?)) + })* + // The specified vocabulary is larger than what is implemented. + // Reject rather than skip: silently dropping an action would + // apply a partial transaction. + Some(other) => Err(Error::not_supported(format!( + "the action-based transaction uses action {other:?}, which is specified \ + but not implemented by this version of Lance", + ))), + None => Err(Error::invalid_input( + "an Action in a user operation was empty", + )), + } + } + } + }; +} + +for_each_action!(define_action_proto); + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::{BasePath, DataFile, DeletionFile, DeletionFileType, RowIdMeta, pb}; + use crate::rowids::version::RowDatasetVersionMeta; + use crate::transaction::UpdateMap; + use crate::transaction::action::{ + AddBase, AddDataFile, AddField, AddFragment, AlterField, ConfigUpdate, DropField, + FieldMetadataUpdate, RemoveFragment, ReserveFragmentIds, ReserveRowIds, ResetTable, + SetDeletionFile, TombstoneFieldData, + }; + use arrow_schema::{DataType, Field as ArrowField}; + use lance_core::datatypes::Field; + use lance_file::version::ConcreteFileVersion; + use std::sync::Arc; + + fn sample_data_file() -> DataFile { + DataFile::new_unstarted("data/1.lance", ConcreteFileVersion::V2_0) + } + + fn all_actions() -> Vec { + vec![ + Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 10, + row_id_meta: Some(RowIdMeta::Inline(vec![1, 2, 3].into())), + last_updated_at_version_meta: Some(RowDatasetVersionMeta::Inline(Arc::from( + [4u8, 5].as_slice(), + ))), + created_at_version_meta: None, + data_change: true, + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Local(0), + file: sample_data_file(), + field_ids: vec![Ref::Committed(1), Ref::Local(3)], + data_change: false, + }), + Action::AddField(AddField { + local: 3, + parent: Some(Ref::Committed(1)), + def: Field::try_from(ArrowField::new("added", DataType::Int32, true)).unwrap(), + }), + Action::AddBase(AddBase { + local: 1, + base: BasePath::new(0, "s3://bucket/x".into(), Some("other".into()), false), + }), + Action::TombstoneFieldData(TombstoneFieldData { + fragment: Ref::Committed(4), + field_ids: vec![Ref::Committed(7), Ref::Committed(8)], + data_change: true, + }), + Action::RemoveFragment(RemoveFragment { + fragment: Ref::Committed(5), + data_change: true, + }), + Action::SetDeletionFile(SetDeletionFile { + fragment: 6, + deletion_file: Some(DeletionFile { + read_version: 3, + id: 9, + file_type: DeletionFileType::Bitmap, + num_deleted_rows: Some(4), + base_id: None, + }), + data_change: true, + }), + Action::AlterField(AlterField { + field: Ref::Committed(2), + name: Some("renamed".into()), + logical_type: Some("int64".into()), + nullable: Some(false), + }), + Action::DropField(DropField { + field: Ref::Committed(3), + }), + Action::ReserveFragmentIds(ReserveFragmentIds { count: 4 }), + Action::ReserveRowIds(ReserveRowIds { count: 40 }), + Action::ResetTable(ResetTable), + Action::ConfigUpdate(ConfigUpdate { + config: Some(UpdateMap { + update_entries: vec![("a", "1").into(), ("b", None).into()], + replace: false, + }), + table_metadata: None, + schema_metadata: Some(UpdateMap { + update_entries: vec![("c", "2").into()], + replace: true, + }), + field_metadata: vec![FieldMetadataUpdate { + field: Ref::Local(3), + updates: UpdateMap { + update_entries: vec![("d", "3").into()], + replace: false, + }, + }], + }), + ] + } + + #[test] + fn test_composite_operation_round_trips() { + let operation = CompositeOperation::new(vec![ + UserAction::new("everything", all_actions()), + UserAction::new("nothing", vec![]), + ]); + + let message = pb::CompositeOperation::from(&operation); + let round_tripped = CompositeOperation::try_from(message).unwrap(); + + assert_eq!(round_tripped, operation); + } + + #[test] + fn test_data_change_is_absent_when_true() { + // Absent means "real change" on the wire, so the common case costs no + // bytes and an old field-less writer is read correctly. + let message = pb::RemoveFragment::from(&RemoveFragment { + fragment: Ref::Committed(1), + data_change: true, + }); + assert_eq!(message.data_change, None); + assert!(RemoveFragment::try_from(message).unwrap().data_change); + } + + #[test] + fn test_unimplemented_action_is_rejected() { + let message = pb::Action { + action: Some(pb::action::Action::RefreshRowVersionMetadata( + pb::RefreshRowVersionMetadata { + fragment_ids: vec![1], + }, + )), + }; + let error = Action::try_from(message).unwrap_err(); + assert!( + matches!(error, Error::NotSupported { .. }), + "expected NotSupported, got {error:?}" + ); + assert!( + error.to_string().contains("not implemented"), + "unexpected message: {error}" + ); + } + + #[test] + fn test_empty_action_is_rejected() { + let error = Action::try_from(pb::Action { action: None }).unwrap_err(); + assert!( + error.to_string().contains("was empty"), + "unexpected message: {error}" + ); + } + + #[test] + fn test_empty_ref_is_rejected() { + let error = Ref::try_from(pb::Ref { kind: None }).unwrap_err(); + assert!( + error.to_string().contains("committed or local"), + "unexpected message: {error}" + ); + } +} diff --git a/rust/lance-table/src/transaction/action/remove_fragment.rs b/rust/lance-table/src/transaction/action/remove_fragment.rs new file mode 100644 index 00000000000..da705392b4f --- /dev/null +++ b/rust/lance-table/src/transaction/action/remove_fragment.rs @@ -0,0 +1,127 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Remove a fragment entirely. + +use super::apply::ApplyState; +use super::proto::{data_change_from_wire, data_change_to_wire, required}; +use super::{Footprint, Ref}; +use crate::format::pb; +use lance_core::deepsize::DeepSizeOf; +use lance_core::{Error, Result}; + +/// Remove a fragment entirely -- every row deleted, or the fragment replaced by +/// compaction. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct RemoveFragment { + pub fragment: Ref, + /// See [`AddFragment::data_change`](super::AddFragment::data_change). + pub data_change: bool, +} + +impl RemoveFragment { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + let fragment_id = state.resolve_fragment(self.fragment)?; + if !state.remove_fragment(fragment_id) { + return Err(Error::invalid_input(format!( + "RemoveFragment targets fragment {fragment_id}, which does not exist" + ))); + } + Ok(()) + } + + pub(super) fn is_data_change(&self) -> bool { + self.data_change + } + + /// Every coordinate inside the fragment, which cannot be enumerated, so the + /// removal is recorded as such and matched by fragment id. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + if let Some(id) = self.fragment.committed() { + footprint.remove_fragment(id); + } + } +} + +impl From<&RemoveFragment> for pb::RemoveFragment { + fn from(value: &RemoveFragment) -> Self { + Self { + fragment: Some(value.fragment.into()), + data_change: data_change_to_wire(value.data_change), + } + } +} + +impl TryFrom for RemoveFragment { + type Error = Error; + + fn try_from(message: pb::RemoveFragment) -> Result { + Ok(Self { + fragment: required(message.fragment, "RemoveFragment.fragment")?.try_into()?, + data_change: data_change_from_wire(message.data_change), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::transaction::action::test_support::{apply, backed_manifest}; + use crate::transaction::action::{Action, AddFragment}; + + #[test] + fn test_remove_fragment() { + let next = apply( + &backed_manifest(), + vec![Action::RemoveFragment(RemoveFragment { + fragment: Ref::Committed(0), + data_change: true, + })], + ) + .unwrap(); + + assert!(next.fragments.is_empty()); + } + + #[test] + fn test_remove_fragment_can_drop_one_minted_in_the_same_operation() { + let next = apply( + &backed_manifest(), + vec![ + Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 10, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }), + Action::RemoveFragment(RemoveFragment { + fragment: Ref::Local(0), + data_change: true, + }), + ], + ) + .unwrap(); + + assert_eq!( + next.fragments.iter().map(|f| f.id).collect::>(), + vec![0] + ); + } + + #[test] + fn test_remove_fragment_rejects_a_missing_fragment() { + let error = apply( + &backed_manifest(), + vec![Action::RemoveFragment(RemoveFragment { + fragment: Ref::Committed(7), + data_change: true, + })], + ) + .unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + assert!(error.to_string().contains("fragment 7"), "{error}"); + } +} diff --git a/rust/lance-table/src/transaction/action/reserve_fragment_ids.rs b/rust/lance-table/src/transaction/action/reserve_fragment_ids.rs new file mode 100644 index 00000000000..33c1f1860c2 --- /dev/null +++ b/rust/lance-table/src/transaction/action/reserve_fragment_ids.rs @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Reserve a range of fragment ids for a later writer. + +use super::Footprint; +use super::apply::ApplyState; +use crate::format::pb; +use lance_core::deepsize::DeepSizeOf; +use lance_core::{Error, Result}; + +/// Reserve a contiguous range of fragment ids from the counter, for a later +/// (possibly distributed) writer to populate. +/// +/// The range starts wherever the counter stands when this action is applied, so +/// the reserving writer learns which ids it got by reading the committed +/// manifest's high-water mark: the range is the `count` ids ending there. +/// Fragments written against the range name those ids as +/// [`Ref::Committed`](super::Ref::Committed). +/// +/// Ending there, not starting where the reserving writer's read version left +/// off: a concurrent commit may have moved the counter in between, and the +/// range is wherever apply found it. A writer that derives its range from the +/// version it read rather than the manifest it committed may name ids that +/// belong to someone else. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct ReserveFragmentIds { + pub count: u32, +} + +impl ReserveFragmentIds { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + state.reserve_fragment_ids(self.count); + Ok(()) + } + + /// A reserved id backs no rows until something is written to it. + pub(super) fn is_data_change(&self) -> bool { + false + } + + /// Nothing. Ids come off a monotonic counter, so two operations reserving + /// at once get disjoint ranges rather than colliding, and a reserved id is + /// not a coordinate until a fragment claims it -- at which point + /// [`AddFragment`](super::AddFragment) writes its existence, so two writers + /// spending the same reservation are caught there. + pub(super) fn footprint(&self, _footprint: &mut Footprint) {} +} + +impl From<&ReserveFragmentIds> for pb::ReserveFragmentIds { + fn from(value: &ReserveFragmentIds) -> Self { + Self { count: value.count } + } +} + +impl TryFrom for ReserveFragmentIds { + type Error = Error; + + fn try_from(message: pb::ReserveFragmentIds) -> Result { + Ok(Self { + count: message.count, + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::transaction::action::test_support::{apply, backed_manifest}; + use crate::transaction::action::{Action, AddFragment, Ref}; + + fn reserve(count: u32) -> Action { + Action::ReserveFragmentIds(ReserveFragmentIds { count }) + } + + fn add_fragment(local: u32) -> Action { + Action::AddFragment(AddFragment { + id: Ref::Local(local), + physical_rows: 10, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }) + } + + #[test] + fn test_reserving_raises_the_high_water_mark_without_adding_fragments() { + let manifest = backed_manifest(); + assert_eq!(manifest.max_fragment_id(), Some(0)); + + let next = apply(&manifest, vec![reserve(3)]).unwrap(); + + // Ids 1, 2 and 3 are now spoken for, but no fragment exists for them. + assert_eq!(next.max_fragment_id(), Some(3)); + assert_eq!(next.fragments.len(), 1); + } + + #[test] + fn test_a_later_mint_skips_the_reserved_range() { + let next = apply(&backed_manifest(), vec![reserve(3), add_fragment(0)]).unwrap(); + + assert_eq!( + next.fragments.iter().map(|f| f.id).collect::>(), + vec![0, 4], + "the minted fragment must not land inside the reserved range" + ); + } + + #[test] + fn test_a_reserved_id_is_usable_by_the_next_operation() { + let reserved = apply(&backed_manifest(), vec![reserve(2)]).unwrap(); + assert_eq!(reserved.max_fragment_id(), Some(2)); + + // The next operation mints past the range rather than into it: the + // reservation holds even though nothing was written against it. + let next = apply(&reserved, vec![add_fragment(0)]).unwrap(); + assert_eq!( + next.fragments.iter().map(|f| f.id).collect::>(), + vec![0, 3] + ); + } + + #[test] + fn test_reserving_nothing_is_a_no_op() { + let manifest = backed_manifest(); + let next = apply(&manifest, vec![reserve(0)]).unwrap(); + + assert_eq!(next.max_fragment_id(), manifest.max_fragment_id()); + } +} diff --git a/rust/lance-table/src/transaction/action/reserve_row_ids.rs b/rust/lance-table/src/transaction/action/reserve_row_ids.rs new file mode 100644 index 00000000000..6b1701f129c --- /dev/null +++ b/rust/lance-table/src/transaction/action/reserve_row_ids.rs @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Reserve a range of stable row ids for a later writer. + +use super::Footprint; +use super::apply::ApplyState; +use crate::format::pb; +use lance_core::deepsize::DeepSizeOf; +use lance_core::{Error, Result}; + +/// Reserve a contiguous range of stable row ids from the counter, for a later +/// writer to populate. +/// +/// The counterpart of [`ReserveFragmentIds`](super::ReserveFragmentIds) for the +/// row id space, and needed alongside it: reserving a fragment id fixes the +/// high half of a row address, but on a dataset with stable row ids an index +/// records row ids instead, and those come off their own counter. +/// +/// The reservation is taken after this operation's own fragments are numbered, +/// so the reserved range is always the `count` ids ending at the committed +/// manifest's `next_row_id`. The reserving writer learns which ids it got by +/// reading that back, and a fragment written against the range carries those +/// ids in its row id sequence, which apply then leaves as it finds them. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct ReserveRowIds { + pub count: u64, +} + +impl ReserveRowIds { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + state.reserve_row_ids(self.count); + Ok(()) + } + + /// A reserved id backs no row until something is written against it. + pub(super) fn is_data_change(&self) -> bool { + false + } + + /// Nothing. Ids come off a monotonic counter, so two operations reserving + /// at once get disjoint ranges rather than colliding. + /// + /// Unlike a fragment id, a reserved row id is never a coordinate: the + /// sequence a fragment carries is opaque to the footprint, so two writers + /// spending the same reservation are not caught. A reservation has to be + /// spent by the writer that made it, and only once. + pub(super) fn footprint(&self, _footprint: &mut Footprint) {} +} + +impl From<&ReserveRowIds> for pb::ReserveRowIds { + fn from(value: &ReserveRowIds) -> Self { + Self { count: value.count } + } +} + +impl TryFrom for ReserveRowIds { + type Error = Error; + + fn try_from(message: pb::ReserveRowIds) -> Result { + Ok(Self { + count: message.count, + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::{Fragment, RowIdMeta}; + use crate::rowids::{RowIdSequence, read_row_ids, write_row_ids}; + use crate::transaction::action::test_support::{apply, backed_manifest}; + use crate::transaction::action::{Action, AddFragment, Ref}; + use crate::transaction::test_support::make_stable_row_id_manifest; + + fn reserve(count: u64) -> Action { + Action::ReserveRowIds(ReserveRowIds { count }) + } + + fn add_fragment(local: u32, physical_rows: u64) -> Action { + Action::AddFragment(AddFragment { + id: Ref::Local(local), + physical_rows, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }) + } + + fn row_id_meta(row_ids: std::ops::Range) -> RowIdMeta { + let sequence = RowIdSequence::try_from_iter(row_ids).unwrap(); + RowIdMeta::Inline(write_row_ids(&sequence).into()) + } + + /// One committed fragment holding row ids 0..10, with the counter at 1000. + fn stable_manifest() -> crate::format::Manifest { + let mut fragment = Fragment::new(1); + fragment.physical_rows = Some(10); + fragment.row_id_meta = Some(row_id_meta(0..10)); + make_stable_row_id_manifest(vec![fragment]) + } + + #[test] + fn test_reserving_raises_the_watermark_without_adding_rows() { + let manifest = stable_manifest(); + assert_eq!(manifest.next_row_id, 1000); + + let next = apply(&manifest, vec![reserve(50)]).unwrap(); + + // Ids 1000..1050 are now spoken for, but no fragment holds them. + assert_eq!(next.next_row_id, 1050); + assert_eq!(next.fragments.len(), 1); + } + + #[test] + fn test_the_reservation_sits_above_the_operations_own_row_ids() { + let next = apply(&stable_manifest(), vec![reserve(50), add_fragment(0, 10)]).unwrap(); + + // The appended fragment numbers from the old watermark and the + // reservation lands on top, so the reserved range is always the + // trailing `count` ids -- whichever order the actions arrived in. + assert_eq!(next.next_row_id, 1060); + let added = next.fragments.iter().find(|f| f.id == 2).unwrap(); + let Some(RowIdMeta::Inline(data)) = &added.row_id_meta else { + panic!("the added fragment should carry an inline row id sequence"); + }; + let row_ids = read_row_ids(data).unwrap().iter().collect::>(); + assert_eq!(row_ids, (1000..1010).collect::>()); + } + + #[test] + fn test_reservations_accumulate() { + let next = apply(&stable_manifest(), vec![reserve(10), reserve(5)]).unwrap(); + + assert_eq!(next.next_row_id, 1015); + } + + #[test] + fn test_reserving_nothing_is_a_no_op() { + let manifest = stable_manifest(); + let next = apply(&manifest, vec![reserve(0)]).unwrap(); + + assert_eq!(next.next_row_id, manifest.next_row_id); + } + + #[test] + fn test_a_fragment_may_carry_reserved_row_ids() { + // The whole point of the action: reserve in one commit, then write data + // (and an index over it) against ids that are already known. + let reserved = apply(&stable_manifest(), vec![reserve(10)]).unwrap(); + assert_eq!(reserved.next_row_id, 1010); + + let next = apply( + &reserved, + vec![Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 10, + row_id_meta: Some(row_id_meta(1000..1010)), + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + })], + ) + .unwrap(); + + // The reservation already moved the counter, so applying the fragment + // leaves it where it is rather than numbering the rows again. + assert_eq!(next.next_row_id, 1010); + let added = next.fragments.iter().find(|f| f.id == 2).unwrap(); + let Some(RowIdMeta::Inline(data)) = &added.row_id_meta else { + panic!("the supplied sequence should have been kept"); + }; + assert_eq!( + read_row_ids(data).unwrap().iter().collect::>(), + (1000..1010).collect::>() + ); + } + + #[test] + fn test_unreserved_row_ids_are_rejected() { + // 1000 onwards is where the counter stands, so these were never set + // aside and the next append would hand them out a second time. + let error = apply( + &stable_manifest(), + vec![Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 10, + row_id_meta: Some(row_id_meta(1000..1010)), + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + })], + ) + .unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error}"); + assert!( + error.to_string().contains("no commit has reserved"), + "{error}" + ); + } + + #[test] + fn test_reserving_without_stable_row_ids_is_rejected() { + // `backed_manifest` has no row id counter to reserve from. + let error = apply(&backed_manifest(), vec![reserve(10)]).unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error}"); + assert!(error.to_string().contains("stable row ids"), "{error}"); + } +} diff --git a/rust/lance-table/src/transaction/action/reset_table.rs b/rust/lance-table/src/transaction/action/reset_table.rs new file mode 100644 index 00000000000..1759b89f628 --- /dev/null +++ b/rust/lance-table/src/transaction/action/reset_table.rs @@ -0,0 +1,192 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Reset the table to an empty state. + +use super::Footprint; +use super::apply::ApplyState; +use crate::format::pb; +use lance_core::Result; +use lance_core::deepsize::DeepSizeOf; + +/// Reset the table to an empty state, in preparation for a fresh schema and +/// data written by later actions in the same operation. +/// +/// This is how a full `Overwrite` / `CREATE OR REPLACE` decomposes. It drops the +/// entire schema, the schema metadata, all fragments and all indices, and +/// preserves the table config, the table metadata and the base paths -- change +/// those with a [`ConfigUpdate`](super::ConfigUpdate) or an +/// [`AddBase`](super::AddBase) in the same operation. +/// +/// The id counters are not reset. A field or fragment added after the reset gets +/// a fresh id, so a stale file naming an old id can never be mistaken for the +/// new table's data. +#[derive(Debug, Clone, PartialEq, DeepSizeOf, Default)] +pub struct ResetTable; + +impl ResetTable { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + state.reset(); + Ok(()) + } + + /// Emptying the table discards every row it held. + pub(super) fn is_data_change(&self) -> bool { + true + } + + /// Everything. A reset writes every coordinate there is, including ones a + /// concurrent set would only mint, so it takes the table exclusively rather + /// than enumerating them. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + footprint.take_exclusive(); + } +} + +impl From<&ResetTable> for pb::ResetTable { + fn from(_value: &ResetTable) -> Self { + Self {} + } +} + +impl TryFrom for ResetTable { + type Error = lance_core::Error; + + fn try_from(_message: pb::ResetTable) -> Result { + Ok(Self) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::DataFile; + use crate::transaction::action::test_support::{ + added_field, apply, apply_with_indices, backed_manifest, + }; + use crate::transaction::action::{Action, AddDataFile, AddField, AddFragment, Ref}; + use crate::transaction::test_support::sample_index_metadata; + use lance_file::version::ConcreteFileVersion; + + fn reset() -> Action { + Action::ResetTable(ResetTable) + } + + #[test] + fn test_reset_empties_the_table() { + let manifest = backed_manifest(); + assert!(!manifest.fragments.is_empty()); + assert!(!manifest.schema.fields.is_empty()); + + let (next, indices) = + apply_with_indices(&manifest, vec![reset()], vec![sample_index_metadata("idx")]) + .unwrap(); + + assert!(next.fragments.is_empty()); + assert!(next.schema.fields.is_empty()); + assert!(indices.is_empty()); + } + + #[test] + fn test_reset_preserves_config_and_base_paths() { + let mut manifest = backed_manifest(); + manifest.config.insert("lance.keep".into(), "yes".into()); + manifest.table_metadata.insert("owner".into(), "me".into()); + + let next = apply(&manifest, vec![reset()]).unwrap(); + + assert_eq!(next.config.get("lance.keep"), Some(&"yes".to_string())); + assert_eq!(next.table_metadata.get("owner"), Some(&"me".to_string())); + } + + #[test] + fn test_reset_then_rebuild_in_one_operation() { + let next = apply( + &backed_manifest(), + vec![ + reset(), + Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("fresh"), + }), + Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 4, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Local(0), + file: DataFile::new_unstarted("data/fresh.lance", ConcreteFileVersion::V2_0), + field_ids: vec![Ref::Local(0)], + data_change: true, + }), + ], + ) + .unwrap(); + + // The rebuilt table holds only what the operation wrote after the reset, + // and its ids continue past the ones the old table used. + assert_eq!(next.schema.fields.len(), 1); + let field = next.schema.field("fresh").unwrap(); + assert_ne!(field.id, 0, "a fresh field must not reuse a dropped id"); + assert_eq!(next.fragments.len(), 1); + assert_ne!(next.fragments[0].id, 0); + assert_eq!(next.fragments[0].files[0].fields.as_ref(), &[field.id]); + } + + #[test] + fn test_reset_conflicts_with_everything() { + use crate::transaction::action::ReserveFragmentIds; + use crate::transaction::action::test_support::footprint; + + let reset = footprint(vec![reset()]); + // Even a pure append, which writes no committed coordinate at all, is + // preempted: its rows would either vanish or survive the reset + // depending on which commit landed first. + let append = footprint(vec![Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 4, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + })]); + + assert!(reset.conflicts_with(&append)); + assert!(append.conflicts_with(&reset)); + assert!(reset.conflicts_with(&reset.clone())); + assert!(!append.conflicts_with(&append.clone())); + + // Nor does a set have to write anything: one that only requires a + // coordinate (a compaction-shaped set, which is not even an insert), + // or records nothing at all, still cannot land on either side of a + // reset. + let requires_only = footprint(vec![ + Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 4, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: false, + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Local(0), + file: DataFile::new_unstarted("data/compacted.lance", ConcreteFileVersion::V2_0), + field_ids: vec![Ref::Committed(0)], + data_change: false, + }), + ]); + let records_nothing = footprint(vec![Action::ReserveFragmentIds(ReserveFragmentIds { + count: 3, + })]); + for other in [requires_only, records_nothing] { + assert!(reset.conflicts_with(&other)); + assert!(other.conflicts_with(&reset)); + } + } +} diff --git a/rust/lance-table/src/transaction/action/set_deletion_file.rs b/rust/lance-table/src/transaction/action/set_deletion_file.rs new file mode 100644 index 00000000000..eea75d4e432 --- /dev/null +++ b/rust/lance-table/src/transaction/action/set_deletion_file.rs @@ -0,0 +1,134 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Set (replace) a fragment's deletion file. + +use super::apply::ApplyState; +use super::proto::{data_change_from_wire, data_change_to_wire}; +use super::{Coordinate, Footprint}; +use crate::format::{DeletionFile, pb}; +use lance_core::deepsize::DeepSizeOf; +use lance_core::{Error, Result}; + +/// Set (replace) a fragment's deletion file. +/// +/// This is reference-stable rather than a delta: the fragment id is committed +/// and physical row offsets never move, so the post-image is unambiguous. The +/// newly-deleted rows -- the delta rebase and conflict detection need -- are +/// derived by diffing against the read version rather than serialized. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct SetDeletionFile { + /// The fragment, by committed id. + /// + /// Unlike its sibling fragment actions this takes no [`Ref`](super::Ref). + /// A deletion file's path is `{fragment_id}-{read_version}-{id}.{suffix}` + /// (see [`deletion_file_path`](crate::io::deletion::deletion_file_path)), + /// so the writer has to know the committed fragment id before it can write + /// the file at all, and a minted id does not exist until apply. + pub fragment: u64, + /// The new deletion file, or `None` to clear the fragment's deletions. + pub deletion_file: Option, + /// See [`AddFragment::data_change`](super::AddFragment::data_change). + pub data_change: bool, +} + +impl SetDeletionFile { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + state + .fragment_mut(self.fragment, "SetDeletionFile")? + .deletion_file = self.deletion_file.clone(); + Ok(()) + } + + pub(super) fn is_data_change(&self) -> bool { + self.data_change + } + + /// The fragment's deletions, which is a distinct coordinate from the data of + /// any field in it: deleting rows and re-encoding a column commute. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + footprint.write(Coordinate::FragmentDeletions(self.fragment)); + } +} + +impl From<&SetDeletionFile> for pb::SetDeletionFile { + fn from(value: &SetDeletionFile) -> Self { + Self { + fragment: value.fragment, + deletion_file: value.deletion_file.as_ref().map(pb::DeletionFile::from), + data_change: data_change_to_wire(value.data_change), + } + } +} + +impl TryFrom for SetDeletionFile { + type Error = Error; + + fn try_from(message: pb::SetDeletionFile) -> Result { + Ok(Self { + fragment: message.fragment, + deletion_file: message + .deletion_file + .map(DeletionFile::try_from) + .transpose()?, + data_change: data_change_from_wire(message.data_change), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::DeletionFileType; + use crate::transaction::action::Action; + use crate::transaction::action::test_support::{apply, backed_manifest}; + + #[test] + fn test_set_deletion_file_sets_and_clears() { + let manifest = backed_manifest(); + let deletion_file = DeletionFile { + read_version: manifest.version, + id: 3, + file_type: DeletionFileType::Array, + num_deleted_rows: Some(2), + base_id: None, + }; + let next = apply( + &manifest, + vec![Action::SetDeletionFile(SetDeletionFile { + fragment: 0, + deletion_file: Some(deletion_file.clone()), + data_change: true, + })], + ) + .unwrap(); + assert_eq!(next.fragments[0].deletion_file, Some(deletion_file)); + + // An absent deletion file is a request to clear it, not a no-op. + let cleared = apply( + &next, + vec![Action::SetDeletionFile(SetDeletionFile { + fragment: 0, + deletion_file: None, + data_change: true, + })], + ) + .unwrap(); + assert_eq!(cleared.fragments[0].deletion_file, None); + } + + #[test] + fn test_set_deletion_file_rejects_a_missing_fragment() { + let error = apply( + &backed_manifest(), + vec![Action::SetDeletionFile(SetDeletionFile { + fragment: 7, + deletion_file: None, + data_change: true, + })], + ) + .unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + } +} diff --git a/rust/lance-table/src/transaction/action/test_support.rs b/rust/lance-table/src/transaction/action/test_support.rs new file mode 100644 index 00000000000..abb303c4dab --- /dev/null +++ b/rust/lance-table/src/transaction/action/test_support.rs @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Fixtures shared by the per-action test modules. + +use super::{Action, CompositeOperation, Footprint, UserAction}; +use crate::format::{DataFile, Fragment, IndexMetadata, Manifest}; +use crate::transaction::test_support::{default_build_config, sample_manifest}; +use crate::transaction::{Operation, Transaction}; +use arrow_schema::{DataType, Field as ArrowField}; +use lance_core::Result; +use lance_core::datatypes::Field; +use lance_file::version::ConcreteFileVersion; +use std::sync::Arc; + +pub(super) fn apply(manifest: &Manifest, actions: Vec) -> Result { + apply_with_indices(manifest, actions, Vec::new()).map(|(manifest, _)| manifest) +} + +pub(super) fn apply_with_indices( + manifest: &Manifest, + actions: Vec, + indices: Vec, +) -> Result<(Manifest, Vec)> { + let transaction = Transaction::new( + manifest.version, + Operation::CompositeOperation(CompositeOperation::new(vec![UserAction::new( + "step", actions, + )])), + None, + ); + transaction.build_manifest(Some(manifest), indices, "tx.txn", &default_build_config()) +} + +/// The footprint of one action set, for pair tests. Every conflict test in the +/// action modules builds its two sides with this, so a pair reads as two lists +/// of actions and an expected answer. +pub(super) fn footprint(actions: Vec) -> Footprint { + Footprint::from(&CompositeOperation::new(vec![UserAction::new( + "step", actions, + )])) +} + +pub(super) fn added_field(name: &str) -> Field { + Field::try_from(ArrowField::new(name, DataType::Int32, true)).unwrap() +} + +/// `sample_manifest` with fragment 0 actually backed by a data file, so the +/// reference-stable actions have something committed to point at. +pub(super) fn backed_manifest() -> Manifest { + let mut manifest = sample_manifest(); + let mut fragment = Fragment::new(0); + fragment.physical_rows = Some(10); + fragment.files.push(DataFile::new( + "data/0.lance", + vec![0], + vec![0], + ConcreteFileVersion::V2_0, + None, + None, + )); + manifest.fragments = Arc::new(vec![fragment]); + manifest +} diff --git a/rust/lance-table/src/transaction/action/tombstone_field_data.rs b/rust/lance-table/src/transaction/action/tombstone_field_data.rs new file mode 100644 index 00000000000..8cf672b6ddd --- /dev/null +++ b/rust/lance-table/src/transaction/action/tombstone_field_data.rs @@ -0,0 +1,228 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Tombstone the data-file binding of committed fields within one fragment. + +use super::apply::{ApplyState, TOMBSTONED_FIELD}; +use super::proto::{data_change_from_wire, data_change_to_wire, required}; +use super::{Footprint, Ref}; +use crate::format::pb; +use lance_core::deepsize::DeepSizeOf; +use lance_core::{Error, Result}; + +/// Tombstone the data-file binding of committed fields within one fragment. +/// +/// Each field's slot in whatever file currently backs it is marked tombstoned, +/// and a file left with no live field is pruned at apply. Data files have no id +/// of their own and a live field is backed by exactly one file, so this is how a +/// column's data is dropped or superseded: re-encoding a column is a tombstone +/// followed by an [`AddDataFile`](super::AddDataFile) for the same field. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct TombstoneFieldData { + pub fragment: Ref, + /// The fields whose current backing is tombstoned. + pub field_ids: Vec, + /// `false` marks a tombstone whose data is re-added by an + /// [`AddDataFile`](super::AddDataFile) for the same fields in the same + /// operation, i.e. a re-encode that moves the bytes without changing any + /// row's value. A tombstone with no matching re-add nulls the column out + /// and is a data change. + pub data_change: bool, +} + +impl TombstoneFieldData { + pub(super) fn apply(&self, state: &mut ApplyState) -> Result<()> { + let fragment_id = state.resolve_fragment(self.fragment)?; + let field_ids = self + .field_ids + .iter() + .map(|field| state.resolve_field(*field)) + .collect::>>()?; + let fragment = state.fragment_mut(fragment_id, "TombstoneFieldData")?; + + for &field_id in &field_ids { + let mut found = false; + for file in fragment.files.iter_mut() { + let Some(position) = file.fields.iter().position(|id| *id == field_id) else { + continue; + }; + let mut fields = file.fields.to_vec(); + fields[position] = TOMBSTONED_FIELD; + file.fields = fields.into(); + found = true; + } + if !found { + return Err(Error::invalid_input(format!( + "TombstoneFieldData names field {field_id}, which no data file in fragment \ + {fragment_id} backs" + ))); + } + } + + // New values for these fields supersede any overlay still shadowing + // them, so the drop is not silently masked by stale overlay cells. + let overlaid: Vec = field_ids + .iter() + .filter_map(|id| u32::try_from(*id).ok()) + .collect(); + crate::format::overlay::tombstone_overlay_fields(&mut fragment.overlays, &overlaid); + + state.rebind_fields(fragment_id, field_ids.iter().copied()); + Ok(()) + } + + pub(super) fn is_data_change(&self) -> bool { + self.data_change + } + + /// The data of each named field in this fragment, and nothing else: another + /// field's data in the same fragment is untouched. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + footprint.write_field_data(self.fragment, self.field_ids.iter().copied()); + } +} + +impl From<&TombstoneFieldData> for pb::TombstoneFieldData { + fn from(value: &TombstoneFieldData) -> Self { + Self { + fragment: Some(value.fragment.into()), + field_ids: value.field_ids.iter().map(|id| (*id).into()).collect(), + data_change: data_change_to_wire(value.data_change), + } + } +} + +impl TryFrom for TombstoneFieldData { + type Error = Error; + + fn try_from(message: pb::TombstoneFieldData) -> Result { + Ok(Self { + fragment: required(message.fragment, "TombstoneFieldData.fragment")?.try_into()?, + field_ids: message + .field_ids + .into_iter() + .map(Ref::try_from) + .collect::>>()?, + data_change: data_change_from_wire(message.data_change), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::DataFile; + use crate::transaction::action::Action; + use crate::transaction::action::test_support::{apply, apply_with_indices, backed_manifest}; + use crate::transaction::test_support::sample_index_metadata; + use lance_file::version::ConcreteFileVersion; + use std::sync::Arc; + + fn tombstone_field_zero() -> Action { + Action::TombstoneFieldData(TombstoneFieldData { + fragment: Ref::Committed(0), + field_ids: vec![Ref::Committed(0)], + data_change: true, + }) + } + + #[test] + fn test_tombstone_field_data_drops_the_backing_file() { + let next = apply(&backed_manifest(), vec![tombstone_field_zero()]).unwrap(); + + // The file backed only field 0, so tombstoning it leaves nothing behind. + assert!(next.fragments[0].files.is_empty()); + } + + #[test] + fn test_tombstone_field_data_keeps_a_file_with_a_live_field() { + let mut manifest = backed_manifest(); + let mut fragment = manifest.fragments[0].clone(); + fragment.files[0] = DataFile::new( + "data/0.lance", + vec![0, 1], + vec![0, 1], + ConcreteFileVersion::V2_0, + None, + None, + ); + manifest.fragments = Arc::new(vec![fragment]); + + let next = apply(&manifest, vec![tombstone_field_zero()]).unwrap(); + + assert_eq!( + next.fragments[0].files[0].fields.as_ref(), + &[TOMBSTONED_FIELD, 1] + ); + } + + #[test] + fn test_tombstone_field_data_prunes_the_fragment_from_covering_indices() { + let (_, indices) = apply_with_indices( + &backed_manifest(), + vec![tombstone_field_zero()], + vec![sample_index_metadata("idx")], + ) + .unwrap(); + + // The index covers field 0, whose data in fragment 0 is now gone. + assert!(indices[0].fragment_bitmap.as_ref().unwrap().is_empty()); + } + + #[test] + fn test_tombstone_field_data_rejects_a_field_no_file_backs() { + let error = apply( + &backed_manifest(), + vec![Action::TombstoneFieldData(TombstoneFieldData { + fragment: Ref::Committed(0), + field_ids: vec![Ref::Committed(7)], + data_change: true, + })], + ) + .unwrap_err(); + + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + assert!(error.to_string().contains("field 7"), "{error}"); + } + + #[test] + fn test_tombstone_field_data_can_name_a_field_minted_in_the_same_operation() { + // A squash of "add column with data" and "re-encode that column" lowers + // to exactly this, and has no committed id to name the field by. + use crate::transaction::action::test_support::added_field; + use crate::transaction::action::{AddDataFile, AddField}; + + let next = apply( + &backed_manifest(), + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: added_field("fresh"), + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Committed(0), + file: DataFile::new_unstarted("data/fresh.lance", ConcreteFileVersion::V1), + field_ids: vec![Ref::Local(0)], + data_change: true, + }), + Action::TombstoneFieldData(TombstoneFieldData { + fragment: Ref::Committed(0), + field_ids: vec![Ref::Local(0)], + data_change: false, + }), + ], + ) + .unwrap(); + + // The file that backed only the minted field is left backing nothing + // and is pruned, so the fragment keeps just its original file. + assert_eq!(next.fragments[0].files.len(), 1); + assert!( + !next.fragments[0] + .files + .iter() + .any(|file| file.path == "data/fresh.lance") + ); + } +} diff --git a/rust/lance-table/src/transaction/conflicts.rs b/rust/lance-table/src/transaction/conflicts.rs index cba1b5fb547..ac1085686d0 100644 --- a/rust/lance-table/src/transaction/conflicts.rs +++ b/rust/lance-table/src/transaction/conflicts.rs @@ -861,6 +861,8 @@ impl PartialEq for Operation { std::mem::discriminant(self) == std::mem::discriminant(other) } (Self::DataOverlay { groups: a }, Self::DataOverlay { groups: b }) => compare_vec(a, b), + (Self::CompositeOperation(a), Self::CompositeOperation(b)) => a == b, + (Self::CompositeOperation(_), _) | (_, Self::CompositeOperation(_)) => false, (Self::DataOverlay { .. }, _) | (_, Self::DataOverlay { .. }) => false, } } diff --git a/rust/lance-table/src/transaction/manifest_build.rs b/rust/lance-table/src/transaction/manifest_build.rs index 6f216f78c06..0bbc069dd94 100644 --- a/rust/lance-table/src/transaction/manifest_build.rs +++ b/rust/lance-table/src/transaction/manifest_build.rs @@ -410,6 +410,23 @@ impl Transaction { config: &ManifestBuildConfig, read_version_state: Option>, ) -> Result<(Manifest, Vec)> { + if let Operation::CompositeOperation(composite_operation) = &self.operation { + let current_manifest = current_manifest.ok_or_else(|| { + Error::invalid_input( + "an action-based transaction describes a change to an existing dataset; \ + it cannot create one", + ) + })?; + return self.build_manifest_from_actions( + composite_operation, + current_manifest, + current_indices, + transaction_file_path, + config, + read_version_state, + ); + } + if config.use_stable_row_ids && config.migration_next_row_id.is_none() && current_manifest @@ -1227,29 +1244,20 @@ impl Transaction { // Base paths are handled in the manifest creation section below final_fragments.extend(maybe_existing_fragments?.clone()); } + Operation::CompositeOperation(_) => { + // Handled by build_manifest_from_actions before this match. + return Err(Error::internal( + "an action-based operation reached the legacy manifest build".to_string(), + )); + } }; - // If a fragment was reserved then it may not belong at the end of the fragments list. - final_fragments.sort_by_key(|frag| frag.id); + Self::normalize_fragments(&mut final_fragments)?; - // Clean up data files that only contain tombstoned fields - Self::remove_tombstoned_data_files(&mut final_fragments); - - // Enforce the newest-last overlay ordering invariant at the write - // boundary. Load normalizes with a sort; this rejects any commit path - // that assembled a fragment's overlays out of order. - for fragment in &final_fragments { - if !fragment.overlays.is_empty() { - crate::format::overlay::verify_overlays_newest_last(&fragment.overlays)?; - } - } - - let user_requested_version = match (&config.storage_format, config.use_legacy_format) { - (Some(storage_format), _) => Some(storage_format.lance_file_format()), - (None, Some(true)) => Some(ConcreteFileVersion::V1), - (None, Some(false)) => Some(ConcreteFileVersion::V2_0), - (None, None) => None, - }; + // If this is an overwrite operation and the user has requested a specific + // version then overwrite with that version. Otherwise, if the user didn't + // request a specific version, then keep whatever version we had before. + let overwrite_storage_format = matches!(self.operation, Operation::Overwrite { .. }); // Applied once the final index list is known, so it sees exactly the // indices this commit publishes rather than what any one operation arm @@ -1263,79 +1271,20 @@ impl Transaction { )?; } - let mut manifest = if let Some(current_manifest) = current_manifest { - // OVERWRITE with initial_bases on existing dataset is not allowed (caught by validation) - // So we always use new_from_previous which preserves base_paths - let mut prev_manifest = - Manifest::new_from_previous(current_manifest, schema, Arc::new(final_fragments)); - - if let (Some(user_requested_version), Operation::Overwrite { .. }) = - (user_requested_version, &self.operation) - { - // If this is an overwrite operation and the user has requested a specific version - // then overwrite with that version. Otherwise, if the user didn't request a specific - // version, then overwrite with whatever version we had before. - prev_manifest.data_storage_format = DataStorageFormat::new(user_requested_version); - } - - prev_manifest - } else { - let data_storage_format = - Self::data_storage_format_from_files(&final_fragments, user_requested_version)?; - Manifest::new( - schema, - Arc::new(final_fragments), - data_storage_format, - reference_paths, - ) - }; - - manifest.tag.clone_from(&self.tag); - - if config.auto_set_feature_flags { - // Internal operations (e.g. CreateIndex) build with the default config, - // which has use_stable_row_ids = false. Without inheriting from the previous - // manifest, apply_feature_flags would clear FLAG_STABLE_ROW_IDS. - let inherited = current_manifest - .map(|m| m.uses_stable_row_ids()) - .unwrap_or(false); - let use_stable_row_ids = config.use_stable_row_ids || inherited; - apply_feature_flags( - &mut manifest, - use_stable_row_ids, - config.disable_transaction_file, - )?; - } - // Set after apply_feature_flags, which resets both flag words -- and a - // `Manifest` only points at its index section, so the flag cannot be - // derived there. - // - // Derived fresh from `final_indices` on every commit, never inherited. - // Every manifest this reaches starts without the covering bit, so there - // is no stale bit to clear. Dropping the last covering index lifts the - // fence by simply not setting it again. Inheriting it from the previous - // manifest instead would make the fence permanent. - // - // Both words: a reader that selects a vector index by membership of - // `fields` would answer a query on a merely-carried column with an index - // keyed on another one, and a writer that treats every entry of `fields` - // as keyed would mismaintain it. - if final_indices - .iter() - .any(|index| !index.covering_fields.is_empty()) - { - manifest.reader_feature_flags |= FLAG_COVERED_INDEX_METADATA; - manifest.writer_feature_flags |= FLAG_COVERED_INDEX_METADATA; - } + let mut manifest = self.assemble_manifest( + current_manifest, + schema, + final_fragments, + &final_indices, + reference_paths, + overwrite_storage_format, + config, + )?; if let Some(current_manifest) = current_manifest { inherit_sticky_feature_flags(&mut manifest, current_manifest)?; } - manifest.set_timestamp(config.timestamp_nanos); - - manifest.update_max_fragment_id(); - match &self.operation { Operation::Overwrite { config_upsert_values: Some(tm), @@ -1521,6 +1470,131 @@ impl Transaction { Ok((manifest, final_indices)) } + /// Put an assembled fragment list into the shape the manifest requires: + /// ordered by id, with no data file left holding only tombstoned fields, and + /// with each fragment's overlays in oldest-to-newest order. + pub(super) fn normalize_fragments(fragments: &mut [Fragment]) -> Result<()> { + // If a fragment was reserved then it may not belong at the end of the list. + fragments.sort_by_key(|frag| frag.id); + + Self::remove_tombstoned_data_files(fragments); + + // Enforce the newest-last overlay ordering invariant at the write + // boundary. Load normalizes with a sort; this rejects any commit path + // that assembled a fragment's overlays out of order. + for fragment in fragments.iter() { + if !fragment.overlays.is_empty() { + crate::format::overlay::verify_overlays_newest_last(&fragment.overlays)?; + } + } + Ok(()) + } + + fn user_requested_version(config: &ManifestBuildConfig) -> Option { + match (&config.storage_format, config.use_legacy_format) { + (Some(storage_format), _) => Some(storage_format.lance_file_format()), + (None, Some(true)) => Some(ConcreteFileVersion::V1), + (None, Some(false)) => Some(ConcreteFileVersion::V2_0), + (None, None) => None, + } + } + + /// Build the manifest itself from an already-decided schema and fragment + /// list, and apply the settings that every operation shares: the tag, + /// feature flags, timestamp, and fragment id watermark. + /// + /// `indices` is the index list this commit publishes, which the covered + /// index fence is derived from. A `Manifest` only points at its index + /// section, so the list cannot be read back off the manifest and is passed + /// alongside the schema and fragments. + /// + /// `overwrite_storage_format` replaces the inherited storage format with the + /// user-requested one, for operations that rewrite the whole dataset. + // The manifest simply has this many inputs, and threading them through a + // parameter struct would only move the list. + #[allow(clippy::too_many_arguments)] + pub(super) fn assemble_manifest( + &self, + current_manifest: Option<&Manifest>, + schema: lance_core::datatypes::Schema, + fragments: Vec, + indices: &[IndexMetadata], + reference_paths: HashMap, + overwrite_storage_format: bool, + config: &ManifestBuildConfig, + ) -> Result { + let user_requested_version = Self::user_requested_version(config); + + let mut manifest = if let Some(current_manifest) = current_manifest { + // OVERWRITE with initial_bases on existing dataset is not allowed (caught by validation) + // So we always use new_from_previous which preserves base_paths + let mut prev_manifest = + Manifest::new_from_previous(current_manifest, schema, Arc::new(fragments)); + + if let (true, Some(user_requested_version)) = + (overwrite_storage_format, user_requested_version) + { + prev_manifest.data_storage_format = DataStorageFormat::new(user_requested_version); + } + + prev_manifest + } else { + let data_storage_format = + Self::data_storage_format_from_files(&fragments, user_requested_version)?; + Manifest::new( + schema, + Arc::new(fragments), + data_storage_format, + reference_paths, + ) + }; + + manifest.tag.clone_from(&self.tag); + + if config.auto_set_feature_flags { + // Internal operations (e.g. CreateIndex) build with the default config, + // which has use_stable_row_ids = false. Without inheriting from the previous + // manifest, apply_feature_flags would clear FLAG_STABLE_ROW_IDS. + let inherited = current_manifest + .map(|m| m.uses_stable_row_ids()) + .unwrap_or(false); + let use_stable_row_ids = config.use_stable_row_ids || inherited; + apply_feature_flags( + &mut manifest, + use_stable_row_ids, + config.disable_transaction_file, + )?; + } + + // Set after apply_feature_flags, which resets both flag words -- and a + // `Manifest` only points at its index section, so the flag cannot be + // derived there. + // + // Derived fresh from the published indices on every commit, never + // inherited. Every manifest this reaches starts without the covering + // bit, so there is no stale bit to clear. Dropping the last covering + // index lifts the fence by simply not setting it again. Inheriting it + // from the previous manifest instead would make the fence permanent. + // + // Both words: a reader that selects a vector index by membership of + // `fields` would answer a query on a merely-carried column with an index + // keyed on another one, and a writer that treats every entry of `fields` + // as keyed would mismaintain it. + if indices + .iter() + .any(|index| !index.covering_fields.is_empty()) + { + manifest.reader_feature_flags |= FLAG_COVERED_INDEX_METADATA; + manifest.writer_feature_flags |= FLAG_COVERED_INDEX_METADATA; + } + + manifest.set_timestamp(config.timestamp_nanos); + + manifest.update_max_fragment_id(); + + Ok(manifest) + } + /// Remove data files that only contain tombstoned fields (-2) /// These files no longer contain any live data and can be safely dropped fn remove_tombstoned_data_files(fragments: &mut [Fragment]) { diff --git a/rust/lance-table/src/transaction/operation.rs b/rust/lance-table/src/transaction/operation.rs index 7612464b3a0..c24dc15a1d0 100644 --- a/rust/lance-table/src/transaction/operation.rs +++ b/rust/lance-table/src/transaction/operation.rs @@ -14,6 +14,7 @@ use crate::format::overlay::DataOverlayFile; use crate::format::{BasePath, DataFile, Fragment, IndexFile, IndexMetadata}; use crate::system_index::mem_wal::CompactedSsTable; use crate::transaction::UpdateMap; +use crate::transaction::action::CompositeOperation; use lance_core::datatypes::Schema; use lance_core::deepsize::DeepSizeOf; use roaring::RoaringBitmap; @@ -219,6 +220,14 @@ pub enum Operation { /// The new base paths to add to the manifest. new_bases: Vec, }, + + /// A Transaction V2 operation: an ordered list of granular actions that + /// commit atomically as one manifest change. + /// + /// Unlike the variants above, this one is not a single named change -- it is + /// the composable form the others decompose into. See + /// [`super::action`] for the vocabulary and its stability caveats. + CompositeOperation(CompositeOperation), } #[derive(Debug, Clone, PartialEq, DeepSizeOf)] @@ -269,6 +278,7 @@ impl std::fmt::Display for Operation { Self::Clone { .. } => write!(f, "Clone"), Self::UpdateMemWalState { .. } => write!(f, "UpdateMemWalState"), Self::UpdateBases { .. } => write!(f, "UpdateBases"), + Self::CompositeOperation(_) => write!(f, "CompositeOperation"), } } } @@ -328,6 +338,7 @@ impl Operation { Self::UpdateMemWalState { .. } => "UpdateMemWalState", Self::Clone { .. } => "Clone", Self::UpdateBases { .. } => "UpdateBases", + Self::CompositeOperation(_) => "CompositeOperation", } } } diff --git a/rust/lance-table/src/transaction/proto.rs b/rust/lance-table/src/transaction/proto.rs index ef6ae7bc5e9..0ce47f74334 100644 --- a/rust/lance-table/src/transaction/proto.rs +++ b/rust/lance-table/src/transaction/proto.rs @@ -12,6 +12,7 @@ use crate::format::key_existence::KeyExistenceFilter; use crate::format::pb; use crate::format::{BasePath, Fragment, IndexFile, IndexMetadata, overlay::DataOverlayFile}; use crate::system_index::mem_wal::CompactedSsTable; +use crate::transaction::action::CompositeOperation; use crate::transaction::{ DataOverlayGroup, DataReplacementGroup, Operation, RewriteGroup, RewrittenIndex, Transaction, UpdateMap, UpdateMapEntry, UpdateMode, UpdatedFragmentOffsets, translate_config_updates, @@ -401,18 +402,14 @@ impl TryFrom for Transaction { .map(DataOverlayGroup::try_from) .collect::>>()?, }, - Some(pb::transaction::Operation::CompositeOperation(_)) => { + Some(pb::transaction::Operation::CompositeOperation(composite_operation)) => { // Action-based transactions (Transaction V2) are an experimental - // wire format (OSS-1530). This version of Lance recognizes the message - // but has no support for it: reject on load, fail-closed. Because - // load_and_sort_new_transactions collects transactions with - // try_collect, a concurrent V2 commit in the conflict window - // aborts the whole commit rather than being silently skipped. - // Do NOT make this parsing lenient. - return Err(Error::not_supported( - "action-based transactions (Transaction V2) are not supported \ - by this version of Lance; please upgrade", - )); + // wire format (OSS-1530). Parsing is fail-closed: an action this build + // does not implement is an error, never a skipped element. + // load_and_sort_new_transactions collects concurrent transactions + // with try_collect, so such a transaction aborts the commit rather + // than being silently treated as a no-op. Do NOT make this lenient. + Operation::CompositeOperation(CompositeOperation::try_from(composite_operation)?) } None => { return Err(Error::internal( @@ -711,6 +708,11 @@ impl From<&Transaction> for pb::Transaction { .collect::>(), }) } + Operation::CompositeOperation(composite_operation) => { + pb::transaction::Operation::CompositeOperation(pb::CompositeOperation::from( + composite_operation, + )) + } }; let transaction_properties = value @@ -815,6 +817,7 @@ mod tests { use super::*; use crate::format::DataFile; use crate::format::overlay::OverlayCoverage; + use crate::transaction::action::{Action, AddFragment, Ref, UserAction}; #[test] fn test_data_overlay_operation_roundtrips() { @@ -861,27 +864,68 @@ mod tests { } #[test] - fn test_composite_operation_rejected_on_load() { - // Action-based transactions (Transaction V2) are an experimental wire - // format that this version of Lance does not support. Loading one must fail closed - // (never be silently skipped or leniently parsed), so that a concurrent - // V2 commit in the conflict window aborts an in-flight commit. + fn test_composite_operation_round_trips_through_transaction() { + let uuid = Uuid::new_v4().to_string(); + let transaction = Transaction { + read_version: 4, + uuid: uuid.clone(), + operation: Operation::CompositeOperation(CompositeOperation::new(vec![ + UserAction::new( + "append batch", + vec![Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 1, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + })], + ), + ])), + tag: None, + transaction_properties: None, + }; + + let message = pb::Transaction::from(&transaction); + // The identity lives on the envelope alone. The operation carries the + // steps, and their descriptions are what keeps history readable. + assert_eq!(message.uuid, uuid); + assert_eq!(message.read_version, 4); + match &message.operation { + Some(pb::transaction::Operation::CompositeOperation(composite_operation)) => { + let descriptions: Vec<&str> = composite_operation + .actions + .iter() + .map(|step| step.description.as_str()) + .collect(); + assert_eq!(descriptions, ["append batch"]); + } + other => panic!("expected CompositeOperation, got {other:?}"), + } + + assert_eq!(Transaction::try_from(message).unwrap(), transaction); + } + + #[test] + fn test_unimplemented_action_fails_closed_on_load() { + // The vocabulary is larger than what is implemented. Loading a + // transaction that uses an unimplemented action must fail rather than + // parse leniently: load_and_sort_new_transactions collects concurrent + // transactions with try_collect, so this aborts an in-flight commit + // instead of letting it proceed against a change it cannot see. let message = pb::Transaction { read_version: 1, uuid: Uuid::new_v4().to_string(), operation: Some(pb::transaction::Operation::CompositeOperation( pb::CompositeOperation { actions: vec![pb::UserAction { - description: "append batch".to_string(), + description: "refresh row versions".to_string(), actions: vec![pb::Action { - action: Some(pb::action::Action::AddFragment(pb::AddFragment { - id: Some(pb::Ref { - kind: Some(pb::r#ref::Kind::Local(0)), - }), - physical_rows: 1, - data_change: Some(true), - ..Default::default() - })), + action: Some(pb::action::Action::RefreshRowVersionMetadata( + pb::RefreshRowVersionMetadata { + fragment_ids: vec![1], + }, + )), }], }], }, diff --git a/rust/lance/src/dataset/tests/dataset_transactions.rs b/rust/lance/src/dataset/tests/dataset_transactions.rs index 572d13524cd..7dd56ebbf51 100644 --- a/rust/lance/src/dataset/tests/dataset_transactions.rs +++ b/rust/lance/src/dataset/tests/dataset_transactions.rs @@ -1723,3 +1723,821 @@ async fn test_covering_commit_fences_the_table_with_a_feature_flag() { "dropping the last covering index must clear the fence" ); } + +mod composite { + //! End-to-end coverage for committing an action set against a real dataset. + //! + //! Each of these is a single commit that does work a named operation would + //! have needed several commits for: a fragment is added and then modified, a + //! field is added and then filled, all inside one version. That is what the + //! action vocabulary buys -- steps that reference each other's minted ids can + //! be squashed into one atomic manifest change. + //! + //! The commit path checks that a referenced data file exists, so these tests + //! point their actions at files the fixture dataset already wrote. The + //! resulting datasets are inspected through their manifests rather than read -- + //! the files hold the wrong columns for where they end up attached. + + use std::sync::Arc; + + use crate::dataset::{CommitBuilder, InsertBuilder, WriteParams}; + use crate::{Dataset, Error, Result}; + use arrow_array::{Int32Array, RecordBatch}; + use arrow_schema::{DataType, Field, Schema}; + use lance_core::datatypes::Field as LanceField; + use lance_table::feature_flags::FLAG_COVERED_INDEX_METADATA; + use lance_table::format::{ + BasePath, DataFile, DeletionFile, DeletionFileType, IndexMetadata, RowIdMeta, + }; + use lance_table::rowids::{RowIdSequence, write_row_ids}; + use lance_table::transaction::action::{ + Action, AddBase, AddDataFile, AddField, AddFragment, AlterField, CompositeOperation, + ConfigUpdate, DropField, Ref, RemoveFragment, ReserveFragmentIds, ReserveRowIds, + ResetTable, SetDeletionFile, TombstoneFieldData, UserAction, + }; + use lance_table::transaction::{Operation, Transaction, UpdateMap, UpdateMapEntry}; + use roaring::RoaringBitmap; + use std::collections::HashSet; + use uuid::Uuid; + + /// A two-fragment dataset, so its two data files can stand in for the files an + /// action set would otherwise have had to write. + async fn test_dataset(enable_stable_row_ids: bool) -> Dataset { + let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)])); + let data = + RecordBatch::try_new(schema, vec![Arc::new(Int32Array::from_iter_values(0..10))]) + .unwrap(); + + InsertBuilder::new("memory://") + .with_params(&WriteParams { + enable_stable_row_ids, + max_rows_per_file: 5, + ..Default::default() + }) + .execute(vec![data]) + .await + .unwrap() + } + + fn existing_data_file(dataset: &Dataset, fragment: usize) -> DataFile { + dataset.fragments()[fragment].files[0].clone() + } + + fn int32_field(name: &str) -> LanceField { + LanceField::try_from(Field::new(name, DataType::Int32, true)).unwrap() + } + + fn composite_txn(read_version: u64, actions: Vec) -> Transaction { + Transaction::new( + read_version, + Operation::CompositeOperation(CompositeOperation::new(vec![UserAction::new( + "step", actions, + )])), + None, + ) + } + + async fn try_commit(dataset: Dataset, actions: Vec) -> Result { + let read_version = dataset.version().version; + CommitBuilder::new(Arc::new(dataset)) + .with_experimental_composite_operations(true) + .execute(composite_txn(read_version, actions)) + .await + } + + async fn commit(dataset: Dataset, actions: Vec) -> Dataset { + try_commit(dataset, actions).await.unwrap() + } + + /// The covering fence is re-derived from the published index list on every + /// commit rather than inherited -- `Manifest::new_from_previous` zeroes both + /// flag words -- so every build path has to re-derive it. See + /// `test_covering_commit_fences_the_table_with_a_feature_flag` for what the + /// fence protects; without this, the first composite operation on a covered + /// table republishes its covering indices with the fence down. + #[tokio::test] + async fn test_a_composite_commit_keeps_the_covering_fence() { + // Two columns, so the index can be keyed on one and merely carry the + // other -- a covering index needs at least one field left indexed. + let schema = Arc::new(Schema::new(vec![ + Field::new("a", DataType::Int32, false), + Field::new("b", DataType::Int32, false), + ])); + let data = RecordBatch::try_new( + schema, + vec![ + Arc::new(Int32Array::from_iter_values(0..10)), + Arc::new(Int32Array::from_iter_values(10..20)), + ], + ) + .unwrap(); + let mut dataset = InsertBuilder::new("memory://") + .with_params(&WriteParams { + max_rows_per_file: 5, + ..Default::default() + }) + .execute(vec![data]) + .await + .unwrap(); + + let index = IndexMetadata { + uuid: Uuid::new_v4(), + name: "covered_idx".to_string(), + fields: vec![0, 1], + covering_fields: vec![1], + dataset_version: dataset.manifest.version, + fragment_bitmap: Some(RoaringBitmap::from_iter([0u32, 1])), + index_details: None, + index_version: 0, + created_at: None, + base_id: None, + files: None, + }; + dataset + .apply_commit( + Transaction::new( + dataset.manifest.version, + Operation::CreateIndex { + new_indices: vec![index], + removed_indices: vec![], + }, + None, + ), + &Default::default(), + &Default::default(), + ) + .await + .unwrap(); + assert_ne!( + dataset.manifest.reader_feature_flags & FLAG_COVERED_INDEX_METADATA, + 0, + "precondition: the covering commit fenced the table" + ); + + // Any action set will do -- the fence is a property of the index list + // this commit publishes, not of what the actions touch. + let dataset = commit( + dataset, + vec![Action::ReserveFragmentIds(ReserveFragmentIds { count: 1 })], + ) + .await; + + assert_ne!( + dataset.manifest.reader_feature_flags & FLAG_COVERED_INDEX_METADATA, + 0, + "an action commit must not lift the covering fence" + ); + assert_ne!( + dataset.manifest.writer_feature_flags & FLAG_COVERED_INDEX_METADATA, + 0, + "an action commit must not lift the writer half of the fence" + ); + } + + /// Transaction V2 has no compatibility contract, so a caller has to say so + /// before a composite operation is written into a dataset's history. + #[tokio::test] + async fn test_committing_a_composite_operation_requires_opting_in() { + let dataset = test_dataset(false).await; + let read_version = dataset.version().version; + + let error = CommitBuilder::new(Arc::new(dataset)) + .execute(composite_txn( + read_version, + vec![Action::RemoveFragment(RemoveFragment { + fragment: Ref::Committed(0), + data_change: true, + })], + )) + .await + .unwrap_err(); + + assert!(matches!(error, Error::NotSupported { .. }), "{error:?}"); + assert!( + error + .to_string() + .contains("with_experimental_composite_operations"), + "{error}" + ); + } + + /// Version independence: the same action set, committed after losing any + /// number of races, lands the same change it would have landed alone. + /// + /// This is the property [`Ref::Local`] exists for. Each replay re-resolves + /// the local tokens against whatever version won, so the ids come out + /// different every time while the shape -- which field the minted fragment + /// carries data for, and where it sits in the schema -- does not. A writer + /// that stamped ids in itself would either collide with a racer or silently + /// bind its data to the racer's field. + #[rstest::rstest] + #[case::uncontended(0)] + #[case::one_loss(1)] + #[case::three_losses(3)] + #[tokio::test] + async fn test_an_action_set_lands_the_same_change_however_many_races_it_loses( + #[case] losses: usize, + ) { + let dataset = Arc::new(test_dataset(false).await); + let read_version = dataset.version().version; + let fragments_before = dataset.fragments().len(); + let borrowed_file = existing_data_file(&dataset, 0); + + // Each racer mints in the same three id spaces this action set does, so + // every one of them moves the ids it would otherwise have taken. + for racer in 0..losses { + CommitBuilder::new(dataset.clone()) + .with_experimental_composite_operations(true) + .execute(composite_txn( + dataset.version().version, + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: int32_field(&format!("racer_{racer}")), + }), + Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 5, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }), + ], + )) + .await + .unwrap(); + } + + // Still reading the version it was built against, so it has to relocate + // onto everything the racers committed. + let committed = CommitBuilder::new(dataset) + .with_experimental_composite_operations(true) + .execute(composite_txn( + read_version, + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: int32_field("mine"), + }), + Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 5, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Local(0), + file: borrowed_file, + field_ids: vec![Ref::Local(0)], + data_change: true, + }), + ], + )) + .await + .unwrap(); + + // The change itself: one new field named "mine", one new fragment, and + // that fragment's one data file backing exactly that field. None of it + // depends on how many commits landed in between. + let mine = committed + .schema() + .fields + .iter() + .find(|field| field.name == "mine") + .expect("the field this action set added should be in the schema"); + assert_eq!( + committed.fragments().len(), + fragments_before + losses + 1, + "every racer and this set should each have added one fragment" + ); + + let minted = committed.fragments().last().unwrap(); + assert_eq!(minted.files.len(), 1); + assert_eq!( + minted.files[0].fields.as_ref(), + &[mine.id], + "the data file must be bound to the field this set minted, not a racer's" + ); + + // Distinct ids all round: relocation re-resolved rather than reused. + let field_ids: HashSet = committed.schema().fields.iter().map(|f| f.id).collect(); + assert_eq!(field_ids.len(), committed.schema().fields.len()); + let fragment_ids: HashSet = committed.fragments().iter().map(|f| f.id).collect(); + assert_eq!(fragment_ids.len(), committed.fragments().len()); + } + + /// The rest of the vocabulary, one commit each. These carry no + /// cross-referencing between steps -- that is covered above -- and exist so + /// that every action is exercised through the real commit path, not only + /// through `build_manifest`. + #[tokio::test] + async fn test_one_commit_adds_a_base() { + let dataset = test_dataset(false).await; + + let dataset = commit( + dataset, + vec![Action::AddBase(AddBase { + local: 0, + base: BasePath::new( + 0, + "s3://elsewhere/data".into(), + Some("staging".into()), + false, + ), + })], + ) + .await; + + let base = dataset + .manifest() + .base_paths + .values() + .find(|base| base.name.as_deref() == Some("staging")) + .expect("the base should be in the manifest"); + assert_eq!(base.path, "s3://elsewhere/data"); + } + + #[tokio::test] + async fn test_one_commit_removes_a_fragment() { + let dataset = test_dataset(false).await; + let removed = dataset.fragments()[0].id; + + let dataset = commit( + dataset, + vec![Action::RemoveFragment(RemoveFragment { + fragment: Ref::Committed(removed), + data_change: true, + })], + ) + .await; + + assert_eq!(dataset.fragments().len(), 1); + assert!(dataset.fragments().iter().all(|f| f.id != removed)); + } + + #[tokio::test] + async fn test_one_commit_sets_a_deletion_file() { + let dataset = test_dataset(false).await; + let fragment_id = dataset.fragments()[0].id; + let read_version = dataset.version().version; + + let dataset = commit( + dataset, + vec![Action::SetDeletionFile(SetDeletionFile { + fragment: fragment_id, + deletion_file: Some(DeletionFile { + read_version, + id: 1, + file_type: DeletionFileType::Array, + num_deleted_rows: Some(2), + base_id: None, + }), + data_change: true, + })], + ) + .await; + + let deletion = dataset.fragments()[0] + .deletion_file + .as_ref() + .expect("the fragment should carry a deletion file"); + assert_eq!(deletion.num_deleted_rows, Some(2)); + assert_eq!(deletion.read_version, read_version); + } + + #[tokio::test] + async fn test_one_commit_alters_a_field() { + let dataset = test_dataset(false).await; + let field_id = dataset.schema().fields[0].id; + + let dataset = commit( + dataset, + vec![Action::AlterField(AlterField { + field: Ref::Committed(field_id as u64), + name: Some("renamed".into()), + logical_type: None, + nullable: Some(true), + })], + ) + .await; + + let field = dataset.schema().field_by_id(field_id).unwrap(); + assert_eq!(field.name, "renamed"); + assert!(field.nullable); + } + + #[tokio::test] + async fn test_one_commit_resets_the_table() { + let dataset = test_dataset(false).await; + assert_eq!(dataset.fragments().len(), 2); + + let dataset = commit(dataset, vec![Action::ResetTable(ResetTable)]).await; + + assert!(dataset.fragments().is_empty()); + assert_eq!(dataset.count_rows(None).await.unwrap(), 0); + } + + #[tokio::test] + async fn test_one_commit_updates_the_config() { + let dataset = test_dataset(false).await; + + let dataset = commit( + dataset, + vec![Action::ConfigUpdate(ConfigUpdate { + config: Some(UpdateMap { + update_entries: vec![UpdateMapEntry { + key: "owner".into(), + value: Some("analytics".into()), + }], + replace: false, + }), + table_metadata: None, + schema_metadata: None, + field_metadata: Vec::new(), + })], + ) + .await; + + assert_eq!( + dataset.manifest().config.get("owner").map(String::as_str), + Some("analytics") + ); + } + + /// `SetDeletionFile` carries an absolute post-image -- the whole deletion + /// file, computed against the read version -- and apply writes it in + /// blindly. That is only safe because two sets touching the same fragment's + /// deletions collide on `Coordinate::FragmentDeletions`: without the + /// conflict, the loser's deletions would be silently dropped rather than + /// recomputed. This pins that. + #[tokio::test] + async fn test_two_action_sets_setting_the_same_deletion_file_conflict() { + let dataset = Arc::new(test_dataset(false).await); + let read_version = dataset.version().version; + let fragment_id = dataset.fragments()[0].id; + + let delete = |id: u64| { + composite_txn( + read_version, + vec![Action::SetDeletionFile(SetDeletionFile { + fragment: fragment_id, + deletion_file: Some(DeletionFile { + read_version, + id, + file_type: DeletionFileType::Array, + num_deleted_rows: Some(1), + base_id: None, + }), + data_change: true, + })], + ) + }; + + CommitBuilder::new(dataset.clone()) + .with_experimental_composite_operations(true) + .execute(delete(1)) + .await + .unwrap(); + + let error = CommitBuilder::new(dataset) + .with_experimental_composite_operations(true) + .with_max_retries(0) + .execute(delete(2)) + .await + .unwrap_err(); + + assert!( + matches!(error, Error::RetryableCommitConflict { .. }), + "{error:?}" + ); + } + + #[tokio::test] + async fn test_one_commit_adds_a_fragment_and_then_modifies_it() { + let dataset = test_dataset(false).await; + let before = dataset.version().version; + let first_file = existing_data_file(&dataset, 0); + let second_file = existing_data_file(&dataset, 1); + let second_path = second_file.path.clone(); + + let dataset = commit( + dataset, + vec![ + Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 4, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Local(0), + file: first_file, + field_ids: vec![Ref::Committed(0)], + data_change: true, + }), + // The same commit then replaces the data it just added, naming the + // fragment by the token it was minted under. + Action::TombstoneFieldData(TombstoneFieldData { + fragment: Ref::Local(0), + field_ids: vec![Ref::Committed(0)], + data_change: true, + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Local(0), + file: second_file, + field_ids: vec![Ref::Committed(0)], + data_change: true, + }), + ], + ) + .await; + + assert_eq!(dataset.version().version, before + 1); + let fragments = dataset.fragments(); + assert_eq!(fragments.len(), 3); + let added = fragments.last().unwrap(); + assert_eq!(added.physical_rows, Some(4)); + // The tombstoned file is gone; only the replacement survives the commit. + let paths = added + .files + .iter() + .map(|file| file.path.clone()) + .collect::>(); + assert_eq!(paths, vec![second_path]); + } + + #[tokio::test] + async fn test_one_commit_adds_a_field_and_then_fills_it() { + let dataset = test_dataset(false).await; + let fragment_id = dataset.fragments()[0].id; + let file = existing_data_file(&dataset, 1); + let path = file.path.clone(); + + let dataset = commit( + dataset, + vec![ + Action::AddField(AddField { + local: 0, + parent: None, + def: lance_core::datatypes::Field::try_from(Field::new( + "b", + DataType::Int32, + true, + )) + .unwrap(), + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Committed(fragment_id), + file, + field_ids: vec![Ref::Local(0)], + data_change: true, + }), + ], + ) + .await; + + let field = dataset.schema().field("b").expect("field b was added"); + let fragment = &dataset.fragments()[0]; + let added = fragment + .files + .iter() + .find(|file| file.path == path) + .expect("the new field's data file was attached"); + // The file points at the id the commit minted, which the caller never knew. + assert_eq!(added.fields.as_ref(), &[field.id]); + } + + #[tokio::test] + async fn test_one_commit_swaps_a_field_for_a_new_one() { + let dataset = test_dataset(false).await; + let fragment_id = dataset.fragments()[0].id; + let file = existing_data_file(&dataset, 1); + + let dataset = commit( + dataset, + vec![ + Action::DropField(DropField { + field: Ref::Committed(0), + }), + Action::AddField(AddField { + local: 0, + parent: None, + def: lance_core::datatypes::Field::try_from(Field::new( + "a", + DataType::Int64, + true, + )) + .unwrap(), + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Committed(fragment_id), + file, + field_ids: vec![Ref::Local(0)], + data_change: true, + }), + ], + ) + .await; + + // Dropping "a" and adding a new "a" of a different type is one version, and + // the new field gets a fresh id rather than inheriting the dropped one. + let schema = dataset.schema(); + assert_eq!(schema.fields.len(), 1); + let field = schema.field("a").unwrap(); + assert_ne!(field.id, 0); + assert_eq!(field.logical_type.to_string(), "int64"); + } + + #[tokio::test] + async fn test_one_commit_assigns_row_ids_to_the_fragments_it_mints() { + let dataset = test_dataset(true).await; + let next_row_id = dataset.manifest().next_row_id; + assert_eq!(next_row_id, 10); + + let dataset = commit( + dataset, + vec![ + Action::AddFragment(AddFragment { + id: Ref::Local(0), + physical_rows: 4, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }), + Action::AddFragment(AddFragment { + id: Ref::Local(1), + physical_rows: 6, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }), + ], + ) + .await; + + assert_eq!(dataset.manifest().next_row_id, 20); + for fragment in dataset.fragments().iter().skip(2) { + assert!( + fragment.row_id_meta.is_some(), + "fragment {} was minted without row ids", + fragment.id + ); + assert!(fragment.created_at_version_meta.is_some()); + } + } + + /// The flow a writer needs when it has to know ids before it commits -- + /// because it is baking them into an index it writes in the same commit as + /// the data. One reservation commit, then one commit carrying both. + #[tokio::test] + async fn test_a_reservation_lets_a_later_commit_name_its_ids() { + let dataset = test_dataset(true).await; + let file = existing_data_file(&dataset, 0); + assert_eq!(dataset.manifest.max_fragment_id, Some(1)); + assert_eq!(dataset.manifest.next_row_id, 10); + + let reserved = commit( + dataset, + vec![ + Action::ReserveFragmentIds(ReserveFragmentIds { count: 1 }), + Action::ReserveRowIds(ReserveRowIds { count: 5 }), + ], + ) + .await; + + // The writer reads its range off the committed high-water marks: one + // fragment id ending at 2, and five row ids ending at 15. + assert_eq!(reserved.manifest.max_fragment_id, Some(2)); + assert_eq!(reserved.manifest.next_row_id, 15); + + let written = commit( + reserved, + vec![ + Action::AddFragment(AddFragment { + id: Ref::Committed(2), + physical_rows: 5, + row_id_meta: Some(RowIdMeta::Inline( + write_row_ids(&RowIdSequence::from(10u64..15)).into(), + )), + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + }), + Action::AddDataFile(AddDataFile { + fragment: Ref::Committed(2), + file, + field_ids: vec![Ref::Committed(0)], + data_change: true, + }), + ], + ) + .await; + + assert_eq!( + written.fragments().iter().map(|f| f.id).collect::>(), + vec![0, 1, 2] + ); + // Both reservations were consumed rather than added to: the fragment + // landed on the id it named, and the rows kept the ids they arrived + // with instead of being numbered again. + assert_eq!(written.manifest.max_fragment_id, Some(2)); + assert_eq!(written.manifest.next_row_id, 15); + } + + #[tokio::test] + async fn test_two_action_sets_on_disjoint_coordinates_both_commit() { + let dataset = Arc::new(test_dataset(false).await); + let read_version = dataset.version().version; + + let append = |local| { + vec![Action::AddFragment(AddFragment { + id: Ref::Local(local), + physical_rows: 4, + row_id_meta: None, + last_updated_at_version_meta: None, + created_at_version_meta: None, + data_change: true, + })] + }; + + let first = CommitBuilder::new(dataset.clone()) + .with_experimental_composite_operations(true) + .execute(Transaction::new( + read_version, + Operation::CompositeOperation(CompositeOperation::new(vec![UserAction::new( + "step", + append(0), + )])), + None, + )) + .await + .unwrap(); + + // The second commit still reads the original version, so it has to be + // checked against the first. Both only mint, so neither writes anything + // the other does. + let second = CommitBuilder::new(dataset) + .with_experimental_composite_operations(true) + .execute(Transaction::new( + read_version, + Operation::CompositeOperation(CompositeOperation::new(vec![UserAction::new( + "step", + append(0), + )])), + None, + )) + .await + .unwrap(); + + assert_eq!(second.version().version, first.version().version + 1); + assert_eq!(second.fragments().len(), 4); + } + + #[tokio::test] + async fn test_two_action_sets_writing_the_same_field_data_conflict() { + let dataset = Arc::new(test_dataset(false).await); + let read_version = dataset.version().version; + let fragment_id = dataset.fragments()[0].id; + + let tombstone = || { + Transaction::new( + read_version, + Operation::CompositeOperation(CompositeOperation::new(vec![UserAction::new( + "step", + vec![Action::TombstoneFieldData(TombstoneFieldData { + fragment: Ref::Committed(fragment_id), + field_ids: vec![Ref::Committed(0)], + data_change: true, + })], + )])), + None, + ) + }; + + CommitBuilder::new(dataset.clone()) + .with_experimental_composite_operations(true) + .execute(tombstone()) + .await + .unwrap(); + + let error = CommitBuilder::new(dataset) + .with_experimental_composite_operations(true) + .with_max_retries(0) + .execute(tombstone()) + .await + .unwrap_err(); + assert!( + error.to_string().contains("preempted"), + "unexpected error: {error}" + ); + } +} diff --git a/rust/lance/src/dataset/write/commit.rs b/rust/lance/src/dataset/write/commit.rs index c653ee7d8a3..78d0767b934 100644 --- a/rust/lance/src/dataset/write/commit.rs +++ b/rust/lance/src/dataset/write/commit.rs @@ -9,6 +9,7 @@ use lance_file::version::{ConcreteFileVersion, LanceFileVersion}; use lance_io::object_store::{ObjectStore, ObjectStoreParams}; use lance_select::RowAddrTreeMap; use lance_table::{ + feature_flags::{ENABLE_UNSTABLE_TRANSACTION_V2_ENV, transaction_v2_enabled}, format::{DataStorageFormat, is_detached_version}, io::commit::{CommitConfig, CommitHandler, ManifestNamingScheme}, }; @@ -54,6 +55,8 @@ pub struct CommitBuilder<'a> { timeout: Option, /// When `Some`, this commit is the second step of `migrate_to_stable_row_ids`. migration_next_row_id: Option, + /// Whether the caller has opted in to committing a [`CompositeOperation`]. + allow_composite_operations: bool, } /// Default timeout applied to [`CommitBuilder::execute`] when none is set. @@ -78,6 +81,7 @@ impl<'a> CommitBuilder<'a> { transaction_properties: None, timeout: Some(DEFAULT_COMMIT_TIMEOUT), migration_next_row_id: None, + allow_composite_operations: false, } } @@ -274,7 +278,67 @@ impl<'a> CommitBuilder<'a> { self } + /// Opt in to committing an action-based transaction. + /// + /// Transaction V2 -- a [`Transaction`] whose operation is a + /// [`CompositeOperation`](lance_table::transaction::action::CompositeOperation) + /// -- is an experimental format feature. Its wire format carries no + /// compatibility contract and breaking changes may be made to it without a + /// separate vote. + /// + /// # Compatibility + /// + /// Committing one is a durable compatibility break. A dataset version + /// written this way **cannot be opened** by Lance before v12.0.0 -- not + /// merely read as history. Those releases decode the manifest's inline + /// transaction while opening the dataset, and an operation they do not + /// know decodes to no operation at all, which fails the open. The fix + /// shipped in v12.0.0 and is not expected to be backported to earlier + /// release lines ([#9454]), so a dataset with one anywhere in its history + /// has a minimum reader version of v12.0.0 from then on. + /// + /// That is why this is opt-in per caller rather than something a version + /// of the library turns on. + /// + /// [#9454]: https://github.com/lance-format/lance/issues/9454 + /// + /// # Not sufficient on its own + /// + /// A release build also needs + /// [`LANCE_ENABLE_UNSTABLE_TRANSACTION_V2`](ENABLE_UNSTABLE_TRANSACTION_V2_ENV) + /// in the environment. This method says the calling code wants V2; the + /// variable says the deployment permits it. A library caller can set the + /// first and must not be able to grant itself the second. Debug builds + /// have the second already, so tests need no setup. + /// + /// Without either, [`Self::execute`] rejects such a transaction with + /// [`Error::NotSupported`], naming the gate that is missing. Neither has + /// any effect on any other operation. + /// + /// ``` + /// # use lance::dataset::CommitBuilder; + /// # use lance::{Dataset, Result}; + /// # use lance_table::transaction::Transaction; + /// # use std::sync::Arc; + /// # async fn commit(dataset: Arc, transaction: Transaction) -> Result { + /// CommitBuilder::new(dataset) + /// .with_experimental_composite_operations(true) + /// .execute(transaction) + /// .await + /// # } + /// ``` + pub fn with_experimental_composite_operations(mut self, allow: bool) -> Self { + self.allow_composite_operations = allow; + self + } + pub async fn execute(self, transaction: Transaction) -> Result { + if matches!(transaction.operation, Operation::CompositeOperation(_)) { + check_composite_operation_gates( + self.allow_composite_operations, + transaction_v2_enabled(), + )?; + } let timeout = self.timeout; if let Some(t) = timeout && t.is_zero() @@ -594,6 +658,37 @@ impl<'a> CommitBuilder<'a> { } } +/// The two gates on committing Transaction V2. +/// +/// `allowed_by_caller` is [`CommitBuilder::with_experimental_composite_operations`]: +/// this code path wants V2. `enabled_by_build` is [`transaction_v2_enabled`]: +/// this deployment permits it at all. They are separate because a library +/// caller can set the first and must not be able to grant itself the second. +/// +/// Taken as parameters rather than read here so both answers are testable +/// without toggling the build profile or the environment, the way +/// `feature_flags::supported_flags_when` is for overlay files. +fn check_composite_operation_gates(allowed_by_caller: bool, enabled_by_build: bool) -> Result<()> { + if !allowed_by_caller { + return Err(Error::not_supported_source( + "Transaction V2 is experimental; a CompositeOperation can only be committed \ + after opting in with CommitBuilder::with_experimental_composite_operations" + .into(), + )); + } + if !enabled_by_build { + return Err(Error::not_supported_source( + format!( + "Transaction V2 is experimental and not enabled in this build. Set \ + {ENABLE_UNSTABLE_TRANSACTION_V2_ENV}=1 to allow it. Its wire format \ + carries no compatibility contract and may change without a separate vote." + ) + .into(), + )); + } + Ok(()) +} + pub struct BatchCommitResult { pub dataset: Dataset, /// The final transaction that was committed. @@ -624,6 +719,31 @@ mod tests { use super::*; + /// Both gates on Transaction V2, in both directions. The build gate is + /// checked with explicit values rather than through the environment, + /// which is process-global and would race other tests. + #[rstest::rstest] + #[case::neither(false, false, "with_experimental_composite_operations")] + // The caller opt-in is reported first: a caller who has not asked for V2 + // at all should be told that, not told to change the deployment. + #[case::build_only(false, true, "with_experimental_composite_operations")] + #[case::caller_only(true, false, ENABLE_UNSTABLE_TRANSACTION_V2_ENV)] + fn test_composite_operations_need_both_gates( + #[case] allowed_by_caller: bool, + #[case] enabled_by_build: bool, + #[case] expected: &str, + ) { + let error = + check_composite_operation_gates(allowed_by_caller, enabled_by_build).unwrap_err(); + assert!(matches!(error, Error::NotSupported { .. }), "{error:?}"); + assert!(error.to_string().contains(expected), "{error}"); + } + + #[test] + fn test_composite_operations_pass_when_both_gates_are_open() { + check_composite_operation_gates(true, true).unwrap(); + } + fn sample_fragment() -> Fragment { let (major_version, minor_version) = LanceFileVersion::Stable.resolve().to_data_file_numbers(); diff --git a/rust/lance/src/io/commit/conflict_resolver.rs b/rust/lance/src/io/commit/conflict_resolver.rs index 1dd5edde987..5bdf9f8dd4a 100644 --- a/rust/lance/src/io/commit/conflict_resolver.rs +++ b/rust/lance/src/io/commit/conflict_resolver.rs @@ -16,6 +16,7 @@ use lance_index::mem_wal::{CompactedSsTable, MEM_WAL_INDEX_NAME}; use lance_select::{RowAddrTreeMap, RowSetOps}; use lance_table::format::IndexMetadata; use lance_table::format::overlay::OverlayCoverage; +use lance_table::transaction::action::Footprint; use lance_table::{format::Fragment, io::deletion::write_deletion_file}; use roaring::RoaringBitmap; use std::{ @@ -110,7 +111,11 @@ impl<'a> TransactionRebase<'a> { | Operation::UpdateMemWalState { .. } | Operation::Clone { .. } | Operation::Restore { .. } - | Operation::UpdateBases { .. } => Ok(Self { + | Operation::UpdateBases { .. } + // An action set can modify fragments, but conflicts against it are + // settled by comparing footprints, which are derived from the + // actions rather than from collected rebase state. + | Operation::CompositeOperation(_) => Ok(Self { transaction, affected_rows, initial_fragments: HashMap::new(), @@ -339,11 +344,54 @@ impl<'a> TransactionRebase<'a> { Operation::UpdateMemWalState { .. } => { self.check_update_mem_wal_state_txn(other_transaction, other_version) } - Operation::Clone { .. } => Ok(()), + // Clone has no checker of its own, so the cross-form rule has to be + // stated here: it is compatible with every named operation, but a + // concurrent action set still fails closed like everywhere else. + Operation::Clone { .. } => { + if matches!( + &other_transaction.operation, + Operation::CompositeOperation(_) + ) { + self.check_action_txn(other_transaction, other_version) + } else { + Ok(()) + } + } Operation::UpdateBases { .. } => { self.check_add_bases_txn(other_transaction, other_version) } + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } + } + } + + /// Whether an action-based transaction conflicts with `other_transaction`. + /// + /// Reached from both directions: when the transaction being committed is + /// action-based, and when a concurrent one is. + /// + /// Two action sets are compared by footprint. Anything else -- a named + /// operation on the other side -- conflicts unconditionally. The two forms + /// describe a change in different vocabularies, and deciding whether a + /// named operation's post-image overlaps an action set's coordinates would + /// mean specifying every named operation in terms of coordinates. Until that + /// exists, a mixed pair fails closed rather than guessing. + fn check_action_txn( + &mut self, + other_transaction: &Transaction, + other_version: u64, + ) -> Result<()> { + let (Operation::CompositeOperation(ours), Operation::CompositeOperation(theirs)) = + (&self.transaction.operation, &other_transaction.operation) + else { + return Err(self.retryable_conflict_err(other_transaction, other_version)); + }; + + if Footprint::from(ours).conflicts_with(&Footprint::from(theirs)) { + return Err(self.retryable_conflict_err(other_transaction, other_version)); } + Ok(()) } fn check_delete_txn( @@ -353,6 +401,11 @@ impl<'a> TransactionRebase<'a> { ) -> Result<()> { if let Operation::Delete { .. } = &self.transaction.operation { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } Operation::CreateIndex { .. } | Operation::ReserveFragments { .. } | Operation::Clone { .. } @@ -509,6 +562,11 @@ impl<'a> TransactionRebase<'a> { } match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } Operation::CreateIndex { .. } | Operation::ReserveFragments { .. } | Operation::Project { .. } @@ -702,6 +760,11 @@ impl<'a> TransactionRebase<'a> { } = &mut self.transaction.operation { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } Operation::Append { .. } | Operation::Clone { .. } // An overlay committed after this index's version is newer than @@ -941,6 +1004,11 @@ impl<'a> TransactionRebase<'a> { } = &self.transaction.operation { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } // Rewrite is only compatible with operations that don't touch // existing fragments or update fragments we don't touch. Operation::Append { .. } @@ -1131,6 +1199,11 @@ impl<'a> TransactionRebase<'a> { other_version: u64, ) -> Result<()> { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } Operation::Overwrite { .. } => { if self .transaction @@ -1180,6 +1253,11 @@ impl<'a> TransactionRebase<'a> { other_version: u64, ) -> Result<()> { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } // Append is not compatible with any operation that completely // overwrites the schema. Operation::Overwrite { .. } @@ -1210,6 +1288,11 @@ impl<'a> TransactionRebase<'a> { ) -> Result<()> { if let Operation::DataReplacement { replacements } = &self.transaction.operation { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } Operation::Append { .. } | Operation::Clone { .. } | Operation::UpdateConfig { .. } @@ -1389,6 +1472,11 @@ impl<'a> TransactionRebase<'a> { other_version: u64, ) -> Result<()> { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } Operation::Append { .. } | Operation::CreateIndex { .. } | Operation::ReserveFragments { .. } @@ -1487,6 +1575,11 @@ impl<'a> TransactionRebase<'a> { other_version: u64, ) -> Result<()> { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } // See the MemWAL exception in check_create_index_txn. Operation::CreateIndex { new_indices, .. } => { if new_indices.iter().any(|idx| idx.name == MEM_WAL_INDEX_NAME) { @@ -1524,6 +1617,11 @@ impl<'a> TransactionRebase<'a> { other_version: u64, ) -> Result<()> { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } Operation::Append { .. } | Operation::Delete { .. } | Operation::Overwrite { .. } @@ -1551,6 +1649,11 @@ impl<'a> TransactionRebase<'a> { other_version: u64, ) -> Result<()> { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } Operation::Overwrite { .. } | Operation::Restore { .. } => { Err(self.incompatible_conflict_err(other_transaction, other_version)) } @@ -1577,6 +1680,11 @@ impl<'a> TransactionRebase<'a> { other_version: u64, ) -> Result<()> { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } // Project is compatible with anything that doesn't change the schema Operation::Append { .. } | Operation::Update { .. } @@ -1613,6 +1721,11 @@ impl<'a> TransactionRebase<'a> { } = &self.transaction.operation { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } Operation::Overwrite { .. } => { // Updates to schema metadata or field metadata conflict with any kind // of overwrite. @@ -1676,6 +1789,11 @@ impl<'a> TransactionRebase<'a> { } = &self.transaction.operation { match &other_transaction.operation { + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } Operation::UpdateMemWalState { compacted_sstables: other_compacted_sstables, .. @@ -1774,8 +1892,27 @@ impl<'a> TransactionRebase<'a> { } Ok(()) } + // A concurrent action-based transaction is compared by action + // footprint rather than by operation pair. + Operation::CompositeOperation(_) => { + self.check_action_txn(other_transaction, other_version) + } // UpdateBases doesn't conflict with data operations - _ => Ok(()), + Operation::Append { .. } + | Operation::Delete { .. } + | Operation::Overwrite { .. } + | Operation::CreateIndex { .. } + | Operation::Rewrite { .. } + | Operation::DataReplacement { .. } + | Operation::DataOverlay { .. } + | Operation::Merge { .. } + | Operation::Restore { .. } + | Operation::ReserveFragments { .. } + | Operation::Update { .. } + | Operation::Project { .. } + | Operation::Clone { .. } + | Operation::UpdateConfig { .. } + | Operation::UpdateMemWalState { .. } => Ok(()), } } else { Err(wrong_operation_err(&self.transaction.operation)) @@ -1829,7 +1966,12 @@ impl<'a> TransactionRebase<'a> { | Operation::Clone { .. } | Operation::UpdateConfig { .. } | Operation::UpdateMemWalState { .. } - | Operation::UpdateBases { .. } => Ok(self.transaction), + | Operation::UpdateBases { .. } + // An action set needs no rewriting to move to a newer version: its + // minted ids are allocated when the actions are applied, against + // whichever manifest they land on, and its committed references + // name coordinates that do not move. + | Operation::CompositeOperation(_) => Ok(self.transaction), } } @@ -2357,6 +2499,9 @@ mod tests { use lance_table::format::IndexMetadata; use lance_table::io::deletion::{deletion_file_path, read_deletion_file}; + use lance_table::transaction::action::{ + Action as TxnAction, CompositeOperation, Ref as ActionRef, TombstoneFieldData, UserAction, + }; use super::*; use crate::dataset::transaction::{DataReplacementGroup, RewriteGroup, UpdateMap}; @@ -4651,6 +4796,95 @@ mod tests { assert!(rebase.check_txn(&txn2, 2).is_ok()); } + fn action_txn(actions: Vec) -> Transaction { + Transaction::new_from_version( + 1, + Operation::CompositeOperation(CompositeOperation::new(vec![UserAction::new( + "step", actions, + )])), + ) + } + + fn tombstone_txn(fragment: u64, field: u64) -> Transaction { + action_txn(vec![TxnAction::TombstoneFieldData(TombstoneFieldData { + fragment: ActionRef::Committed(fragment), + field_ids: vec![ActionRef::Committed(field)], + data_change: true, + })]) + } + + /// Assert the verdict holds whichever transaction is the one rebasing. + async fn assert_conflict(dataset: &Dataset, a: Transaction, b: Transaction, expected: bool) { + for (ours, theirs) in [(a.clone(), b.clone()), (b, a)] { + let mut rebase = TransactionRebase::try_new(dataset, ours, None) + .await + .unwrap(); + assert_eq!(rebase.check_txn(&theirs, 2).is_err(), expected); + } + } + + #[tokio::test] + async fn test_action_txns_on_disjoint_coordinates_do_not_conflict() { + let dataset = test_dataset(10, 2).await; + assert_conflict(&dataset, tombstone_txn(0, 0), tombstone_txn(1, 0), false).await; + assert_conflict(&dataset, tombstone_txn(0, 0), tombstone_txn(0, 1), false).await; + } + + #[tokio::test] + async fn test_action_txns_writing_the_same_field_data_conflict() { + let dataset = test_dataset(10, 2).await; + assert_conflict(&dataset, tombstone_txn(0, 0), tombstone_txn(0, 0), true).await; + } + + /// A named operation on the other side conflicts whatever it names, even + /// when it plainly touches nothing the action set does. Footprints are only + /// defined over actions, so there is nothing to compare a post-image + /// against; the pair fails closed rather than guessing. + #[rstest::rstest] + #[case::append(Operation::Append { + fragments: vec![{ + let mut fragment = Fragment::new(0); + fragment.physical_rows = Some(5); + fragment + }], + })] + #[case::delete(Operation::Delete { + updated_fragments: vec![], + deleted_fragment_ids: vec![7], + predicate: "a > 5".into(), + })] + #[case::update_config(Operation::UpdateConfig { + config_updates: None, + table_metadata_updates: None, + schema_metadata_updates: None, + field_metadata_updates: HashMap::new(), + })] + // The two operations whose checkers answer Ok without inspecting the other + // side: UpdateBases matched with a wildcard, and Clone has no checker at + // all. Both used to pass here while the mirror direction conflicted. + #[case::update_bases(Operation::UpdateBases { + new_bases: vec![lance_table::format::BasePath { + id: 1, + path: "s3://bucket/base".to_string(), + name: Some("base".to_string()), + is_dataset_root: false, + }], + })] + #[case::clone(Operation::Clone { + is_shallow: true, + ref_name: None, + ref_version: 1, + ref_path: "memory://source".to_string(), + branch_name: None, + })] + #[tokio::test] + async fn test_a_legacy_operation_conflicts_with_an_action_txn(#[case] legacy: Operation) { + let dataset = test_dataset(10, 2).await; + let legacy = Transaction::new_from_version(1, legacy); + + assert_conflict(&dataset, tombstone_txn(0, 0), legacy, true).await; + } + #[tokio::test] async fn test_add_bases_name_conflict() { let dataset = test_dataset(10, 2).await; @@ -4961,7 +5195,8 @@ mod tests { | Operation::UpdateConfig { .. } | Operation::UpdateBases { .. } | Operation::Restore { .. } - | Operation::UpdateMemWalState { .. } => Box::new(std::iter::empty()), + | Operation::UpdateMemWalState { .. } + | Operation::CompositeOperation(_) => Box::new(std::iter::empty()), Operation::Delete { updated_fragments, deleted_fragment_ids,