diff --git a/docs/src/format/table/transaction.md b/docs/src/format/table/transaction.md index f7115b7b500..36c97f705ea 100644 --- a/docs/src/format/table/transaction.md +++ b/docs/src/format/table/transaction.md @@ -334,6 +334,7 @@ The Restore operation reverts the table to a previous version. It's generally as other operation. Here are the operations that conflict with Restore: - UpdateMemWalState +- UpdateConfig (only if it updates schema or field metadata, which a restore rewinds) ### ReserveFragments @@ -431,6 +432,7 @@ An UpdateConfig operation only modifies table config and tends to be compatible are the operations that conflict with UpdateConfig: - Overwrite +- Restore (only if the UpdateConfig updates schema or field metadata) - UpdateConfig (only if the two operations modify the same config) ### DataReplacement diff --git a/rust/lance/src/io/commit/conflict_resolver.rs b/rust/lance/src/io/commit/conflict_resolver.rs index 1dd5edde987..a4790ef1195 100644 --- a/rust/lance/src/io/commit/conflict_resolver.rs +++ b/rust/lance/src/io/commit/conflict_resolver.rs @@ -308,6 +308,16 @@ impl<'a> TransactionRebase<'a> { { return Err(self.retryable_conflict_err(other_transaction, other_version)); } + // A restore replaces the manifest with an older one, schema and field + // metadata included. Rebasing a metadata update over it reinstates + // metadata the restore discarded, and re-reading cannot help: the + // update was computed against a schema the table no longer has. + if (matches!(ours, Operation::Restore { .. }) && updates_schema_or_field_metadata(theirs)) + || (updates_schema_or_field_metadata(ours) + && matches!(theirs, Operation::Restore { .. })) + { + return Err(self.incompatible_conflict_err(other_transaction, other_version)); + } let op = &self.transaction.operation; match op { @@ -4221,6 +4231,44 @@ mod tests { ); } + /// A restore rewinds schema and field metadata with the rest of the + /// manifest, so a metadata update must not rebase over it; a plain config + /// upsert carries no metadata and still rebases. + #[rstest::rstest] + #[case::field_metadata(Some(HashMap::from([(0, HashMap::from([("fresh".to_string(), "{}".to_string())]))])), None, true)] + #[case::schema_metadata(None, Some(HashMap::from([("owner".to_string(), "refresh".to_string())])), true)] + #[case::config_only(None, None, false)] + fn test_update_config_conflicts_with_restore( + #[case] field_metadata: Option>>, + #[case] schema_metadata: Option>, + #[case] conflicts: bool, + ) { + let restore = Transaction::new(0, Operation::Restore { version: 1 }, None); + let update = create_update_config_for_test( + Some(HashMap::from([("key".to_string(), "value".to_string())])), + None, + schema_metadata, + field_metadata, + ); + // Either commit order: the metadata the restore discarded must not + // come back, whichever transaction rebases. + for (ours, theirs) in [ + (update.clone(), restore.operation.clone()), + (restore.operation, update), + ] { + let mut rebase = TransactionRebase { + transaction: Transaction::new(0, ours, None), + initial_fragments: HashMap::new(), + modified_fragment_ids: HashSet::new(), + affected_rows: None, + conflicting_frag_reuse_indices: Vec::new(), + conflicting_mem_wal_compacted_sstables: Vec::new(), + }; + let result = rebase.check_txn(&Transaction::new(0, theirs, None), 1); + assert_eq!(result.is_err(), conflicts, "{result:?}"); + } + } + #[test] fn test_mem_wal_install_conflicts_with_merge() { let mem_wal_index = IndexMetadata {