diff --git a/protos/transaction/actions.proto b/protos/transaction/actions.proto index 949d8c95ef1..3bc5e580c7e 100644 --- a/protos/transaction/actions.proto +++ b/protos/transaction/actions.proto @@ -13,7 +13,7 @@ package lance.table; /* * Action-based transactions (Transaction V2) — DRAFT. * - * A `UserOperation` replaces the single legacy `Operation` (see + * A `CompositeOperation` replaces the single legacy `Operation` (see * transaction.proto) with an ordered list of granular `Action`s that commit * atomically as one manifest change. This file is the wire draft only. * @@ -21,7 +21,7 @@ package lance.table; * ------ * The messages and field numbers here are NOT yet a stable contract. Library * support is read-side fail-closed only: a transaction carrying a - * `UserOperation` is rejected on load, and there is no write path. Apply, + * `CompositeOperation` is rejected on load, and there is no write path. Apply, * id translation, and conflict resolution are intentionally absent. * * DESIGN RATIONALE @@ -106,10 +106,10 @@ package lance.table; * id) that may not be committed yet. * * `committed` is an already-assigned id. `local` is a placeholder token minted - * by an `Add*` action earlier in the same `UserOperation`; it resolves to a + * by an `Add*` action earlier in the same `CompositeOperation`; it resolves to a * freshly-allocated committed id at apply, and re-resolves against the target's * counters on merge/rebase. `local` tokens are scoped to a single - * `UserOperation` and must be distinct within it (validated by the writer once + * `CompositeOperation` and must be distinct within it (validated by the writer once * the write path exists). */ message Ref { @@ -123,23 +123,21 @@ message Ref { * A user-facing, composable transaction: an ordered list of user actions that * commit atomically as a single manifest change. */ -message UserOperation { - // Human-readable description, e.g. "INSERT INTO t VALUES (1)". - string description = 1; +message CompositeOperation { // Unique identifier for this operation (matches Transaction.uuid semantics). - string uuid = 2; + string uuid = 1; // The dataset version this operation was planned against. - uint64 read_version = 3; + uint64 read_version = 2; // The ordered list of user actions applied by this operation. - repeated UserAction actions = 4; + repeated UserAction actions = 3; } /* - * A single user-recognizable step within a UserOperation (e.g. "append batch", + * A single user-recognizable step within a CompositeOperation (e.g. "append batch", * "rebuild index"). * * The description keeps the transaction history human-readable. When a range of - * transactions is squashed, each original UserOperation collapses into one + * transactions is squashed, each original CompositeOperation collapses into one * UserAction so the readable sequence survives; the action lists are flattened * when applied to the manifest. */ @@ -286,8 +284,8 @@ message AddBase { */ message TombstoneFieldData { Ref fragment = 1; - // Committed field ids whose current backing is tombstoned. - repeated uint64 field_ids = 2; + // The fields whose current backing is tombstoned. + repeated Ref field_ids = 2; // Data-change marker; see AddFragment.data_change. optional bool data_change = 3; } @@ -376,7 +374,7 @@ message UpdateCompactedSsTables { * no live fields). References an existing committed field id. */ message DropField { - uint64 field = 1; + Ref field = 1; } /* @@ -389,7 +387,7 @@ message DropField { * AddDataFile to rewrite the data. New facets are added as optional fields. */ message AlterField { - uint64 field = 1; + Ref field = 1; optional string name = 2; // New Arrow logical type (see Field.logical_type). The cast. optional string logical_type = 3; diff --git a/protos/transaction/transaction.proto b/protos/transaction/transaction.proto index 6da49fd8668..09ba8d41e37 100644 --- a/protos/transaction/transaction.proto +++ b/protos/transaction/transaction.proto @@ -343,7 +343,7 @@ message Transaction { DataOverlay data_overlay = 115; // Action-based transaction (Transaction V2). See actions.proto. // DRAFT: currently rejected on load; no write path. - UserOperation user_operation = 116; + CompositeOperation composite_operation = 116; } // Fields 200/202 (`blob_append` / `blob_overwrite`) previously represented blob dataset ops. diff --git a/rust/lance-table/src/format/fragment.rs b/rust/lance-table/src/format/fragment.rs index 149a9b44fc6..bf49d742fee 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. @@ -716,19 +732,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..145886ca9f0 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 (draft) //! 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..0aa301d36c2 --- /dev/null +++ b/rust/lance-table/src/transaction/action.rs @@ -0,0 +1,302 @@ +// 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 drafted 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 +//! action::translate lowering a named operation into actions +//! ``` +//! +//! # Stability +//! +//! Transaction V2 is a pre-vote draft. Nothing in this module is a compatibility +//! contract, and a transaction carrying a [`CompositeOperation`] is rejected outright +//! by libraries that predate it. + +/// 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, + 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 reset_table; +mod set_deletion_file; +mod tombstone_field_data; +mod translate; + +#[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 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. +#[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. +#[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 drafted 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..98df7506d7a --- /dev/null +++ b/rust/lance-table/src/transaction/action/add_base.rs @@ -0,0 +1,133 @@ +// 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)?; + + let conflicting = state + .bases() + .find(|base| base.name == self.base.name || base.path == self.base.path); + if let Some(conflicting) = conflicting { + return Err(Error::invalid_input(format!( + "Conflict detected: Base path with name '{:?}' or path '{}' already exists. \ + Existing: name='{:?}', path='{}'", + self.base.name, self.base.path, conflicting.name, conflicting.path + ))); + } + + 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: the manifest + /// requires both to be unique, so two operations claiming either collide. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + footprint.add(Coordinate::BaseName(self.base.name.clone())); + footprint.add(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"); + + 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("already exists"), + "unexpected error: {error}" + ); + } + + #[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("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..6fe28c7270c --- /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.add_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..0fe9b6fcf24 --- /dev/null +++ b/rust/lance-table/src/transaction/action/add_field.rs @@ -0,0 +1,238 @@ +// 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::{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()?; + + // 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 + } + + /// Nothing: the field does not exist in the read version. Attaching it + /// under a committed parent does not rewrite the parent's definition. + pub(super) fn footprint(&self, _footprint: &mut Footprint) {} +} + +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_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..310a6bca259 --- /dev/null +++ b/rust/lance-table/src/transaction/action/add_fragment.rs @@ -0,0 +1,219 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Mint a new, empty fragment. + +use super::Footprint; +use super::apply::ApplyState; +use super::proto::{data_change_from_wire, data_change_to_wire}; +use crate::format::{ExternalFile, Fragment, RowIdMeta, pb}; +use crate::rowids::version::RowDatasetVersionMeta; +use lance_core::Result; +use lance_core::deepsize::DeepSizeOf; + +/// Mint a new, empty fragment. +/// +/// Its data files arrive via [`AddDataFile`](super::AddDataFile) actions naming +/// this fragment's local token. A freshly-minted fragment has no deletion +/// vector: it has no committed rows to delete yet. +#[derive(Debug, Clone, PartialEq, DeepSizeOf)] +pub struct AddFragment { + /// Token standing in for the fragment id until it is allocated at apply. + pub local: u32, + /// 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.mint_fragment(self.local)?; + 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: the fragment does not exist in the read version, so no + /// concurrent writer can be naming it. + pub(super) fn footprint(&self, _footprint: &mut Footprint) {} +} + +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 { + local: value.local, + 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 { + local: message.local, + 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, Ref}; + use crate::transaction::test_support::sample_manifest; + use lance_file::version::ConcreteFileVersion; + + #[test] + fn test_add_fragment_and_data_file_mint_ids() { + let manifest = sample_manifest(); + let next = apply( + &manifest, + vec![ + Action::AddFragment(AddFragment { + 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)); + } +} 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..4c475ae7735 --- /dev/null +++ b/rust/lance-table/src/transaction/action/alter_field.rs @@ -0,0 +1,187 @@ +// 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::{Footprint, Ref}; +use crate::format::pb; +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 Arrow logical type. The cast. + 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 field = state + .schema_mut() + .field_by_id_mut(field_id) + .ok_or_else(|| { + Error::invalid_input(format!( + "AlterField names field {field_id}, which does not exist" + )) + })?; + 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.as_str().into(); + // 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. The data rewrite a cast needs is separate + /// actions, which record their own coordinates. + pub(super) fn footprint(&self, footprint: &mut Footprint) { + footprint.add_field_definition(self.field); + } +} + +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.clone(), + 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, + 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_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..206ef485e36 --- /dev/null +++ b/rust/lance-table/src/transaction/action/apply.rs @@ -0,0 +1,531 @@ +// 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. +//! +//! [`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}; +use crate::rowids::version::build_version_meta; +use crate::transaction::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. + /// + /// Unlike the legacy path this always requires a current manifest: an action + /// set describes a delta, so there is nothing for it to be a delta against + /// when the dataset does not exist yet. + pub(in crate::transaction) fn build_manifest_from_actions( + &self, + composite_operation: &CompositeOperation, + current_manifest: Option<&Manifest>, + current_indices: Vec, + transaction_file_path: &str, + config: &ManifestBuildConfig, + ) -> Result<(Manifest, Vec)> { + 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", + ) + })?; + if config.use_stable_row_ids && !current_manifest.uses_stable_row_ids() { + return Err(Error::not_supported_source( + "Cannot enable stable row ids on existing dataset".into(), + )); + } + + 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) + } +} + +/// 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 minted. + minted_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, + + /// 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(), + minted_fragments: HashSet::new(), + reserved_fragment_ids: None, + 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, + ) -> 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_minted_fragments(&mut next_row_id, new_version)?; + + 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)?; + let mut manifest = transaction.assemble_manifest( + Some(current_manifest), + schema, + fragments, + 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 + } + + 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.minted_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.minted_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); + } + + pub(super) 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.minted_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); + } + + 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_minted_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(()); + }; + let minted_ids = std::mem::take(&mut self.minted_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.minted_fragments = minted_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::transaction::Operation; + use crate::transaction::action::test_support::{added_field, apply, backed_manifest}; + use crate::transaction::action::{Action, AddDataFile, AddField, AddFragment}; + use crate::transaction::test_support::default_build_config; + use lance_file::version::ConcreteFileVersion; + + #[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 { + 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 very same actions against the manifest the first run + // produced re-resolves both local tokens against the newer counters. + let first = apply(&backed_manifest(), actions.clone()).unwrap(); + let second = apply(&first, actions).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..9ef57f506ad --- /dev/null +++ b/rust/lance-table/src/transaction/action/config_update.rs @@ -0,0 +1,496 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Apply config and metadata updates. + +use super::apply::ApplyState; +use super::proto::required; +use super::{ConfigMap, 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 dropping the field also + /// collides with an update to it. + 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.add_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. + if let Some(id) = update.field.committed() + && let Ok(id) = i32::try_from(id) + { + footprint.add_map_update(ConfigMap::Field(id), &update.updates); + } + } + } + + 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}; + use crate::transaction::action::{Action, AddField, CompositeOperation, Footprint, UserAction}; + 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, + } + } + + fn footprint(actions: Vec) -> Footprint { + Footprint::from(&CompositeOperation::new(vec![UserAction::new( + "step", actions, + )])) + } + + #[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), + })]); + + assert!(dropped.conflicts_with(&metadata)); + assert!(metadata.conflicts_with(&dropped)); + assert!(!other.conflicts_with(&metadata)); + } +} 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..0dbd604d950 --- /dev/null +++ b/rust/lance-table/src/transaction/action/footprint.rs @@ -0,0 +1,415 @@ +// 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::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 base path's name, which the manifest requires to be unique. + BaseName(Option), + /// 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 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), +} + +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::BaseName(_) + | Self::BaseLocation(_) + | Self::ConfigEntry { .. } => None, + } + } + + /// The field this coordinate belongs to, if it is field-scoped. + fn field(&self) -> Option { + match self { + Self::FieldData { field, .. } => Some(*field), + Self::FieldDefinition(id) => Some(*id), + // A field's metadata goes with the field, so dropping the field + // writes over a concurrent update to its metadata. + Self::ConfigEntry { + map: ConfigMap::Field(id), + .. + } => Some(*id), + Self::FragmentExistence(_) + | Self::FragmentDeletions(_) + | Self::BaseName(_) + | Self::BaseLocation(_) + | Self::ConfigEntry { .. } => 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, + } + } +} + +/// Everything an action set writes, in one set. +#[derive(Debug, Clone, Default, PartialEq)] +pub struct Footprint { + writes: HashSet, + /// Fragments this set removes outright. Removing a fragment writes every + /// coordinate inside it, which cannot be enumerated, so it is tracked + /// separately and matched against the other set by fragment id. + removed_fragments: HashSet, + /// Fields this set drops from the schema. Like a fragment removal, this + /// writes every coordinate belonging to the field -- its definition and its + /// data in every fragment -- so it is matched by field id. + /// + /// Only the named field, not its descendants: a footprint has no schema to + /// expand a struct with. A concurrent write to a child of a dropped struct + /// is therefore not caught here and fails when it is applied against the + /// version where the child no longer exists. + removed_fields: HashSet, + /// String maps this set replaces outright rather than merging into. Like a + /// fragment removal, this writes every key in the map, including keys it + /// does not name, so it is matched by map rather than by key. + replaced_maps: HashSet, + /// Whether this set rewrites the table wholesale. Such a set writes every + /// coordinate there is, including ones a concurrent set would only mint, so + /// it is tracked as a flag rather than enumerated. + exclusive: bool, +} + +impl Footprint { + pub fn conflicts_with(&self, other: &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 || other.exclusive { + return true; + } + if !self.writes.is_disjoint(&other.writes) { + return true; + } + // Two sets replacing the same map collide even when neither names a + // key, since clearing a map is a replacement with no entries. + if !self.replaced_maps.is_disjoint(&other.replaced_maps) { + return true; + } + self.removes_something_touched_by(other) || other.removes_something_touched_by(self) + } + + /// Whether this set wipes out something -- a fragment, a field, a whole + /// string map -- that `other` also writes to. + fn removes_something_touched_by(&self, other: &Self) -> bool { + other.writes.iter().any(|coordinate| { + coordinate + .fragment() + .is_some_and(|id| self.removed_fragments.contains(&id)) + || coordinate + .field() + .is_some_and(|id| self.removed_fields.contains(&id)) + || coordinate + .config_map() + .is_some_and(|map| self.replaced_maps.contains(map)) + }) + } + + pub(super) fn add(&mut self, coordinate: Coordinate) { + self.writes.insert(coordinate); + } + + /// The data of each field within `fragment`. A fragment or field minted in + /// this same operation records nothing: no concurrent writer can be naming + /// one. + pub(super) fn add_field_data(&mut self, fragment: Ref, fields: impl IntoIterator) { + let Some(fragment) = fragment.committed() else { + return; + }; + for field in fields.into_iter().filter_map(committed_field) { + self.add(Coordinate::FieldData { fragment, field }); + } + } + + /// A field's entry in the schema. + pub(super) fn add_field_definition(&mut self, field: Ref) { + if let Some(field) = committed_field(field) { + self.add(Coordinate::FieldDefinition(field)); + } + } + + pub(super) fn remove_fragment(&mut self, fragment: u64) { + self.add(Coordinate::FragmentExistence(fragment)); + self.removed_fragments.insert(fragment); + } + + /// 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 add_map_update(&mut self, map: ConfigMap, update: &UpdateMap) { + if update.replace { + self.replaced_maps.insert(map); + return; + } + for entry in &update.update_entries { + self.add(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; + } + + pub(super) fn remove_field(&mut self, field: Ref) { + if let Some(field) = committed_field(field) { + self.add(Coordinate::FieldDefinition(field)); + self.removed_fields.insert(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::{ + Action, AddBase, AddDataFile, AddField, AddFragment, AlterField, DropField, RemoveFragment, + SetDeletionFile, TombstoneFieldData, UserAction, + }; + use arrow_schema::{DataType, Field as ArrowField}; + use lance_core::datatypes::Field; + use lance_file::version::ConcreteFileVersion; + use rstest::rstest; + + fn footprint(actions: Vec) -> Footprint { + Footprint::from(&CompositeOperation::new(vec![UserAction::new( + "step", actions, + )])) + } + + fn add_fragment(local: u32) -> Action { + Action::AddFragment(AddFragment { + local, + 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 { + Action::AddDataFile(AddDataFile { + fragment, + file: DataFile::new_unstarted("data/x.lance", ConcreteFileVersion::V2_0), + field_ids: fields + .iter() + .map(|field| Ref::Committed(*field as u64)) + .collect(), + data_change: true, + }) + } + + 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), + }) + } + + #[test] + fn test_minting_actions_write_nothing() { + let minting = footprint(vec![ + add_fragment(0), + Action::AddField(AddField { + local: 1, + parent: None, + def: Field::try_from(ArrowField::new("new", DataType::Int32, true)).unwrap(), + }), + 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, + )] + #[case::same_field_definition( + vec![Action::AlterField(AlterField { field: Ref::Committed(1), name: Some("a".into()), logical_type: None, nullable: None })], + 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![Action::DropField(DropField { field: Ref::Committed(1) })], + vec![Action::AlterField(AlterField { field: Ref::Committed(1), name: None, logical_type: None, nullable: Some(true) })], + true, + )] + #[case::dropping_a_field_collides_with_rewriting_its_data( + vec![Action::DropField(DropField { field: Ref::Committed(1) })], + vec![tombstone(0, &[1])], + true, + )] + #[case::dropping_a_field_leaves_other_fields_alone( + vec![Action::DropField(DropField { field: Ref::Committed(1) })], + vec![tombstone(0, &[2])], + false, + )] + #[case::dropping_a_field_leaves_deletions_alone( + vec![Action::DropField(DropField { field: Ref::Committed(1) })], + vec![set_deletion_file(0)], + false, + )] + #[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, + )] + 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); + } +} 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..c7c7f2a4b6e --- /dev/null +++ b/rust/lance-table/src/transaction/action/proto.rs @@ -0,0 +1,307 @@ +// 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 { + // uuid and read_version mirror the enclosing Transaction and are + // stamped in by its conversion. + uuid: String::new(), + read_version: 0, + 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 drafted 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 drafted \ + 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, 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 { + 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::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..43cd41fdd89 --- /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 { + 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..3e4697eb6e3 --- /dev/null +++ b/rust/lance-table/src/transaction/action/reserve_fragment_ids.rs @@ -0,0 +1,122 @@ +// 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). +#[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. + 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}; + + fn reserve(count: u32) -> Action { + Action::ReserveFragmentIds(ReserveFragmentIds { count }) + } + + fn add_fragment(local: u32) -> Action { + Action::AddFragment(AddFragment { + 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/reset_table.rs b/rust/lance-table/src/transaction/action/reset_table.rs new file mode 100644 index 00000000000..c39198d7282 --- /dev/null +++ b/rust/lance-table/src/transaction/action/reset_table.rs @@ -0,0 +1,169 @@ +// 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 { + 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::{CompositeOperation, Footprint, UserAction}; + + let footprint = |actions| { + Footprint::from(&CompositeOperation::new(vec![UserAction::new( + "step", actions, + )])) + }; + + 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 { + 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())); + } +} 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..02eac9b28e3 --- /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.add(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..15a92932d2a --- /dev/null +++ b/rust/lance-table/src/transaction/action/test_support.rs @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Fixtures shared by the per-action test modules. + +use super::{Action, CompositeOperation, 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()) +} + +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..6cecf3ed2df --- /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.add_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/action/translate.rs b/rust/lance-table/src/transaction/action/translate.rs new file mode 100644 index 00000000000..153dba7f894 --- /dev/null +++ b/rust/lance-table/src/transaction/action/translate.rs @@ -0,0 +1,433 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright The Lance Authors + +//! Translating a legacy [`Operation`] into the action vocabulary. +//! +//! Each named operation is a fixed recipe over the granular actions. Expressing +//! the recipe here rather than in the commit loop is what lets several +//! operations be squashed into one commit: once both are action sets, combining +//! them is concatenation. +//! +//! Translation is fail-closed. An operation whose recipe is not written yet, or +//! one carrying a detail the actions cannot express, is rejected rather than +//! silently translated into something narrower. +//! +//! `Merge` and `Project` are not translated. Both hand over a whole new schema +//! rather than a description of what changed, so recovering the delta needs the +//! read version's schema to diff against -- which this conversion, taking only +//! the operation, does not have. The actions themselves are sufficient: +//! `Project` is a set of [`DropField`](super::DropField)s and `Merge` a set of +//! [`AddField`](super::AddField)s plus their data files. + +use super::{ + Action, AddBase, AddDataFile, AddFragment, Ref, RemoveFragment, SetDeletionFile, + TombstoneFieldData, UserAction, +}; +use crate::format::Fragment; +use crate::transaction::{DataReplacementGroup, Operation}; +use lance_core::{Error, Result}; + +impl TryFrom<&Operation> for Vec { + type Error = Error; + + fn try_from(operation: &Operation) -> Result { + match operation { + Operation::Append { fragments } => Ok(vec![UserAction::new( + format!("append {} fragments", fragments.len()), + append_actions(fragments)?, + )]), + Operation::Delete { + updated_fragments, + deleted_fragment_ids, + predicate, + } => Ok(vec![UserAction::new( + format!("delete rows matching {predicate}"), + delete_actions(updated_fragments, deleted_fragment_ids), + )]), + Operation::UpdateBases { new_bases } => Ok(vec![UserAction::new( + format!("add {} base paths", new_bases.len()), + new_bases + .iter() + .enumerate() + .map(|(index, base)| { + Action::AddBase(AddBase { + local: index as u32, + base: base.clone(), + }) + }) + .collect(), + )]), + Operation::DataReplacement { replacements } => Ok(vec![UserAction::new( + format!("replace data files in {} fragments", replacements.len()), + data_replacement_actions(replacements)?, + )]), + other => Err(Error::not_supported(format!( + "translating a {} operation into actions", + other.name() + ))), + } + } +} + +fn append_actions(fragments: &[Fragment]) -> Result> { + let mut actions = Vec::with_capacity(fragments.len() * 2); + for (index, fragment) in fragments.iter().enumerate() { + let local = index as u32; + let physical_rows = fragment.physical_rows.ok_or_else(|| { + Error::invalid_input( + "an appended fragment must know its physical row count to become an AddFragment", + ) + })?; + // A fragment being appended has no committed state, so anything that + // only makes sense against committed data means the caller built the + // operation by hand out of an existing fragment. + if fragment.deletion_file.is_some() || !fragment.overlays.is_empty() { + return Err(Error::invalid_input(format!( + "appended fragment {} carries a deletion file or overlays, which an append \ + cannot produce", + fragment.id + ))); + } + + actions.push(Action::AddFragment(AddFragment { + local, + physical_rows: physical_rows as u64, + row_id_meta: fragment.row_id_meta.clone(), + last_updated_at_version_meta: fragment.last_updated_at_version_meta.clone(), + created_at_version_meta: fragment.created_at_version_meta.clone(), + data_change: true, + })); + + for file in &fragment.files { + actions.push(Action::AddDataFile(AddDataFile { + fragment: Ref::Local(local), + file: file.clone(), + field_ids: committed_field_refs(file.fields.as_ref())?, + data_change: true, + })); + } + } + Ok(actions) +} + +/// A legacy delete replaces whole fragments, but the only thing it ever changes +/// on one is its deletion file, so that is what the translation carries over. +fn delete_actions(updated_fragments: &[Fragment], deleted_fragment_ids: &[u64]) -> Vec { + let mut actions = Vec::with_capacity(updated_fragments.len() + deleted_fragment_ids.len()); + for fragment in updated_fragments { + actions.push(Action::SetDeletionFile(SetDeletionFile { + fragment: fragment.id, + deletion_file: fragment.deletion_file.clone(), + data_change: true, + })); + } + for id in deleted_fragment_ids { + actions.push(Action::RemoveFragment(RemoveFragment { + fragment: Ref::Committed(*id), + data_change: true, + })); + } + actions +} + +/// Replacing a field's data is a drop of the old backing file followed by an +/// add of the new one. The legacy path swaps the path on the existing file in +/// place instead, so the resulting fragment holds the same set of data files +/// but not necessarily in the same order -- files are addressed by field, so +/// the order carries no meaning. +fn data_replacement_actions(replacements: &[DataReplacementGroup]) -> Result> { + let mut actions = Vec::with_capacity(replacements.len() * 2); + for DataReplacementGroup(fragment_id, new_file) in replacements { + let fragment = Ref::Committed(*fragment_id); + actions.push(Action::TombstoneFieldData(TombstoneFieldData { + fragment, + field_ids: committed_field_refs(new_file.fields.as_ref())?, + data_change: true, + })); + actions.push(Action::AddDataFile(AddDataFile { + fragment, + file: new_file.clone(), + field_ids: committed_field_refs(new_file.fields.as_ref())?, + data_change: true, + })); + } + Ok(actions) +} + +fn committed_field_refs(field_ids: &[i32]) -> Result> { + field_ids + .iter() + .map(|id| { + u64::try_from(*id).map(Ref::Committed).map_err(|_| { + Error::invalid_input(format!( + "a data file in this operation lists field id {id}, which is not a committed \ + field" + )) + }) + }) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::{ + BasePath, DataFile, DeletionFile, DeletionFileType, IndexMetadata, Manifest, RowIdMeta, + }; + use crate::rowids::{RowIdSequence, write_row_ids}; + use crate::transaction::Transaction; + use crate::transaction::action::CompositeOperation; + use crate::transaction::test_support::{ + default_build_config, make_stable_row_id_manifest, sample_manifest, + }; + use lance_file::version::ConcreteFileVersion; + use std::sync::Arc; + + /// Build the same manifest twice -- once down the legacy path, once by + /// translating the operation to actions -- and assert they agree. + fn assert_parity(manifest: &Manifest, operation: Operation) -> Manifest { + let (legacy, legacy_indices) = build(manifest, operation.clone()); + + let actions = Vec::::try_from(&operation).unwrap(); + let (translated, translated_indices) = build( + manifest, + Operation::CompositeOperation(CompositeOperation::new(actions)), + ); + + // Data files are addressed by field, so the two paths are allowed to + // hold them in different orders within a fragment. + assert_eq!(translated.fragments.len(), legacy.fragments.len()); + for (translated, legacy) in translated.fragments.iter().zip(legacy.fragments.iter()) { + assert_eq!(sorted_files(translated), sorted_files(legacy)); + assert_eq!( + Fragment { + files: Vec::new(), + ..translated.clone() + }, + Fragment { + files: Vec::new(), + ..legacy.clone() + } + ); + } + assert_eq!(translated.schema, legacy.schema); + assert_eq!(translated.base_paths, legacy.base_paths); + assert_eq!(translated.next_row_id, legacy.next_row_id); + assert_eq!(translated.max_fragment_id, legacy.max_fragment_id); + assert_eq!(translated_indices, legacy_indices); + translated + } + + fn sorted_files(fragment: &Fragment) -> Vec { + let mut files = fragment.files.clone(); + files.sort_by(|a, b| a.path.cmp(&b.path)); + files + } + + fn build(manifest: &Manifest, operation: Operation) -> (Manifest, Vec) { + Transaction::new(manifest.version, operation, None) + .build_manifest( + Some(manifest), + Vec::new(), + "tx.txn", + &default_build_config(), + ) + .unwrap() + } + + fn appendable_fragment(path: &str) -> Fragment { + let mut fragment = Fragment::new(0); + fragment.physical_rows = Some(10); + fragment.files.push(DataFile::new( + path, + vec![0], + vec![0], + ConcreteFileVersion::V2_0, + None, + None, + )); + fragment + } + + fn manifest_with_fragments(fragments: Vec) -> Manifest { + let mut manifest = sample_manifest(); + manifest.fragments = Arc::new(fragments); + manifest + } + + #[test] + fn test_append_matches_the_legacy_path() { + let manifest = manifest_with_fragments(vec![appendable_fragment("data/0.lance")]); + let next = assert_parity( + &manifest, + Operation::Append { + fragments: vec![ + appendable_fragment("data/1.lance"), + appendable_fragment("data/2.lance"), + ], + }, + ); + + // The appended fragments take ids from the manifest's counter, not the + // zero they were handed to the operation with. + assert_eq!( + next.fragments.iter().map(|f| f.id).collect::>(), + vec![0, 1, 2] + ); + } + + #[test] + fn test_append_assigns_stable_row_ids_like_the_legacy_path() { + let mut existing = appendable_fragment("data/1.lance"); + existing.id = 1; + existing.row_id_meta = Some(RowIdMeta::Inline( + write_row_ids(&RowIdSequence::from(0u64..10)).into(), + )); + let manifest = make_stable_row_id_manifest(vec![existing]); + + let next = assert_parity( + &manifest, + Operation::Append { + fragments: vec![appendable_fragment("data/2.lance")], + }, + ); + + assert_eq!(next.next_row_id, 1010); + let appended = next.fragments.iter().find(|f| f.id == 2).unwrap(); + assert!(appended.row_id_meta.is_some()); + assert!(appended.created_at_version_meta.is_some()); + } + + #[test] + fn test_append_rejects_a_fragment_without_a_row_count() { + let operation = Operation::Append { + fragments: vec![Fragment::new(0)], + }; + let error = Vec::::try_from(&operation).unwrap_err(); + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + } + + #[test] + fn test_delete_matches_the_legacy_path() { + let manifest = manifest_with_fragments(vec![appendable_fragment("data/0.lance"), { + let mut fragment = appendable_fragment("data/1.lance"); + fragment.id = 1; + fragment + }]); + + let mut updated = manifest.fragments[0].clone(); + updated.deletion_file = Some(DeletionFile { + read_version: manifest.version, + id: 7, + file_type: DeletionFileType::Array, + num_deleted_rows: Some(3), + base_id: None, + }); + + let next = assert_parity( + &manifest, + Operation::Delete { + updated_fragments: vec![updated], + deleted_fragment_ids: vec![1], + predicate: "id > 5".into(), + }, + ); + + assert_eq!(next.fragments.len(), 1); + assert!(next.fragments[0].deletion_file.is_some()); + } + + #[test] + fn test_update_bases_matches_the_legacy_path() { + let manifest = manifest_with_fragments(vec![appendable_fragment("data/0.lance")]); + let next = assert_parity( + &manifest, + Operation::UpdateBases { + new_bases: vec![ + BasePath::new(0, "s3://bucket/a".into(), Some("a".into()), false), + BasePath::new(0, "s3://bucket/b".into(), Some("b".into()), false), + ], + }, + ); + + assert_eq!(next.base_paths.len(), 2); + } + + #[test] + fn test_data_replacement_matches_the_legacy_path() { + let mut fragment = appendable_fragment("data/0.lance"); + fragment.files.push(DataFile::new( + "data/0b.lance", + vec![1], + vec![0], + ConcreteFileVersion::V2_0, + None, + None, + )); + let manifest = manifest_with_fragments(vec![fragment]); + + let replacement = DataFile::new( + "data/0-new.lance", + vec![0], + vec![0], + ConcreteFileVersion::V2_0, + None, + None, + ); + let next = assert_parity( + &manifest, + Operation::DataReplacement { + replacements: vec![DataReplacementGroup(0, replacement)], + }, + ); + + let paths = sorted_files(&next.fragments[0]) + .iter() + .map(|file| file.path.clone()) + .collect::>(); + assert_eq!(paths, vec!["data/0-new.lance", "data/0b.lance"]); + } + + #[test] + fn test_data_replacement_of_an_unbacked_field_is_rejected() { + let manifest = manifest_with_fragments(vec![appendable_fragment("data/0.lance")]); + let operation = Operation::DataReplacement { + replacements: vec![DataReplacementGroup( + 0, + DataFile::new( + "data/0-new.lance", + vec![9], + vec![0], + ConcreteFileVersion::V2_0, + None, + None, + ), + )], + }; + let actions = Vec::::try_from(&operation).unwrap(); + let error = Transaction::new( + manifest.version, + Operation::CompositeOperation(CompositeOperation::new(actions)), + None, + ) + .build_manifest( + Some(&manifest), + Vec::new(), + "tx.txn", + &default_build_config(), + ) + .unwrap_err(); + + // The legacy path treats this as an all-NULL column gaining real data; + // the action form has no way to say "drop this if it is there". + assert!(matches!(error, Error::InvalidInput { .. }), "{error:?}"); + } + + #[test] + fn test_an_untranslated_operation_is_rejected() { + let operation = Operation::ReserveFragments { num_fragments: 3 }; + let error = Vec::::try_from(&operation).unwrap_err(); + assert!(matches!(error, Error::NotSupported { .. }), "{error:?}"); + assert!(error.to_string().contains("ReserveFragments"), "{error}"); + } +} diff --git a/rust/lance-table/src/transaction/conflicts.rs b/rust/lance-table/src/transaction/conflicts.rs index ad942d6c182..dc929bd9adf 100644 --- a/rust/lance-table/src/transaction/conflicts.rs +++ b/rust/lance-table/src/transaction/conflicts.rs @@ -863,6 +863,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 e47bc139e58..5390854faae 100644 --- a/rust/lance-table/src/transaction/manifest_build.rs +++ b/rust/lance-table/src/transaction/manifest_build.rs @@ -463,6 +463,16 @@ impl Transaction { config: &ManifestBuildConfig, read_version_state: Option>, ) -> Result<(Manifest, Vec)> { + if let Operation::CompositeOperation(composite_operation) = &self.operation { + return self.build_manifest_from_actions( + composite_operation, + current_manifest, + current_indices, + transaction_file_path, + config, + ); + } + if config.use_stable_row_ids && config.migration_next_row_id.is_none() && current_manifest @@ -1287,29 +1297,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); - - // Clean up data files that only contain tombstoned fields - Self::remove_tombstoned_data_files(&mut final_fragments); + Self::normalize_fragments(&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 @@ -1325,55 +1326,14 @@ 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, - )?; - } - // Carried from the manifest this one is derived from. `new_from_previous` - // zeroes both feature words, so `apply_feature_flags` cannot see the - // previous state and every ordinary commit would otherwise drop the bit. - if let Some(current_manifest) = current_manifest { - inherit_mem_wal_index_catchup(&mut manifest, current_manifest)?; - } + let mut manifest = self.assemble_manifest( + current_manifest, + schema, + final_fragments, + reference_paths, + overwrite_storage_format, + config, + )?; // Set after apply_feature_flags, which resets both flag words: activation // is the one place the bit is turned on, and it must survive that reset. @@ -1413,10 +1373,6 @@ impl Transaction { manifest.writer_feature_flags |= FLAG_MEM_WAL_INDEX_CATCHUP; } - manifest.set_timestamp(config.timestamp_nanos); - - manifest.update_max_fragment_id(); - match &self.operation { Operation::Overwrite { config_upsert_values: Some(tm), @@ -1599,6 +1555,106 @@ 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. + /// + /// `overwrite_storage_format` replaces the inherited storage format with the + /// user-requested one, for operations that rewrite the whole dataset. + pub(super) fn assemble_manifest( + &self, + current_manifest: Option<&Manifest>, + schema: lance_core::datatypes::Schema, + fragments: Vec, + 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, + )?; + } + // Carried from the manifest this one is derived from. `new_from_previous` + // zeroes both feature words, so `apply_feature_flags` cannot see the + // previous state and every ordinary commit would otherwise drop the bit. + if let Some(current_manifest) = current_manifest { + inherit_mem_wal_index_catchup(&mut manifest, current_manifest)?; + } + + 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 1874984864b..df6356630a2 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; @@ -220,6 +221,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)] @@ -270,6 +279,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"), } } } @@ -329,6 +339,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 5082ec8b57e..278c606db92 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, @@ -416,18 +417,14 @@ impl TryFrom for Transaction { .map(DataOverlayGroup::try_from) .collect::>>()?, }, - Some(pb::transaction::Operation::UserOperation(_)) => { + Some(pb::transaction::Operation::CompositeOperation(composite_operation)) => { // Action-based transactions (Transaction V2) are a draft 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", - )); + // 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( @@ -732,6 +729,15 @@ impl From<&Transaction> for pb::Transaction { .collect::>(), }) } + Operation::CompositeOperation(composite_operation) => { + let mut message = pb::CompositeOperation::from(composite_operation); + // The operation's identity and read version are the enclosing + // transaction's; the wire carries them in both places so a + // squashed operation keeps its own provenance. + message.uuid = value.uuid.clone(); + message.read_version = value.read_version; + pb::transaction::Operation::CompositeOperation(message) + } }; let transaction_properties = value @@ -836,6 +842,7 @@ mod tests { use super::*; use crate::format::DataFile; use crate::format::overlay::OverlayCoverage; + use crate::transaction::action::{Action, AddFragment, UserAction}; #[test] fn test_data_overlay_operation_roundtrips() { @@ -882,28 +889,64 @@ mod tests { } #[test] - fn test_user_operation_rejected_on_load() { - // Action-based transactions (Transaction V2) are a draft 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 { + 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 operation repeats the envelope's identity so a squashed operation + // keeps the provenance of the commit it came from. + match &message.operation { + Some(pb::transaction::Operation::CompositeOperation(composite_operation)) => { + assert_eq!(composite_operation.uuid, uuid); + assert_eq!(composite_operation.read_version, 4); + } + other => panic!("expected CompositeOperation, got {other:?}"), + } + + assert_eq!(Transaction::try_from(message).unwrap(), transaction); + } + + #[test] + fn test_unimplemented_action_fails_closed_on_load() { + // The drafted 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::UserOperation( - pb::UserOperation { - description: "INSERT INTO t VALUES (1)".to_string(), + operation: Some(pb::transaction::Operation::CompositeOperation( + pb::CompositeOperation { uuid: Uuid::new_v4().to_string(), read_version: 1, 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 { - 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 5b9c76bc90c..8a577efce00 100644 --- a/rust/lance/src/dataset/tests/dataset_transactions.rs +++ b/rust/lance/src/dataset/tests/dataset_transactions.rs @@ -1395,3 +1395,330 @@ async fn test_alter_columns_materializes_fresh_field_id_in_every_fragment() { let batch = dataset.scan().try_into_batch().await.unwrap(); assert_eq!(batch["a"].as_primitive::().values(), &[1, 2]); } + +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; + use crate::dataset::{CommitBuilder, InsertBuilder, WriteParams}; + use arrow_array::{Int32Array, RecordBatch}; + use arrow_schema::{DataType, Field, Schema}; + use lance_table::format::DataFile; + use lance_table::transaction::action::{ + Action, AddDataFile, AddField, AddFragment, CompositeOperation, DropField, Ref, + TombstoneFieldData, UserAction, + }; + use lance_table::transaction::{Operation, Transaction}; + + /// 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() + } + + async fn commit(dataset: Dataset, actions: Vec) -> Dataset { + let read_version = dataset.version().version; + CommitBuilder::new(Arc::new(dataset)) + .execute(Transaction::new( + read_version, + Operation::CompositeOperation(CompositeOperation::new(vec![UserAction::new( + "step", actions, + )])), + None, + )) + .await + .unwrap() + } + + #[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 { + 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 { + 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 { + 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()); + } + } + + #[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 { + 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()) + .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) + .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()) + .execute(tombstone()) + .await + .unwrap(); + + let error = CommitBuilder::new(dataset) + .with_max_retries(0) + .execute(tombstone()) + .await + .unwrap_err(); + assert!( + error.to_string().contains("preempted"), + "unexpected error: {error}" + ); + } +} diff --git a/rust/lance/src/io/commit/conflict_resolver.rs b/rust/lance/src/io/commit/conflict_resolver.rs index 99b5188f91b..1af930e0e04 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::{CompositeOperation, Footprint, UserAction}; use lance_table::{format::Fragment, io::deletion::write_deletion_file}; use roaring::RoaringBitmap; use std::{ @@ -89,7 +90,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(), @@ -314,9 +319,36 @@ impl<'a> TransactionRebase<'a> { 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. + fn check_action_txn( + &mut self, + other_transaction: &Transaction, + other_version: u64, + ) -> Result<()> { + let (Some(ours), Some(theirs)) = ( + footprint_of(&self.transaction.operation), + footprint_of(&other_transaction.operation), + ) else { + // One side does not decompose into actions yet, so there is nothing + // to compare and the conservative answer stands. + return Err(self.retryable_conflict_err(other_transaction, other_version)); + }; + + if ours.conflicts_with(&theirs) { + return Err(self.retryable_conflict_err(other_transaction, other_version)); + } + Ok(()) + } + fn check_delete_txn( &mut self, other_transaction: &Transaction, @@ -324,6 +356,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 { .. } @@ -478,6 +515,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 { .. } @@ -641,6 +683,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 @@ -840,6 +887,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 { .. } @@ -1030,6 +1082,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 @@ -1079,6 +1136,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 { .. } @@ -1109,6 +1171,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 { .. } @@ -1287,6 +1354,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 { .. } @@ -1385,6 +1457,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) { @@ -1422,6 +1499,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 { .. } @@ -1449,6 +1531,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)) } @@ -1475,6 +1562,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 { .. } @@ -1511,6 +1603,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. @@ -1574,6 +1671,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, .. @@ -1727,7 +1829,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), } } @@ -2238,6 +2345,23 @@ fn overlay_group_coverage(group: &DataOverlayGroup) -> RoaringBitmap { union } +/// The set of coordinates an operation writes, or `None` when it does not +/// decompose into actions yet. +/// +/// A legacy operation gets a footprint through its action translation, so an +/// action set can be compared against a concurrent named operation without +/// either side needing an entry in the operation-pair matrix. +fn footprint_of(operation: &Operation) -> Option { + match operation { + Operation::CompositeOperation(composite_operation) => { + Some(Footprint::from(composite_operation)) + } + other => Vec::::try_from(other) + .ok() + .map(|actions| Footprint::from(&CompositeOperation::new(actions))), + } +} + fn wrong_operation_err(op: &Operation) -> Error { Error::internal(format!("function called against a wrong operation: {}", op)) } @@ -2256,6 +2380,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, Ref as ActionRef, RemoveFragment, TombstoneFieldData, + }; use super::*; use crate::dataset::transaction::{DataReplacementGroup, RewriteGroup}; @@ -4158,6 +4285,97 @@ 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; + } + + #[tokio::test] + async fn test_removing_a_fragment_conflicts_with_a_concurrent_delete_on_it() { + let dataset = test_dataset(10, 2).await; + let removal = action_txn(vec![TxnAction::RemoveFragment(RemoveFragment { + fragment: ActionRef::Committed(0), + data_change: true, + })]); + let delete = Transaction::new_from_version( + 1, + Operation::Delete { + updated_fragments: vec![dataset.fragments()[0].clone()], + deleted_fragment_ids: vec![], + predicate: "a > 5".into(), + }, + ); + + assert_conflict(&dataset, removal, delete, true).await; + } + + #[tokio::test] + async fn test_an_action_txn_does_not_conflict_with_a_concurrent_append() { + let dataset = test_dataset(10, 2).await; + let append = Transaction::new_from_version( + 1, + Operation::Append { + fragments: vec![{ + let mut fragment = Fragment::new(0); + fragment.physical_rows = Some(5); + fragment + }], + }, + ); + + // An append only mints; it names nothing the action set could touch. + assert_conflict(&dataset, tombstone_txn(0, 0), append, false).await; + } + + #[tokio::test] + async fn test_an_untranslatable_operation_stays_conservative() { + let dataset = test_dataset(10, 2).await; + let project = Transaction::new_from_version( + 1, + Operation::Project { + schema: dataset.schema().clone(), + preserves_nullability: false, + }, + ); + + assert_conflict(&dataset, tombstone_txn(0, 0), project, true).await; + } + #[tokio::test] async fn test_add_bases_name_conflict() { let dataset = test_dataset(10, 2).await; @@ -4468,7 +4686,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,