From 865e74420a930aab878350d3a20af3deb3573e00 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 27 Mar 2024 10:52:48 +0100 Subject: [PATCH] multimap: don't sort values, use push & swap_remove --- crates/table/src/btree_index.rs | 29 +++++++++++------------- crates/table/src/btree_index/multimap.rs | 20 +++++----------- 2 files changed, 19 insertions(+), 30 deletions(-) diff --git a/crates/table/src/btree_index.rs b/crates/table/src/btree_index.rs index 1c46f96f54d..53606e971ec 100644 --- a/crates/table/src/btree_index.rs +++ b/crates/table/src/btree_index.rs @@ -130,16 +130,17 @@ impl TypedIndex { /// this will behave oddly; it may return an error, /// or may insert a nonsense value into the index. /// Note, however, that it will not invoke undefined behavior. - fn insert(&mut self, cols: &ColList, row_ref: RowRef<'_>) -> Result { + fn insert(&mut self, cols: &ColList, row_ref: RowRef<'_>) -> Result<(), InvalidFieldError> { fn insert_at_type( this: &mut MultiMap, cols: &ColList, row_ref: RowRef<'_>, - ) -> Result { + ) -> Result<(), InvalidFieldError> { debug_assert!(cols.is_singleton()); let col_pos = cols.head(); let key = row_ref.read_col(col_pos).map_err(|_| col_pos)?; - Ok(this.insert(key, row_ref.pointer())) + this.insert(key, row_ref.pointer()); + Ok(()) } match self { TypedIndex::Bool(ref mut this) => insert_at_type(this, cols, row_ref), @@ -158,7 +159,8 @@ impl TypedIndex { TypedIndex::AlgebraicValue(ref mut this) => { let key = row_ref.project_not_empty(cols)?; - Ok(this.insert(key, row_ref.pointer())) + this.insert(key, row_ref.pointer()); + Ok(()) } } } @@ -379,7 +381,7 @@ impl BTreeIndex { /// This index will extract the necessary values from `row` based on `self.cols`. /// /// Return false if `ptr` was already indexed prior to this call. - pub fn insert(&mut self, cols: &ColList, row_ref: RowRef<'_>) -> Result { + pub fn insert(&mut self, cols: &ColList, row_ref: RowRef<'_>) -> Result<(), InvalidFieldError> { self.idx.insert(cols, row_ref) } @@ -420,12 +422,11 @@ impl BTreeIndex { &mut self, cols: &ColList, rows: impl IntoIterator>, - ) -> Result { - let mut all_inserted = true; + ) -> Result<(), InvalidFieldError> { for row_ref in rows { - all_inserted &= self.insert(cols, row_ref)?; + self.insert(cols, row_ref)?; } - Ok(all_inserted) + Ok(()) } /// Deletes all entries from the index, leaving it empty. @@ -517,14 +518,10 @@ mod test { prop_assert_eq!(index.idx.len(), 0); prop_assert_eq!(index.contains_any(&value), false); - prop_assert_eq!(index.insert(&cols, row_ref).unwrap(), true); + index.insert(&cols, row_ref).unwrap(); prop_assert_eq!(index.idx.len(), 1); prop_assert_eq!(index.contains_any(&value), true); - // Try inserting again, it should fail. - prop_assert_eq!(index.insert(&cols, row_ref).unwrap(), false); - prop_assert_eq!(index.idx.len(), 1); - prop_assert_eq!(index.delete(&cols, row_ref).unwrap(), true); prop_assert_eq!(index.idx.len(), 0); prop_assert_eq!(index.contains_any(&value), false); @@ -548,7 +545,7 @@ mod test { ); // Insert. - prop_assert_eq!(index.insert(&cols, row_ref).unwrap(), true); + index.insert(&cols, row_ref).unwrap(); // Inserting again would be a problem. prop_assert_eq!(index.idx.len(), 1); @@ -581,7 +578,7 @@ mod test { let ptr = table.insert(&mut blob_store, &row).unwrap().1; val_to_ptr.insert(x, ptr); let row_ref = table.get_row_ref(&blob_store, ptr).unwrap(); - prop_assert_eq!(index.insert(&cols, row_ref).unwrap(), true); + index.insert(&cols, row_ref).unwrap(); } fn test_seek(index: &BTreeIndex, val_to_ptr: &HashMap, range: impl RangeBounds, expect: impl IntoIterator) -> TestCaseResult { diff --git a/crates/table/src/btree_index/multimap.rs b/crates/table/src/btree_index/multimap.rs index 4819f68bad6..1260ac2b8bb 100644 --- a/crates/table/src/btree_index/multimap.rs +++ b/crates/table/src/btree_index/multimap.rs @@ -23,16 +23,9 @@ impl MultiMap { /// Inserts the relation `key -> val` to this multimap. /// - /// Returns false if `key -> val` was already in the map. - pub fn insert(&mut self, key: K, val: V) -> bool { - let vset = self.map.entry(key).or_default(); - // Use binary search to maintain the sort order. - // This is used to determine in `O(log(vset.len()))` whether `val` was already present. - let Err(idx) = vset.binary_search(&val) else { - return false; - }; - vset.insert(idx, val); - true + /// The map does not check whether `key -> val` was already in the map. + pub fn insert(&mut self, key: K, val: V) { + self.map.entry(key).or_default().push(val); } /// Deletes `key -> val` from this multimap. @@ -40,10 +33,9 @@ impl MultiMap { /// Returns whether `key -> val` was present. pub fn delete(&mut self, key: &K, val: &V) -> bool { if let Some(vset) = self.map.get_mut(key) { - // The `vset` is sorted so we can binary search. - if let Ok(idx) = vset.binary_search(val) { - // Maintain the sorted order. Don't use `swap_remove`! - vset.remove(idx); + // The `vset` is not sorted, so we have to do a linear scan first. + if let Some(idx) = vset.iter().position(|v| v == val) { + vset.swap_remove(idx); return true; } }